1
\$\begingroup\$

I have a list of cities, and a set of "travel time" between any given two cities. The "travel time" is fixed and not necessarily dependent on absolute distance between the two cities, and it currently exists in an Excel doc that's manually maintained.

I wanted users to be able to specify two cities from two dropdown menus and see how long it takes to travel between those cities without submitting anything to the page.

Challenges I ran into:

  • How to minimize redundant data in the database
  • How to delay running the AJAX request until both variables have been specified (this is my first time using AJAX for...anything.)

Here is the completed code - it works, but it seems a little clumsy. I'd really appreciate any feedback. (The database entries are fake and only for testing purposes.)

AJAX:

// Create empty variables in preparation for the two values from the two dropdown menus
var first; 
var second; 
function firstly(x) { 
// Grab the value from the first dropdown and set it to 'first'
 first = x.value;
 if (typeof second != 'undefined') { // If 'second' isn't undefined, run the 'route' function with the two variables
 route(first, second);
 } // Otherwise do nothing
}
// This is the same function but for the second variable
function secondly(x) {
 second = x.value;
 if (typeof first != 'undefined') {
 route(first, second);
 }
}
// This is the AJAX function that takes two parameters
function route(first, second) {
 var xmlhttp;
 if (window.XMLHttpRequest) { // code for IE7+, Firefox, Chrome, Opera, Safari
 xmlhttp=new XMLHttpRequest();
 } else { // code for IE6, IE5
 xmlhttp=new ActiveXObject("Microsoft.XMLHTTP");
 }
 xmlhttp.onreadystatechange=function() {
 // if the XMLHttpRequest is successful
 if (xmlhttp.readyState==4 && xmlhttp.status==200)
 {
 // Then populate div id="distance" with the responseText
 document.getElementById("distance").innerHTML=xmlhttp.responseText;
 }
 }
 // Actually send the data using the variables assigned with firstly() and secondly()
 xmlhttp.open("GET","route.php?first="+first+"&second="+second,true);
 xmlhttp.send();
}

PHP:

<?php 
require_once('config.php'); 
$first = $_GET['first'];
$second = $_GET['second'];
 if ($first === $second) {
 echo "These two cities are the same: '" . $first . "' and '" . $second . "'.";
 } else {
 $select = "SELECT * FROM distances WHERE (first='$first' OR second='$first') AND (first='$second' OR second='$second');";
 $result = mysql_query($select);
 if (!$result) { die ('Error: ' . mysql_error()); };
 while($row = mysql_fetch_array($result)) { echo $first . " to " . $second . " will take " . $row['distance'] . " hours.<br />"; }
 }
?>

DROPDOWNS:

<?php 
// This creates the two dropdown menus based on what's available in the database
$select = "SELECT DISTINCT(first) AS first FROM distances";
 $result = mysql_query($select);
 if (!$result) { die ('Error: ' . mysql_error()); };
// Store a distinct list of all cities in the table in an array
 $cities = array();
 $count = 0;
 while ($row = mysql_fetch_array($result)) { 
 $count++;
 $cities[$count] = $row['first'];
 }
?>
<form>
<!-- When an option is selected, it runs firstly() -->
 <select name="first" onchange="firstly(this)">
 <option selected value=""></option>
 <?php 
 // Loop through all the entries in $cities and create an option for each one
 for ($i = 1; $i <= count($cities); ++$i) {
 echo "<option value='" . $cities[$i] . "'>" . $cities[$i] . "</option>";
 }
 ?>
 </select>
<br />
<!-- Repeat the process for the destination city -->
 <select name="second" onchange="secondly(this)">
 <option selected value=""></option>
 <?php 
 for ($i = 1; $i <= count($cities); ++$i) {
 echo "<option value='" . $cities[$i] . "'>" . $cities[$i] . "</option>";
 }
 ?>
 </select>
<br />
</form>
 <div id="distance"></div>

Gist link: https://gist.github.com/phirephoenix/5523188

Quentin Pradet
7,0641 gold badge25 silver badges44 bronze badges
asked May 6, 2013 at 3:31
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Sometimes, it's good to code from scratch. However, in your case, it seems like you'd be maintaining this code for a while. Scratch coding isn't that fun in the long run because you will run into bugs, cross-browser issues, and security.

I would suggest you use a framework, even a simple one, to distribute the effort of maintenance to the community so you could focus on what's important in your app.

