Google Maps marker management for games

Question

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;
  }
};

konijn · 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);
  }
};

Google Maps marker management for games

One Answer

Add your own answers!

Ask a Question