Skip to main content
Code Review

Return to Answer

added 46 characters in body
Source Link
Spotted
  • 599
  • 2
  • 9

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 induceinduces 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(int coins) {
 if(cans >=> coins0) {
 cans-= coins;-;
 tokens += coins;tokens++;
 }
}
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(int coins) {
 if (cans >=> coins0) {
 cans-= coins;-;
 tokens += coins;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;
 }
}

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 induce an unexpected side-effect (see Principle of least astonishment for details). Here is how I would implement purchase

public void purchase(int coins) {
 if(cans >= coins) {
 cans-= coins;
 tokens += coins;
 }
}
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(int coins) {
 if (cans >= coins) {
 cans-= coins;
 tokens += coins;
 }
 }
 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;
 }
}

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++;
 }
}
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;
 }
}
deleted 14 characters in body
Source Link
Spotted
  • 599
  • 2
  • 9

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.

All the rest isThe two setters are superfluous and can be removed (the two setters).

The requirement doesn't state what's the behaviour when there are no cans, so I assumed it is nothing.

All the rest is superfluous and can be removed (the two setters).

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.

The two setters are superfluous and can be removed.

Source Link
Spotted
  • 599
  • 2
  • 9

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.

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 induce an unexpected side-effect (see Principle of least astonishment for details). Here is how I would implement purchase

public void purchase(int coins) {
 if(cans >= coins) {
 cans -= coins;
 tokens += coins;
 }
}

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

All the rest is superfluous and can be removed (the two setters).

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(int coins) {
 if (cans >= coins) {
 cans -= coins;
 tokens += coins;
 }
 }
 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;
 }
}
lang-java

AltStyle によって変換されたページ (->オリジナル) /