For suggestions, I recommend starting with jQuery for the JS since it's the most common. For PHP, your choice, but I do recommend CodeIgniter since it's light weight. For templating, you could use Mustache to avoid mixing logic to your templates.

Just so you know, jQuery offers custom builds. You can build jQuery only to contain essentials, like AJAX and only some utility functions for as low as 10KB in size. Your JS could have been just the following if you used jQuery:

var distance = $("#distance");
function secondly(x) {
 if(first) route(first, x.value);
}
function route(first, second) {
 $.ajax({
 type : 'get',
 data : {
 first : first,
 second : second
 }
 dataType : 'html'
 }).done(function(responseText){
 distance.text(responseText);
 });
}

I couldn't provide PHP, SQL or templating suggestions since it's been a while since I have used them (I'm doing full JS at the moment). I'll leave it to the others to review your PHP and SQL.

answered May 6, 2013 at 4:40
\$\endgroup\$
2
\$\begingroup\$

How to minimize redundant data in the database

You said that the distance between two cities can't be guessed. Does that mean that distance(Philadelphia, Boston) is different from distance(Philadelphia, New York) + distance(New York, Boston)? If so, the only thing you can do is to store n2/2 distances, n being the number of cities (since distance(A, B) == distance(B, A)). If you don't want to have to guess which city goes first, you can specify some order: the first city in lexicographical order always goes first.

How to delay running the AJAX request until both variables have been specified (this is my first time using AJAX for...anything.)

It seems you got this one correctly looking at your code. (If not, never leave non-working code in a way that suggests it works.)

<?php 
require_once('config.php'); 
$first = $_GET['first'];
$second = $_GET['second'];

Never Trust User Input. Sure, you provided a drop-down, but nothing prevents anyone from sending something else. $first can be anything: you should check that it looks like a city.

 if ($first === $second) {
 echo "These two cities are the same: '" . $first . "' and '" . $second . "'.";

You could also do this check using AJAX, which would save an extra roundtrip. Of course, leave it in there too: your web site should also work when users don't have JavaScript enabled for whatever reason.

 } else {
 $select = "SELECT * FROM distances WHERE (first='$first' OR second='$first') AND (first='$second' OR second='$second');";
 $result = mysql_query($select);

Never Trust User Input. Depending on the value of $first and $second, $select could now contain a DROP or anything else. Even if you have sanitized your inputs earlier, you now need to use a prepared statement, either with mysqli (close to mysql_*) or PDO (object-oriented).

 if (!$result) { die ('Error: ' . mysql_error()); };
 while($row = mysql_fetch_array($result)) { echo $first . " to " . $second . " will take " . $row['distance'] . " hours.<br />"; }
 }
?>

If you only expect one result, the while is not necessary. Also, be wary in general not to mix SQL and HTML: different parts of your application should be responsible for querying the database and printing the output. However, for such a small script, it's OK.

<?php 
// This creates the two dropdown menus based on what's available in the database
$select = "SELECT DISTINCT(first) AS first FROM distances";
 $result = mysql_query($select);
 if (!$result) { die ('Error: ' . mysql_error()); };
// Store a distinct list of all cities in the table in an array
 $cities = array();
 $count = 0;
 while ($row = mysql_fetch_array($result)) { 
 $count++;
 $cities[$count] = $row['first'];
 }
?>
<form>
<!-- When an option is selected, it runs firstly() -->
 <select name="first" onchange="firstly(this)">
 <option selected value=""></option>
 <?php 
 // Loop through all the entries in $cities and create an option for each one
 for ($i = 1; $i <= count($cities); ++$i) {
 echo "<option value='" . $cities[$i] . "'>" . $cities[$i] . "</option>";
 }
 ?>
 </select>
<br />
<!-- Repeat the process for the destination city -->
 <select name="second" onchange="secondly(this)">
 <option selected value=""></option>
 <?php 
 for ($i = 1; $i <= count($cities); ++$i) {
 echo "<option value='" . $cities[$i] . "'>" . $cities[$i] . "</option>";
 }
 ?>
 </select>
<br />
</form>
 <div id="distance"></div>
  1. Mixing SQL and HTML makes the code a bit hard to read.
  2. This applies to all your code: please indent in a consistent way.
  3. Make sure that your code works without JavaScript: provide an action for the form that will do mostly the same thing than Ajax.
  4. <br /> is unnecessary, use <p>.
answered May 6, 2013 at 6:44
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.