simple java class - critique please!

estex198 · 2009-03-24 06:43:10

Here is my lab assignment homework. I've already received credit for this assignment. I'm asking for tips to improve THIS code. I'm curious as to how the program could run more efficiently, resource-friendly, or look simpler for someone else performing modifications. How would a professional programmer write a class that functions the same as this one? What do you think about the variables I'm using and their scope? Would you use more/fewer variables? More/fewer methods? I'm not very good at documenting my code. Any tips for providing better documentation? Any other tips or pointers are welcome. Thanks a lot for your time.

- Rusty

// Lab 6 Occupant class
// Rusty Estes
// 03-23-09
// CS 160 MWF 9am
import java.text.DecimalFormat;

public class Occupant
{
    DecimalFormat df = new DecimalFormat("$#,###.00");

    private int adults,
            children,
            numberNights;

    String discount;

    private double baseCharge, return_value;
    private boolean AAA_mem, AARP_mem;

    final double MOTEL_TAX_RATE = .10,
             REG_ADULT_RATE = 25.0,
             REG_CHILD_RATE = 8.0,
             DISCNT_ADULT_RATE = 20.0,
             DISCNT_CHILD_RATE = 7.50,
             DISCNT_MULT_NIGHT = .85;

// * All constructor variables start with the prefix "cstr_" *
public Occupant(int cstr_adults, int cstr_children, int cstr_numberNights, boolean cstr_AAA_mem, boolean cstr_AARP_mem)
{
    this.adults = cstr_adults;
    this.children = cstr_children;
    this.numberNights = cstr_numberNights;
    this.AAA_mem = cstr_AAA_mem;
    this.AARP_mem = cstr_AARP_mem;
} // end constructor method


public int getAdults()
{
      return this.adults;
} // end method getAdults

public int getChildren()
{
      return this.children;
} // end method getChildren

public int getNumberNights()
{
      return this.numberNights;
} // end method getNumberNights

public double getAdultCharge()
{
      if (this.adults <= 2)
      {
      return (this.REG_ADULT_RATE * this.numberNights * this.adults);
      }
      else
      {
      return (this.DISCNT_ADULT_RATE * this.numberNights * this.adults);
      }
} // end method getAdultCharge()

public double getChildCharge()
{
      if (this.children <= 2)
      {
      return (this.REG_CHILD_RATE * this.numberNights * this.children);
      }
      else
      {
      return (this.DISCNT_ADULT_RATE * this.numberNights * this.children);
      }
} // end method getChildCharge

public double getBaseCharge()
{
    this.baseCharge = (this.getChildCharge() + this.getAdultCharge());
    
    if (numberNights > 3)
    {
    baseCharge *= DISCNT_MULT_NIGHT;
    }

    return baseCharge;
} // end method getBaseCharge

public double getMotelTax()
{
      return (this.getBaseCharge() * this.MOTEL_TAX_RATE);
} // end method getMotelTax

public double getTotalCharge()
{
      return_value = this.getBaseCharge() + this.getMotelTax();

      if (this.AAA_mem == true && this.AARP_mem == true)
      {
      return_value -= (return_value * .08);
      }
      else if (this.AAA_mem == true)
      {
      return_value -= (return_value * .05);
      }
      else if (this.AARP_mem == true)
      {
      return_value -= (return_value * .05);
      }

      return return_value;
} // end method getTotalCharge


private String getDiscount()
{
     
     return_value = this.getBaseCharge() + this.getMotelTax();

     if (this.AAA_mem == true && this.AARP_mem == true)
      {
      discount = "AAA & AARP -" + df.format(return_value * .08);
      }
      else if (this.AAA_mem == true)
      {
      discount = "AAA -" + df.format(return_value * .05);
      }
      else if (this.AARP_mem == true)
      {
      discount = "AARP -" + df.format(return_value * .05);
      }
      else
      {
      discount = "(No discount)";
      }

      return discount;
}




public String toString()
{


      return 
         "----------------------------------------------------------\n\n" +
         "Number of adults: " + this.adults + "\n" + 
         "Number of Children: " + this.children + "\n" +
         "Nights booked: " + this.numberNights + "\n" +
         "Base Charge: " + df.format(this.getBaseCharge()) + "\n" +
         "Tax: " + df.format(this.getMotelTax()) + "\n" +
         "Discount: " + getDiscount() + "\n" +
         "\n" +
         "Total Charges: " + df.format(this.getTotalCharge()) + "\n";

} // end Occupant toString method


} // end Occupant class

