Java %= and general feedback

Question

I'm doing a program for a course in school (intro software development) and I'm looking for some general feedback as well as any ways this bit of code could be cleaner. I'm also wondering if it is bad practice to use %= as I've talked with some people and have been told it's not good to use any operators like that, other than += and -= to keep code readable, although maybe that is specific to this particular course.

private int       heightInInches;
private final int VALUE_OF_ONE_FOOT = 12;

public String getHeightInFeetAndInches() {
    String includeInches = "";
    int leftOver = (heightInInches %= VALUE_OF_ONE_FOOT);
    int newHeight = (heightInInches % VALUE_OF_ONE_FOOT);

    if(leftOver < 0) {
        includeInches = "";
    }else if(leftOver <= 1) {
        includeInches = "Inch";
    }else {
        includeInches = "Inches";
    }
    return newHeight + " feet " + leftOver + " " + includeInches;
}

This method should return a person's height in the form of (for example): 6 feet 1 inch(es) (if the inch amount exceeds 1).

Answer 1

I have no problem with +=, -= and even *=. But for /= and %=... perhaps I am just not used to them enough to like seeing them. I would recommend not using %=, but that is mostly my personal preference.

Other comments:

• Instead of using the heightInInches as a field of the class, I would recommend passing it as a parameter to the method. Your method is only dependent of this single variable so I don't really see a reason to having it dependent on your class, especially since your method actually modifies this variable. You'd be much safer in having your method as

  public String getHeightInFeetAndInches(int heightInInches) {
  

  You could even make your method static if you would like (which I personally would recommend)

• Your VALUE_OF_ONE_FOOT should be static as it is a constant value.

• It is totally impossible for your leftOver value to be less than zero unless your heightInInches is also less than zero. And so far I haven't seen a single person with a negative height. Did you mean if (leftOver <= 0)?

• I would have the last space as a part of the includeInches variable, as if it would be empty right now your return would end with a trailing space.

• Please use more spaces and make this line }else if(leftOver <= 1) { into } else if (leftOver <= 1) {

• As @palacsint has pointed out, your code has a serious bug. I believe you should use the following:

  int leftOver = heightInInches % VALUE_OF_ONE_FOOT;
  int newHeight = heightInInches / VALUE_OF_ONE_FOOT;
  

  This would also have the benefit of not modifying your heightInInches variable (although I still think you should pass it as a parameter to the method instead of using it as a field).

• As pointed out by P. Lalonde in the comments below, your VALUE_OF_ONE_FOOT should be renamed to something like INCHES_PER_FOOT. All constants are values of something.

• Feature Request: Add support for the metric system ;)

Answer 2

+1 to @Simon and some other notes:

1. If heightInInches = 14 it returns 2 feet 2 Inches. I guess it should be 1 feet 2 Inches.

2. The method currently violates the Command Query Separation principle because it modifies the heightInInches field.

   Functions should either do something or answer something, but not both. Either your function should change the state of an object, or it should return some information about that object. Doing both often leads to confusion.

   (Clean Code by Robert C. Martin, p45)

3. In the output feet starts with a lowercase letter, Inch(es) starts with uppercase letter. It could be consistent.

4. About the identation of the fields: if you have a new variable with a longer name you don't want to modify two other lines too to keep it nice. This also could cause unnecessary patch/merge conflicts.

   From Code Complete, 2nd Edition by Steve McConnell, p758:

   Do not align right sides of assignment statements

   [...]

   With the benefit of 10 years’ hindsight, I have found that, while this indentation style might look attractive, it becomes a headache to maintain the alignment of the equals signs as variable names change and code is run through tools that substitute tabs for spaces and spaces for tabs. It is also hard to maintain as lines are moved among different parts of the program that have different levels of indentation.