8
\$\begingroup\$

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
\$\endgroup\$
2
  • \$\begingroup\$ 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 \$\endgroup\$
    – Darius X.
    Apr 16, 2014 at 1:00
  • \$\begingroup\$ Inside the first check for room=="I", you are checking for room="N". That's an impossible condition. You probably mean nurse=="N" \$\endgroup\$
    – Darius X.
    Apr 16, 2014 at 1:05

6 Answers 6

4
\$\begingroup\$

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

\$\endgroup\$
4
\$\begingroup\$

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{
       ...
}
\$\endgroup\$
1
  • \$\begingroup\$ Much better, +1! \$\endgroup\$
    – syb0rg
    Apr 16, 2014 at 2:31
3
\$\begingroup\$

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?

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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

\$\endgroup\$
0
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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