Last edited by estex198 (2009-03-24 22:08:56)

fumbles · 2009-03-24 06:58:51

.

Last edited by fumbles (2020-09-26 11:50:48)

Dusty · 2009-03-24 13:02:49

You're not following the Java style guidelines for braces location either. That's not really a bad thing, so long as you consciously choose not to use them. In general, I would get used to the style guidelines since standards help coders read each other's code more effectively.

In practice, I would overload the constructor so that some of the 'optional' elements like AAAMember could get default values (in that case, maybe False). Just so the calling class has less state to keep track of when it doesn't care. In an assignment, I probably wouldn't bother.

One thing you don't want to change is putting braces around one liners, good for you.
Code like this is good:
if (this.adults <= 2)
{
return (this.REG_ADULT_RATE * this.numberNights * this.adults);
}
else
{
return (this.DISCNT_ADULT_RATE * this.numberNights * this.adults);
}

Code like this works just as well, but is bad:
if (this.adults <= 2)
return (this.REG_ADULT_RATE * this.numberNights * this.adults);
else
return (this.DISCNT_ADULT_RATE * this.numberNights * this.adults);

Why? Because sooner or later you will want to put another statement under the else but will forget to wrap it in braces. This can cause confusing and subtle bugs. So keep up that good habit.

Dusty

kakTuZ · 2009-03-24 14:41:05

First thing: You can't write Comments? Practice! What does that class do? Without that proper documentation it is hard to improve the design.
Ok, you wrote that for your class where you had a task which described the behavior of the class, but we need that information

It is not necessary to prefix the constructor parameters.

public Occupant(int adults, int children, int numberNights, boolean AAA_mem, boolean AARP_mem)
{
    this.adults       = adults;
    this.children     = children;
    this.numberNights = numberNights;
    this.AAA_mem      = AAA_mem;
    this.AARP_mem     = AARP_mem;
}

The discount variable is only used as a local variable. Put variables always in the smallest possible scope. The same applies to 'baseCharge' and 'return_value'.
You have defined some constants, that is goog. But there are still magic numbers in getTotalCharge() and getDiscount().
You have declared the 'df' variable and the double constants as package private. If that was unintentionally, make them private, so they are not part of the interface of you class.

The class has one big desing plus: It is immutable (after you moved the unnecessary class variables stated above to local variables). The state of a immutable class does not change. There are only
private variables and getters! Every User of you class can rely on the immutability (which is the strongest form of thread-safety) when using it. You can bring out this property by making
adults, children, numberNights, AAA_mem, and AARP_mem final and stating that fact in the documentation.

estex198 · 2009-03-24 22:10:40

A few questions...

1. Is it always best to use smallest possible scope? What about in methods that are repeatedly called (such as getBaseCharge())? Would it be better to share the 1 memory location by declaring a class level variable rather than declaring a variable, and allocating memory for that variable each and every time the method is called? from what I understand when a method stops all variables declared within the method are destroyed, along with their memory locations? Is this just a Java thing? What about in other OO languages such as python or c/c++?

2.

You have defined some constants, that is goog. But there are still magic numbers in getTotalCharge() and getDiscount().

magic numbers? Sorry, but I'm not sure what you mean by this.

3.

You have declared the 'df' variable and the double constants as package private. If that was unintentionally, make them private, so they are not part of the interface of you class.

