patternjavaModerate
Java %= and general feedback
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
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).
%= 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
Other comments:
-
Instead of using the
You could even make your method static if you would like (which I personally would recommend)
-
Your
-
It is totally impossible for your
-
As @palacsint has pointed out, your code has a serious bug. I believe you should use the following:
This would also have the benefit of not modifying your
-
As pointed out by P. Lalonde in the comments below, your
-
Feature Request: Add support for the metric system ;)
+=, -= 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 aspublic 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.