JS/jQuery critique

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
thinsoldier
Forum Contributor
Posts: 367
Joined: Fri Jul 20, 2007 11:29 am
Contact:

JS/jQuery critique

Post by thinsoldier »

Hello I just a built a small tool for myself but I really think the JS/jQuery could be a lot better.
A lot more object oriented I guess.
The script is toward the bottom of the file.

It simply observes a list of radio buttons and a text field. If a radio is chosen and the field contains all integers it makes an ajax request and updates the contents of a div.

Then further down it watches a select field and updates another select field.

Any suggestions?

Code: Select all

<!DOCTYPE html>
<html>
<head>
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.1/jquery.min.js"></script>
</head>
<body>
<form>
<h2>From</h2>
<div id="fromlist">
<ul>
	<li> 
	<input type="radio" id="from0" value="download" name="from_table">
	<label for="from0">download</label> </li>
	<li> 
	<input type="radio" id="from1" value="competition_policy" name="from_table">
	<label for="from1">competition_policy</label> </li>
	<li> 
	<input type="radio" id="from2" value="licencing" name="from_table">
	<label for="from2">licencing</label> </li>
	<li> 
	<input type="radio" id="from3" value="news" name="from_table">
	<label for="from3">news</label> </li>
	<li> 
	<input type="radio" id="from4" value="decisions" name="from_table">
	<label for="from4">decisions</label> </li>
</ul>
</div>

<div>
id# <input type="text" name="from_id" id="from_id_field">
</div>


<div id="preview">

</div>


<h2>To</h2>

<div>
Table 
<select id="tolist" name="to_table">
	<option value="">Select</option>
	<option value="download">download</option>
	<option value="competition_policy">competition_policy</option>
	<option value="licencing">licencing</option>
	<option value="news">news</option>
	<option value="decisions">decisions</option>
</select>
</div>


<div>
Category <select name="category[]" id="cat"></select>
</div>

<br>
<input type="submit">

</form>



<script>
$("#fromlist").click(watchFromList);
$("#from_id_field").keyup(watchFromID);
$("#tolist").change(watchToList);

var from_table;
var from_id;
var to_table;

function watchToList(event)
{
	to_table = event.target.value;
	// returns long string of option tags <option value="34">foo</option>
	$.get('_section-api.php',{'cmd':'categories','table':to_table })
		.done( function(data) { $('#cat').html(data); });
}


function watchFromList(event)
{
	if(event.target.value)
	{
		from_table = event.target.value;
		ajaxForFrom();
	}
}

function watchFromID(event)
{
	var test = event.target.value.match(/^\d{1,}$/)
	if(test){ 

		from_id = event.target.value;
		ajaxForFrom();
	} else {
		console.log('id test fail');
		$('#preview').html('');
	}
}



function ajaxForFrom()
{
	if( from_table && from_id )
	{
		// sample response: {"id":"196","category":"Revoked","headline":"Holdings Revocation Notice"}
		$.getJSON('_section-api.php',{'cmd':'preview','table':from_table, 'id':from_id })
			.done(function(data) { updatePreview(data) })
	}
	else {
		console.log('not enough info to pull FROM item details')
	}
}

function updatePreview( jsn )
{
	$('#preview').html('');
	var string = '';
	for( i in jsn )
	{
		 string += i + ': ' + jsn[i] + "<br>";
	}
	$('#preview').html(string);
}

</script>

</body>
</html>
Warning: I have no idea what I'm talking about.
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Re: JS/jQuery critique

Post by pickle »

There's nothing really wrong with the way you're doing it as far as I can see. Generally though, accepted practice is to use the .on() handler. And since your functions are so short, I'd simply include them as an anonymous function in your call to .on()...

Code: Select all

$("#fromlist").on('click',function(e){
   //code you currently have in watchToList()
  //"this" refers to "event.target" - could simplify the code a bit
});
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
Post Reply