HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Java %= and general feedback

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andfeedbackjavageneral

Problem

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

Solution

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

-
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

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

Code Snippets

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

Context

StackExchange Code Review Q#41909, answer score: 14

Revisions (0)

No revisions yet.