Vending Machine class

Question

I am looking for ways to improve the code that I wrote for this.

Consider a simple vending machine class. The machine accepts tokens and dispenses cans of refreshing beverages. Write a complete class (on the next page) as described below:

• The class has two instance data fields; one to keep track of the number of cans in the machine and one to keep track of the number of tokens (think coins) collected.
• There should be two constructors. One takes no arguments and starts with 50 cans and zero tokens. The other takes one argument, the initial number of cans in the machine.
• There should be a method to purchase a drink which adds one token to the machine and subtracts one can – assuming there are still cans in the machine to be purchased.
• There should be one method to add cans to the machine.
• There should be separate methods to return the number of cans remaining and the number of tokens collected. Write a toString( ) method to allow for easy printing of a vending machine object

public class Vendingmachine
{
 private int tokens;
 private int cans;
 public Vendingmachine()
 {
 tokens=0;
 cans=50;
 }
 public Vendingmachine( int ca)
 {
 tokens=0;
 cans=ca;
 System.out.printf(" The constructor for this %s\n", this);
 }
 public void setTokens( int coins)
 {
 tokens = coins;
 }
 public void setCans ( int ca)
 {
 cans = ca;
 }
 public void purchase( int coins)
 {
 tokens = coins;
 if (tokens >=1)
 cans = cans-tokens;
 if (cans == 0)
 addCans();
 }
 public int getTokens()
 {
 return tokens;
 }
 public int getCans()
 {
 cans = cans- tokens;
 return cans;
 }
 public void addCans()
 {
 cans =50;
 }
 public String toString()
 {
 String str = "You have purchased "+ cans + " cans and have entered the following amount of coins: " + tokens;
 return str;
 }
}

Answer 1

I've noted a few things you could consider:

• Your class should probably be VendingMachine, note the capital M.
• The value 50 is a magic number and should probably be defined as a const at the top of your class.
• The use of coins and tokens is confusing as they appear to refer to the same thing. Pick one term and use it everywhere.
• getCans() changes the value of cans. This is a side-effect, which is generally considered a no-no, especially in a getter.
• The Java idiom for decrementing is -=, so I'd change cans = cans - tokens; to cans -= tokens;

• Both constructors contain similar code. I'd change the first to use the second, thus:

  public Vendingmachine() { this(50); }

Answer 2

There's a few things I would personally change.

For starters, the spacing seems to be inconsistent. In your constructors, you have this:

public Vendingmachine()
{
 tokens=0;
 cans=50;
} 

But later on, you have this:

public void setTokens( int coins)
{
 tokens = coins;
}
public void setCans ( int ca)
{
 cans = ca;
}

It won't do anything to affect your performance, but keeping it consistent increases readability to yourself and others. I'm also curious why your setTokens and addCans functions are tabbed in twice, most are tabbed in once, and your toString isn't tabbed in at all.

This doesn't make much difference at all, but instead of using printf (I'm assuming you come from a C background), you can use println. Either are perfectly fine for what you're doing and which one you use is usually just up to preference. For me, this is almost entirely for legibility. So you can do this:

public Vendingmachine( int ca)
{
 tokens=0;
 cans=ca;
 System.out.printf(" The constructor for this %s\n", this);
}

Or this:

public Vendingmachine( int ca)
{
 tokens=0;
 cans=ca;
 System.out.println("The constructor for this " + this);
}

Although I'm not exactly sure what you're trying to put in there with this, since it just outputs your toString method on top of the text.

Next, I think there's something up with the purchase function. From reading the problem statement, it looks like it only uses one token, but your code seems to use multiple. Assuming that we know it only uses one token, all you have to do is check if there are any cans left, add some cans if not or subtract one from cans and add one to tokens. It would look something like this:

public void purchase()
{
 if(cans == 0)
 addCans();
 else
 {
 cans--;
 tokens++;
 }
}

But aside from that, the only thing I would pick out is the spacing of it.

Answer 3

About each requirement

The class has two instance data fields; one to keep track of the number of cans in the machine and one to keep track of the number of tokens (think coins) collected.

Looks good since you actually have these fields.

There should be two constructors. One takes no arguments and starts with 50 cans and zero tokens. The other takes one argument, the initial number of cans in the machine.

The requirement is fulfilled. However how it is implemented is a code smell now. It's a good practice to have only one main constructor doing the fields assignments, the other constructor just calling the main one with other arguments:

public Vendingmachine() {
 this(50);
}
public Vendingmachine(int cans) {
 this.tokens = 0;
 this.cans = cans;
}

There should be a method to purchase a drink which adds one token to the machine and subtracts one can – assuming there are still cans in the machine to be purchased.

The requirement doesn't state what's the behaviour when there are no cans, so I assumed it is nothing. If you know what the requirement is in this case, I'll update my answer accordingly.

Looking at your current purchase method, I think the requirement is not reached. First you erase the old amount of tokens with tokens=coins; then you refill the vending machine with 50 cans when it is empty, which looks really strange and induces an unexpected side-effect (see Principle of least astonishment for details). The requirement also states that you should be able to only retrieve one can at time (and not many). Here is how I would implement purchase

public void purchase() {
 if(cans > 0) {
 cans--;
 tokens++;
 }
}

There should be one method to add cans to the machine.

At the moment there is an addCans method. However since you should add cans and not set cans, you should rather take as an argument the number of cans you should add:

public void addCans(int cans) {
 this.cans += cans;
}

There should be separate methods to return the number of cans remaining and the number of tokens collected. Write a toString( ) method to allow for easy printing of a vending machine object

The getCans is really nasty here ! One expect to only return a value but before it does some unexpected computation ! You should write it in the form of a simple getter.

public int getCans() {
 return cans;
}

getTokens is fine.

The resulting string in toString is misleading. The cans is the amount of cans the vending machine currently holds and the tokens is the number of coins that have been inserted. You should also directly return the string instead of creating an empty variable

public String toString() {
 return cans + " cans left and " + tokens + " coins inside.";
}

Beyond the requirements

The two setters are superfluous and can be removed.

Also, Java class naming convention is CamelCase so you should rename Vendingmachine to VendingMachine.

Since your class is not meant to be inherited, you can mark it final.

Here is the full resulting class

public class final VendingMachine {
 private int tokens;
 private int cans;
 public VendingMachine() {
 this(50);
 }
 public VendingMachine(int cans) {
 this.tokens = 0;
 this.cans = cans;
 }
 public void purchase() {
 if (cans > 0) {
 cans--;
 tokens++;
 }
 }
 public void addCans(int cans) {
 this.cans += cans;
 }
 public String toString() {
 return cans + " cans left and " + tokens + " coins inside.";
 }
 public int getTokens() {
 return tokens;
 }
 public int getCans() {
 return cans;
 }
}

• D.B.: Your suggested purchase method is able to add more than one token and subtract more than one can, that is not what the requirements stated. The requirement specifically says one token, one can.
• Spotted: @D.B. True, I have edited my answer.
• Rob Audenaerde: Also you allow for negative cans to be added..