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.

It works just fine, but is there any other way to write this shorter?

Nurses are only available to intensive care patients (room I) and TV's and telephones are only available to non intensive care patients (room D or room P). Also, X_COST has a value of 0.

if (room.equals("I")) { // if (room.equals("I"))

    roomCost = 395 * (double)days;
    //System.out.println(roomCost);
    TVCost = X_COST;
    //System.out.println(TVCost);
    phoneCost = X_COST;
    //System.out.println(phoneCost);

    if (nurse.equals("X"))

        nurseCost = X_COST;
        //System.out.println(nurseCost);

    else if (room.equals("N"))

        nurseCost = 250 * (double)days;
        //System.out.println(nurseCost);

    else

        nurseCost = 275 * (double)days;
        //System.out.println(nurseCost);                    

} // if (room.equals("I"))



else if (room.equals("P")) { // else if (room.equals("P")) 

    roomCost = 350 * (double)days;  
    //System.out.println(roomCost);
    nurseCost = X_COST;
    //System.out.println(nurseCost);

    if (TV.equals("V")) { // if (TV.equals("V"))

        TVCost = 40 * (double)days;
        //System.out.println(TVCost);

        if (phone.equals("T"))

            phoneCost = 15 * (double)days;
            //System.out.println(phoneCost);

        else

            phoneCost = X_COST;
            //System.out.println(phoneCost);

    } // if (TV.equals("V"))

    else { // else !(TV.equals("V"))

        TVCost = X_COST;
        //System.out.println(TVCost);

        if (phone.equals("T"))

            phoneCost = 15 * (double)days;
            //System.out.println(phoneCost);

        else

            phoneCost = X_COST;
            //System.out.println(phoneCost);

    }  // else !(TV.equals("V"))

} // else if (room.equals("P")) 



else { // else

    roomCost = 310 * (double)days;
    //System.out.println(roomCost); 
    nurseCost = X_COST;
    //System.out.println(nurseCost);

    if (TV.equals("V")) { // if (TV.equals("V"))

        TVCost = 40 * (double)days;
        //System.out.println(TVCost);

        if (phone.equals("T"))

            phoneCost = 15 * (double)days;
            //System.out.println(phoneCost);

        else

            phoneCost = X_COST;
            //System.out.println(phoneCost);

    } // if (TV.equals("V"))

    else {  // else !(TV.equals("V"))

        TVCost = X_COST;
        //System.out.println(TVCost);

        if (phone.equals("T"))

            phoneCost = 15 * (double)days;
            //System.out.println(phoneCost);

        else

            phoneCost = X_COST;
            //System.out.println(phoneCost);

    }  // else !(TV.equals("V"))

} // else
share|improve this question
    
After writing code that says "if (room.equals("I"))... there is absolutely no point adding a comment that says exactly the same thing. Make your code readable, so that you rarely need comments –  Darius X. Apr 16 at 1:00
    
Inside the first check for room=="I", you are checking for room="N". That's an impossible condition. You probably mean nurse=="N" –  Darius X. Apr 16 at 1:05

6 Answers 6

Code like the following ...

   if (phone.equals("T"))

        phoneCost = 15 * (double)days;
        //System.out.println(phoneCost);

    else

        phoneCost = X_COST;

... is copy-and-pasted into more than one type of room.

Instead of having a structure like ...

if room type
    calculate nurse cost
    calculate phone cost
    calculate TV cost
else if other room type
    calculate nurse cost again
    calculate phone cost again
    calculate TV cost again

... I suspect it would be better to have a structure like ...

// Calculate TV cost.
TVCost = (room.equals("I") || !TV.equals("V")) ? X_COST : 40 * (double)days;
// Calculate Phone cost
... etc ...
// Calculate nurse cost
... etc ...

Also I don't understand this ...

else if (room.equals("N"))

    nurseCost = 250 * (double)days;
    //System.out.println(nurseCost);

... because that's in a place where you already decided that room.equals("I").

share|improve this answer

roomCost is identical in all cases, so set it only once. Also, you should only check for room.equals("I"), because that is significantly different than the other rooms. Then if else another room, then you only need to check if TV.equals("V").

These changes will really make the resultant code much smaller and easier to support going forward. I probably would do this differently, like maybe using a switch statement (with compiler set to Java 1.7), but then my comments here are for supporting the way you would like to do it.

roomCost = 395 * (double)days;

if (room.equals("I")) { // if (room.equals("I"))
    TVCost = X_COST;
    phoneCost = X_COST;

    if (nurse.equals("X"))
        nurseCost = X_COST;
    else if (room.equals("N"))
        nurseCost = 250 * (double)days;
    else
        nurseCost = 275 * (double)days;                   

} // if (room.equals("I"))
else{
       ...
}
share|improve this answer
    
Much better, +1! –  syb0rg Apr 16 at 2:31

First things first, there is room for improvement for code formatting - there's too many empty lines, if-statements without curly braces (unless this is mandated by force in your context, but absence of braces is fertile ground for bugs...) and unnecessary comments at the end of your main if-statement. The problem with the last is what happens when conditions change in the future but the comments are not? That will surely cause confusion. This is what I'm talking about:

else if (room.equals("P")) { // else if (room.equals("P")) 
    ...
} // else if (room.equals("P")) 



else { // else
    ...
} // else

I did some formatting myself and shrank your code down to 48 concise lines. From there, I can see that the calculation for the phone and TV costs are the same - so you definitely should consider creating one method for each to wrap them and call them in the relevant places. This will also make future updates easier as you only need to modify two methods (which can be as simple as a one-liner method) instead of potentially six different lines.

Why is there a need to cast the number of days as a double type?

Also, as pointed out by @ChrisW, the first if-condition has a nested if-statement that doesn't make sense - a bug from copying-and-pasting?

if (room.equals("I")) {
    ...
    if (nurse.equals("X")) {
        nurseCost = X_COST;
    } else if (room.equals("N")) { // room equals N?
        nurseCost = 250 * (double)days;
    } else {
        nurseCost = 275 * (double)days;
    }
}

edit: just saw that X_COST = 0... in that case may I suggest renaming it as NO_COST or something along those lines?

share|improve this answer

Your outer if statements for rooms cannot be changed, however you can change some of the inner ones to conditional assignments if you want:

if (phone.equals("T"))

    phoneCost = 15 * (double)days;
    //System.out.println(phoneCost);

else

    phoneCost = X_COST;

becomes:

phoneCost = phone.equals("T") ? 15 * (double)days : X_COST;

You can do this with all of assignment operations where each branch is just passing a different value into the same variable.

share|improve this answer

You could place many if-else statements in switch case. Switch case statements are a substitute for long if statements. Java supports switch case with Strings. Here you can find an example http://docs.oracle.com/javase/7/docs/technotes/guides/language/strings-switch.html

share|improve this answer

Simple if else can be shortened by using ternary operator. If you have long if else ladder, you can use switch case.

Links. Switch case Ternary Operator

share|improve this answer

Your Answer

 
discard

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.