Super Market Pricing Model

Question

I have modelled the task of super market pricing as per the below task.
http://codekata.com/kata/kata01-supermarket-pricing/

Please review my code and suggest any improvements.

class Product
{   
    String Name; //Name of the product
    int unit_price; 
    UnitType unit_type; 
    boolean groupOffer; //Is this product eligible for a group offer?
    int buy,get; //If so..what are the values of buy and get..

    public Product(String name, int unit_price, UnitType unit_type, boolean groupOffer) {
        // TODO Auto-generated method stub
        Name = name;
        this.unit_price = unit_price;
        this.unit_type = unit_type;
        this.groupOffer = groupOffer;
    }

    public Product(String name, int unit_price, UnitType unit_type,boolean groupOffer, int buy, int get) {
        this(name,unit_price,unit_type,groupOffer);
        this.buy = buy;
        this.get = get;
    }

}

enum UnitType
{
    NUMERIC, //to represent non-breakable units.
    POUND,
    KG,
    GRAM;
}

public class SuperMarket {  
    public static void main(String args[])
    {
        //Configuring the price of products.
        Product pen = new Product("Pen",5,UnitType.NUMERIC,Boolean.FALSE);
        Product potato = new Product("Potato",2,UnitType.POUND,Boolean.FALSE);
        Product soda = new Product("Soda",1,UnitType.NUMERIC,Boolean.TRUE,4,1);
        //Calculating the prices
        System.out.println(getPrice(pen,4));
        System.out.println(getPrice(pen,4.1f)); //Pen cannot be a fraction
        System.out.println(getPrice(potato,3)); 
        System.out.println(getPrice(potato,3.1f));//Potato can be a fraction
        System.out.println(getPrice(soda,10)); // Eligible for group offer
    }

    //Get the price of products
    public static float getPrice(Product product,float quantity)
    {
        if(product.unit_type == UnitType.NUMERIC)
        {
           if(quantity != Math.round(quantity))
               return -1000;
        }
        float price = product.unit_price * quantity;
        //To decrease the price of free items.
        if(product.groupOffer)
        {
             price = (quantity/(product.buy+product.get)) * product.buy * product.unit_price; 
        }
        return price;
    }
}

Answer 1

0. String Name; – Variable names begin with lowercase by convention: String name;.

1. unit_price; unit_type; – Variable names are camel case by convention: unitPrice; unitType;

2. boolean groupOffer – It's convention to start boolean variables with is..., has..., can..., must...: boolean isEligibleForGroupOffer. Such you also don't need the comment.

3. int unit_price; int buy,get;

   • Your amounts are integers only? Really? Not float or BigDecimal:

     Currency

     It is recommended to use BigDecimal class while dealing with Currency or monetary values as it provides better handling of floating point numbers and their operations.

   • buy and get are verbs that fit better to a method name. Do you mean buyingPrice and sellingPrice?
   • More than one variable declaration per line is discouraged.

4. I'd rename UnitType to SellingUnitand NUMERIC to PIECES.

5. I'd implement the body of your first constructor with just:

    this(name, unitPrice, sellingUnit, isEligibleForGroupOffer, 0, 0);
   

6. And the one of the second with:

    this.name = name;
    this.unitPrice = unitPrice;
    this.sellingUnit = sellingUnit;
    this.isEligibleForGroupOffer= isEligibleForGroupOffer;
    this.buyingPrice = buyingPrice;
    this.sellingPrice = sellingPrice;
   

7. I'd add a space after the commas in parameter and argument lists for easier reading.

8. I'd change static ... getPrice(...) to a non-static member of Product with then a header of public BigDecimal getPriceFor(float quantity).

9. It's convention to separate name and parenthesis in statements like if (...) with a space to easily distinguish them from method invocations.

10. I'd simplify:

     float price = product.unit_price * quantity;
     //To decrease the price of free items.
     if(product.groupOffer) {
       price = (quantity/(product.buy+product.get)) * product.buy * product.unit_price; 
     }
     return price;
    

    to:

     return isEligibleForGroupOffer  // To decrease the price of free items.
              ? quantity / (buyingPrice + sellingPrice) * buyingPrice * unitPrice
              : unitPrice * quantity;
    

Answer 2

In addition to @GeroldBroser's answer, I'm not sure if the UnitType.NUMERIC enum value is a good fit... The others are really measurements for mass, and thus a unit-less value is probably better represented as a null value.

Also, your -1000 return value is an ugly hack to 'ignore' further calculations in getPrice(). You should think of a better way to handle such cases, than using a default number.

edit:

For constructor chaining, the recommended approach is for the stricter (i.e. less arguments) constructor to passthrough default values to the liberal (i.e. more arguments) one. This lets you consolidate all the settings into one method, and also lets you convert to an immutable class implementation afterwards, without the compiler complaining that final fields are not set in the stricter constructors.