Created
August 2, 2017 00:39
-
-
Save ErikGranse/8e0a0f8f76822c86c44c6794678d3b0f to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
// Analyze the following code snippet for "code smells" | |
// Assume the method below is in a class, and it already compiles successfully | |
// --- | |
/** | |
* Calculates the weight of a person for a given unit on a given planet | |
* | |
*/ | |
public float calculateWeight(int personId, String unit, int planetId) { | |
Person person = personDao.getPerson(personId); | |
Planet planet = planetDao.getPlanet(planetId); | |
float gravitationalConstant = planet.getGravitationalConstant(); | |
if (unit.equals("POUNDS")) { | |
gravitationalConstant = gravitationalConstant * 2.205f; //convert kg to pounds | |
} else if (!unit.equals("KILOGRAMS")) { //an invalid unit was provided | |
log.error("Invalid unit provided"); | |
throw new IllegalArgumentException("Unknown unit provided."); | |
} | |
float weight = person.getMass() * gravitationalConstant; | |
return weight; | |
} |
Yeah, I agree. My description of intent from years ago isn't great.
What I was trying to say is that having a candidate call out implementation details like the potential NPEs is OK, but I really want to see people comment on the terrible fundamental design of this method.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I would argue for a "public" method, yes, it's important to document and/or handle null inputs in the method args. For "private" methods, since the class controls all inputs, it depends on if the calling method validated the args were valid or delegated to the private method.
There's such a thing as being "too" defensive and seeing potential NPE in every line of code.