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;
}
};
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
statementGAME_ICON_URL
becomes model.GAME_ICON_URL
createGameInfoWindow
becomes ui.createGameInfoWindow
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
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP