TransWikia.com

Google Maps marker management for games

Code Review Asked by Joeyboy on October 27, 2021

Is there a better way to design this code? I have read that too many function parameters is a sign of poorly design code, and would love some feedback on this code.

The functions Google Maps markers and update the state.

Also any other feedback unrelated is welcome.

Functions:

// Initialising map and markers
export const handleApiLoaded = (
  games,
  map,
  setGameMarkers,
  setInfoWindow,
  handleMarkerClick
) => {
  const infoWindow = new google.maps.InfoWindow({});

  const gameMarkers = [];
  for (const game of games) {
    const newMarker = createGameMarker(
      map,
      game,
      infoWindow,
      gameMarkers,
      handleMarkerClick
    );

    newMarker ? gameMarkers.push(newMarker) : null;
  }

  // Update state
  setGameMarkers(gameMarkers);
  setInfoWindow(infoWindow);
};


// Create marker object for game
export const createGameMarker = (
  map,
  game,
  infoWindow,
  gameMarkers,
  handleMarkerClick
) => {
  const [lat, lng] = [parseFloat(game.latitude), parseFloat(game.longitude)];

  if (locationIsUnoccupied(lat, lng, gameMarkers)) {
    var icon = {
      url: GAME_ICON_URL,
      scaledSize: new google.maps.Size(30, 45),
    };

    var marker = new google.maps.Marker({
      position: {
        lat: lat,
        lng: lng,
      },
      icon: icon,
      map,
    });

    marker.addListener("click", () => {
      infoWindow.close();
      map.panTo(marker.getPosition());
      handleMarkerClick(marker, map);
      infoWindow.setContent(createGameInfoWindow(game));
      infoWindow.open(map, marker);
    });

    return marker;
  }
};

One Answer

From a short review;

  • locationIsUnoccupied and createGameInfoWindow are mysterious, where are they defined?
  • newMarker ? gameMarkers.push(newMarker) : null; should trigger jshint, I would just go for the if statement
  • I would structure all the code and functions in to MVC (model view controller)
    • So GAME_ICON_URL becomes model.GAME_ICON_URL
    • So createGameInfoWindow becomes ui.createGameInfoWindow
    • So locationIsUnoccupied becomes model.locationIsUnoccupied

This could make your code look like;

// Initialising map and markers
export function handleApiLoaded(model, ui, controller){
  ui.infoWindow = new google.maps.InfoWindow({});

  model.gameMarkers = [];
  for (const game of games) {
    addGameMarker(game, model, ui, controller);
  }

  //Not sure the below would still make sense?
  model.setGameMarkers(gameMarkers);
  ui.setInfoWindow(ui.infoWindow);
};


// Create marker object for game
export function addGameMarker(game, model, ui, controller){

  const [lat, lng] = [parseFloat(game.latitude), parseFloat(game.longitude)];

  if (model.locationIsUnoccupied(lat, lng, model.gameMarkers)) {
    var icon = {
      url: model.GAME_ICON_URL,
      //Why 30 and 45, a comment would be good here
      scaledSize: new google.maps.Size(30, 45),
    };

    var marker = new google.maps.Marker({
      position: {
        lat: lat,
        lng: lng,
      },
      icon: icon,
      map: model.map,
    });

    marker.addListener("click", () => {
      ui.infoWindow.close();
      //Maybe `map` should be part of `ui`, deep thoughts to be had..
      model.map.panTo(marker.getPosition());
      ui.handleMarkerClick(marker, map);
      ui.infoWindow.setContent(createGameInfoWindow(game));
      ui.infoWindow.open(map, marker);
    });

    model.gameMarkers.push(marker);
  }
};

Answered by konijn on October 27, 2021

Add your own answers!

Ask a Question

Get help from others!

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP