2
\$\begingroup\$

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;
  }
};
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Adding the missing locationIsUnoccupied and createGameInfoWindow will improve the reviews you receive. Currently this question is off-topic for Code Review due to missing context. Please see our help center for more information on how to ask a good question. \$\endgroup\$ – pacmaninbw Jul 21 '20 at 13:04
  • 1
    \$\begingroup\$ @pacmaninbw: In all fairness, the methods sought after aren't burdened with an excessive number of parameters. \$\endgroup\$ – greybeard Jul 23 '20 at 5:48
1
\$\begingroup\$

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);
  }
};
\$\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.