... you mean declare them explicitly as private?

4.

The class has one big desing plus: It is immutable (after you moved the unnecessary class variables stated above to local variables). The state of a immutable class does not change. There are only
private variables and getters! Every User of you class can rely on the immutability (which is the strongest form of thread-safety) when using it. You can bring out this property by making
adults, children, numberNights, AAA_mem, and AARP_mem final and stating that fact in the documentation.

So essentially your saying that class immutability is good wherever possible. But how can I make variables final when they are initialized within the constructor method? Is it a good idea to declare them within an constructor? Am I missing something here?

Thanks again!

- Rusty

dsr · 2009-03-24 23:12:51

estex198 wrote:

You have defined some constants, that is goog. But there are still magic numbers in getTotalCharge() and getDiscount().
magic numbers? Sorry, but I'm not sure what you mean by this.

He means 0.05 and 0.08 should be constants (declared static final) so that the caller knows what they stand for.

You have declared the 'df' variable and the double constants as package private. If that was unintentionally, make them private, so they are not part of the interface of you class.
... you mean declare them explicitly as private?

Yes. By default, they're accessible to any class in the same package.

Note that as Dusty said, you're not using Sun Microsystems' standard brace style for Java. That's fine if you prefer having braces on their own lines, but note that the official style (and therefore what a lot of people use) is like this:

public class Foo extends Bar {
    private int meta;
    private int syntactic;

    public Foo(int n) {
        if (n % 2 == 0) {
            meta = 0;
        } else {
            meta = 1;
        }
        syntactic = 42;
    }
}

Java's code conventions are here.

kakTuZ · 2009-03-25 12:03:24

estex198 wrote:

1. Is it always best to use smallest possible scope? What about in methods that are repeatedly called (such as getBaseCharge())?

It IS always best to use smallest possible scope. Failing to do so makes the code hard to read, forfeit thread safety and you are likely to make more mistakes.
You care about performance? Don't do it. These functions are neither time critical nor often called (Imagine if you include this in a productivity system. How often will the functions be called in a real motel or something? 100 Times a Day?).

estex198 wrote:

Would it be better to share the 1 memory location by declaring a class level variable rather than declaring a variable, and allocating memory for that variable each and every time the method is called? from what I understand when a method stops all variables declared within the method are destroyed, along with their memory locations? Is this just a Java thing? What about in other OO languages such as python or c/c++?

What you describe is the way it is done in C/C++. It would be a bit long to write how it works. But see http://en.wikipedia.org/wiki/Java_(prog … management for more information.

estex198 wrote:

4.
The class has one big desing plus: It is immutable (after you moved the unnecessary class variables stated above to local variables). The state of a immutable class does not change. There are only
private variables and getters! Every User of you class can rely on the immutability (which is the strongest form of thread-safety) when using it. You can bring out this property by making
adults, children, numberNights, AAA_mem, and AARP_mem final and stating that fact in the documentation.
So essentially your saying that class immutability is good wherever possible. But how can I make variables final when they are initialized within the constructor method? Is it a good idea to declare them within an constructor? Am I missing something here?

A final field can only be assigned once. And it is perfectly legal to do this in the constructor. Java will warn about final fields that have no assignments in the constructor (if they are not already initialized in their declaration line).

I can recommend the Book Effective Java as an advanced guide. Perhaps you can find it in the library of you University.

Arch Linux

#1 2009-03-24 06:43:10

simple java class - critique please!

#2 2009-03-24 06:58:51

Re: simple java class - critique please!

#3 2009-03-24 13:02:49

Re: simple java class - critique please!

#4 2009-03-24 14:41:05

Re: simple java class - critique please!

#5 2009-03-24 22:10:40

Re: simple java class - critique please!

#6 2009-03-24 23:12:51

Re: simple java class - critique please!

#7 2009-03-25 12:03:24

Re: simple java class - critique please!

Board footer