1
\$\begingroup\$

I am using PHP to get data from a MySQL database, and use the data to drop multiple markers into Google Maps. Please give me some suggestions for my code.

<?php
require_once 'Common/system_start.php';
$sql="select * from tab_mem order by sn desc ";
$result=  mysql_query($sql);
$data=mysql_fetch_assoc($result);
$row=mysql_num_rows($result);

header("Content-type: text/xml");

// Iterate through the rows, adding XML nodes for each

while ($row = @mysql_fetch_assoc($result)){
// ADD TO XML DOCUMENT NODE
$node = $dom->createElement("marker");
$newnode = $parnode->appendChild($node);

$newnode->setAttribute("address", $row['address']);

}

 echo $dom->saveXML();
?>

HTML5

  <script type="text/javascript" src="https://maps.googleapis.com/maps/api/js?sensor=true"></script>
    <script type="text/javascript">


     var map;
     var geocoder;
 //initial GPS


  navigator.geolocation.watchPosition(onSuccess, onError, { frequency: 3000 });



 function onSuccess(position){


//get GPS latlong
var latlng = new google.maps.LatLng(position.coords.latitude, position.coords.longitude);



// display map
  var gmap = new google.maps.Map(
      map_div, {
        zoom:16,
        center: latlng,
        mapTypeId: google.maps.MapTypeId.ROADMAP
     });


//drop the marker
function drop() {
  for (var i = 0; i < a; i++) {
    setTimeout(function() {
    addMarker();
   }, i * 200);
  }
 }



     //decode address to latlng
//pass address data from PHP
 while (markers.length){
var address="";
 geocoder=new google.maps.Geocoder();
 alert("...");
   }
  geocoder.geocode({'address':address},function(results,status){
    if(status==google.maps.GeocoderStatus.OK){


        function addMarker() { markers.push(new google.maps.Marker({

                          postion:results().geometry.location,
                          map:gmap,
                        animation:google.map.animation.DROP


  }));
      }

    // Sets the map on all markers in the array.
       // function setAllMap(map) {
      // for (var i = 0; i < markers.length; i++) {
     // markers[i].setMap(map);
    //}
   // }



//markerNearBy.setMap(map);
}else{
alert("Geocode was not sucessful for the following reason:"+status);
}
});
}
}





 function onError(error) {
          alert('code: '+ error.code+ '\n' 
          +'message: ' + error.message + '\n');    
      }

</script>
</head>
<body onLoad="load()">
<!-- map display here-->
<div id="map_div""></div>

\$\endgroup\$
2
  • 2
    \$\begingroup\$ fixing the whitespace so it's a little easier to read would help \$\endgroup\$
    – AD7six
    Commented May 9, 2012 at 8:51
  • \$\begingroup\$ Completely agree, its sooo hard to read XD \$\endgroup\$
    – mseancole
    Commented May 9, 2012 at 13:56

2 Answers 2

Reset to default
1
\$\begingroup\$

PHP

The commonly accepted approach to SQL statements is to make commands all caps, so I would convert my SQL statements to look like so...

$sql="SELECT * FROM tab_mem ORDER BY sn DESC ";

In other words, only the information from your tables should be lower case. This makes it easier to read.

You really should never use error suppression. Sure if it isn't a fatal error its nice to ignore those errors and let the program continue running, but you should never, no matter how minor, let an error go unresolved. It doesn't help your program and could potentially cause more problems rather than less.

You just declared $data and $row then turn around name a new $row that you just set $data to, never having used the old ones. Why? If you aren't going to use that information, don't set it. I'd just remove the $data and first $row variables. This is one of the few times I think its ok to set variables in an expression, otherwise I'd point this out too.

Don't know if it is just stackoverflow messing with your code or not, but ensure that your code is indented! If it is you can ignore this line, but judging from later bits of code, I have to assume that its not just stackoverflow. Examples given in JavaScript section.

Where in the world did $dom and $parnode come from? Does this even work?

JavaScript

I'll admit to not being well versed in JavaScript myself, only enough to write minor scripts for myself. However a lot of what I know from PHP can be trasferred over to JavaScript, and my first problem here is that you have JavaScript functions within your JavaScript functions. Move those! I can't imagine when that would be necessary. All functions should be within the same scope. Minor exceptions would be lamda functions, though I hold that if you can make it into a function to be reused, do so.

Your code indentation here is really lacking. If I did not have a text editor with matching-brace highlighting there is no way in the world I could read this. An I'm still having trouble anyways. Here's a couple of examples of good indentation:

var gmap = new google.maps.Map(
    map_div, {
        zoom:16,
        center: latlng,
        mapTypeId: google.maps.MapTypeId.ROADMAP
    }
);
//etc...
while (markers.length){
    var address="";
    geocoder=new google.maps.Geocoder();
    alert("...");
}

Proper indentation would also show you that you have an extra closing brace } on the end of your onSuccess() function. Which probably causes errors.

I'm going to stop here. I've already pointed out quite a bit for you to look at. If after you have corrected these things you want me to look over it again you can update your question with the new code and drop me a comment and I'll try to come back to it. Good Luck!

\$\endgroup\$
1
\$\begingroup\$

+1 to @showerhead, and:

  1. I would use longer table and attribute names than tab_mem and sn. Longer names would make the code more readable since readers don't have to decode the abbreviations every time.

  2. I'd check the result of mysql_query($sql). It returns FALSE on error. The code could write an error message and a proper HTTP error code (500 Internal Server Error, for example) to the user and could stop the execution of the script. I think there is no point to continue the execution.

  3. header("Content-type: text/xml") should be at the beginning of the script. Previous error messages or notices could cause a Warning: Cannot modify header information - headers already sent error message here. See: Headers already sent by PHP

\$\endgroup\$
3
  • 1
    \$\begingroup\$ @ShuyouChang: I actually meant to touch on that #1 but got sidetracked. I would just add that should also include variable names. $parnode while obvious if you sat down to think about it, should probably still be expanded to $parentnode or if you want to keep it short then just $parent. \$\endgroup\$
    – mseancole
    Commented May 9, 2012 at 20:01
  • \$\begingroup\$ @showerhead: Or it could be $parentNode. Camelcase is easy to read. \$\endgroup\$
    – palacsint
    Commented May 9, 2012 at 20:13
  • 1
    \$\begingroup\$ I guess my finger missed the shift key, cause I could have sworn I capped that, but good point! \$\endgroup\$
    – mseancole
    Commented May 9, 2012 at 20:22

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.