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

\$\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.

\$\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 n²/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>.
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.