3
\$\begingroup\$

This JavaScript file is part of a large Ruby on Rails app. Any feedback is appreciated.

Update 3.7.16: as per @Jonah's suggestion, here's a description of the code's purpose:

I've built a bus-tracking application using Ruby on Rails that displays a user's location as well as the location of several nearby buses all on one map. Each bus has its own marker with an event listener that opens an info window on click.

The program is currently set up to initialize one info window, and then to update the content and position of that info window when a bus marker is clicked, such that it displays a unique descriptor based on paramaters of the bus it's attached to. It creates a marker template and then placing it many times using a loop through every element in the array of buses, displayBuses. It updates the info window by assigning the corresponding element in the array of descriptors, busInfo.

I originally coded this to create an array of markers and an array of descriptors (each containing one element for each bus) and to create a new info window for each bus attached to its marker, and containing its descriptor. That wasn't working the way I expected it to, so I modified some code I found on Stack Overflow to create the current model that initializes only one info window and updates it each time a marker is clicked. I'm particularly curious to hear folks' thoughts on this method, as it seems a little hacky to me.

// This is a manifest file that'll be compiled into application.js, which will include all the files
// listed below.
//
// Any JavaScript/Coffee file within this directory, lib/assets/javascripts, vendor/assets/javascripts,
// or any plugin's vendor/assets/javascripts directory can be referenced here using a relative path.
//
// It's not advisable to add code directly here, but if you do, it'll appear at the bottom of the
// compiled file.
//
// Read Sprockets README (https://github.com/rails/sprockets#sprockets-directives) for details
// about supported directives.
//
//= require jquery
//= require jquery_ujs
//= require bootstrap-sprockets
//= require_tree .

$(document).ready(function() {
  var currentLat = gon.current_lat;
  var currentLng = gon.current_lng;
  var displayBuses = gon.display_buses;

  var currentLatLng = new google.maps.LatLng(currentLat, currentLng);

  function initMap() {
    var map = new google.maps.Map(document.getElementById('map'), {
      center: currentLatLng,
      zoom: 15,
      scrollwheel: false
    });

    setMarkers(map);
  }

  function setMarkers(map) {
    var userMarker = new google.maps.Marker({
      position: currentLatLng,
      map: map,
      title: 'User',
      animation: google.maps.Animation.DROP
    });

    var userInfo = new google.maps.InfoWindow({
      content: 'You are here.'
    });

    google.maps.event.addListener(userMarker, 'click', function() {
      userInfo.open(map, userMarker);
    });

    var imgHt = 30;
    var imgWd = imgHt * 1.5;

    var busIcon = {
      url: "<%= asset_path 'bus-marker.png' %>",
      scaledSize: new google.maps.Size(imgHt, imgWd),
      origin: new google.maps.Point(0, 0),
      anchor: new google.maps.Point(imgWd / 2, imgHt)
    };

    var infoWindow = new google.maps.InfoWindow;

    var busLatLng, busMarker, i;

    var busInfo = [];

    for (i = 0; i < displayBuses.length; i++) {
      var busLat = displayBuses[i].LATITUDE;
      var busLng = displayBuses[i].LONGITUDE;
      var busNum = displayBuses[i].VEHICLE;
      var busRoute = displayBuses[i].ROUTE;
      var busStop = displayBuses[i].TIMEPOINT;

      busLatLng = new google.maps.LatLng(busLat, busLng);

      busMarker = new google.maps.Marker({
        position: busLatLng,
        map: map,
        title: 'Bus number ' + busNum,
        icon: busIcon,
        animation: google.maps.Animation.BOUNCE
      });

      busInfo[i] = new google.maps.InfoWindow({
        content: 'Bus number ' + busNum + ' is on Route ' + busRoute + '. The next stop is at ' + busStop + '.'
      });

      google.maps.event.addListener(busMarker, 'click', (function(busMarker, busInfo) {
        return function() {
          infoWindow.setContent(busInfo.content);
          infoWindow.open(map, busMarker);
        }
      })(busMarker, busInfo[i]));
    }
  };

  google.maps.event.addDomListener(window, 'load', initMap);
});
\$\endgroup\$
  • \$\begingroup\$ Can you add a high level description of what the code is doing? Or even better: screenshots or a working a fiddle? From a reviewers perspective, those things make the job easier and more pleasant, and will therefore be more likely to attract good answers. \$\endgroup\$ – Jonah Mar 7 '16 at 2:14
  • \$\begingroup\$ What's the variable gon, where do you create that? \$\endgroup\$ – duncan Apr 10 '16 at 8:18
  • \$\begingroup\$ gon is a ruby gem for importing ruby variables into javascript: github.com/gazay/gon \$\endgroup\$ – Andrew Breunig Apr 11 '16 at 12:40
1
\$\begingroup\$

Firstly you have one infowindow, infoWindow. Then you loop over all the buses, creating a new infowindow for each:

  busInfo[i] = new google.maps.InfoWindow({
    content: 'Bus number ' + busNum + ' is on Route ' + busRoute + '. The next stop is at ' + busStop + '.'
  });

You then pass that in as a parameter when you're setting up the event listeners:

  google.maps.event.addListener(busMarker, 'click', (function(busMarker, busInfo) {
    return function() {
      infoWindow.setContent(busInfo.content);
      infoWindow.open(map, busMarker);
    }
  })(busMarker, busInfo[i]));

But you're not using it as an infowindow, other than to read its content property. You'd be as well just having the array containing just a string:

  busInfo[i] = 'Bus number ' + busNum + ' is on Route ' + busRoute + '. The next stop is at ' + busStop + '.';

  google.maps.event.addListener(busMarker, 'click', (function(busMarker, busInfo) {
    return function() {
      infoWindow.setContent(busInfo);
      infoWindow.open(map, busMarker);
    }
  })(busMarker, busInfo[i]));

Secondly, when you create your busIcon, you set the scaledSize like this:

scaledSize: new google.maps.Size(imgHt, imgWd),

You've got the width and height parameters the wrong way around; check the documentation. It should be:

scaledSize: new google.maps.Size(imgWd, imgHt),
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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