code-smells.java

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

    }

A snippet used in interviews. There are at least a dozen problems in the code, and they all start with a lousy design for a method. Fewer of them are straight-forward code mistakes (e.g., the possible NPEs all over the place).

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.

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.