HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Super Market Pricing Model

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
supermarketmodelpricing

Problem

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 = p

Solution

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

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

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

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



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

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

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


-
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;


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

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

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

-
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;

Code Snippets

this(name, unitPrice, sellingUnit, isEligibleForGroupOffer, 0, 0);
this.name = name;
 this.unitPrice = unitPrice;
 this.sellingUnit = sellingUnit;
 this.isEligibleForGroupOffer= isEligibleForGroupOffer;
 this.buyingPrice = buyingPrice;
 this.sellingPrice = sellingPrice;
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;
return isEligibleForGroupOffer  // To decrease the price of free items.
          ? quantity / (buyingPrice + sellingPrice) * buyingPrice * unitPrice
          : unitPrice * quantity;

Context

StackExchange Code Review Q#109472, answer score: 6

Revisions (0)

No revisions yet.