Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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).

share|improve this question
7  
You might not want to be so hasty to choose an accepted answer. Give people time to answer, and then make a selection. Selecting an answer so soon may also ward off some answers that you may have received had you not accepted an answer so early. –  syb0rg Feb 17 at 20:58
add comment

2 Answers 2

up vote 14 down vote accepted

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

share|improve this answer
    
Yes, I did mean (leftOver <= 0) Thanks for the great answer, Cheers! –  user36982 Feb 17 at 20:45
4  
I agree with all those mentioned, I would also add that VALUE_OF_ONE_FOOT is redundant, a constant is quite often a VALUE_OF something, I would try to get something more descriptive, like NB_INCH_PER_FOOT or INCHES_PER_FOOT –  P. Lalonde Feb 17 at 20:45
1  
@Mud No problem, and Welcome to Code Review! –  Simon André Forsberg Feb 17 at 20:46
1  
You forgot to mention ’^=’ which I think is definitely used more than ’/=’ –  Timtech Feb 18 at 1:01
    
You're right about the bug, of course, but I think you've put the operators on the wrong variables - newHeight is supposed to be the "feet" variable, not leftover. –  Clockwork-Muse Feb 18 at 12:07
show 1 more comment

+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.

share|improve this answer
1  
I thought it was strange doing two modulo in a row, couldn't put my finger on it. Thanks for filling in! –  Simon André Forsberg Feb 17 at 20:52
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.