10
\$\begingroup\$

I want to make sure that the code is correct in terms of its design, code correctness, best practices & Junit testing.

The complete description is given below:

Functioning of the app quickly estimates the Final Cost depending on different markups. The following are the markups:

  • Flat Markup of 5% on all jobs
  • Markup of 1.2% for per Working person
  • Type of materials markup:
    • For pharmaceuticals, 7.5% markup
    • For food, 13% markup
    • For electronics,2% markup
    • For everything else, No markup

The markup calculator should accept the initial base price along with the different categories of markups and calculate a final cost for a project. Example:

Input 1: $1299.99
3 people
food
Output 1: $1591.58

The app is designed like a MVC (Model view controller) logic app where the Model is used to store the data & perform calculations, Controller acts as the getter & setter for values from user & View is used to print the final input & output to the user as MVC format helps any developer to understand the structure & can also embed this app into another MVC app for use

MainCalculator

public class MainCalculator {

    //BASE PRICE 
    private static String BASE_PRICE="$1299.99";

    //Number of People
    private static String NUM_OF_PEOPLE= "3 people";

    //Types of materials
    private static String TYPE_OF_MATERIAL[] = new String[]{"FOOD","food"};

    //Error messages are stored in this
    private static String message;

    //status remains true if validation for error checking passes the test i.e. program has handled all test cases
    private static boolean status= true;

    /* 
     * MSG_ARGUMENT_NULL, MSG_INVALID_BASE_PRICE, MSG_INVALID_NUM_OF_PEOPLE, MSG_INSUFFICIENT_ARGUMENTS are the messages
     * displayed when a particular test case fails in the below methodsS
     * 
     */

    //@param MSG_ARGUMENT_NULL is printed when validateNullArgument() returns false
    private static final String MSG_ARGUMENT_NULL = "Mandatory inputs cannot be null, <BASE_PRICE>, <NUM_OF_PEOPLE> is mandatory";
    private static final String MSG_ARGUMENT_NULL_TYPE_OF_MATERIAL = "One of the arguments is null in <Type of Material>";

    //@param MSG_INSUFFICIENT_ARGUMENTS is printed when validateEmptyArgument() returns false
    private static final String MSG_INSUFFICIENT_ARGUMENTS = "Insufficient number of values <BASE PRICE>, <NUM OF PEOPLE>, <TYPE OF MATERIAL>(Optional)";   

    //@param MSG_INVALID_BASE_PRICE and/or MSG_INVALID_NUM_OF_PEOPLE is printed when validateNumberFormat() returns false
    private static final String MSG_INVALID_VALUE = "The <BASE PRICE>/<NUM OF PEOPLE> is not in correct number format";

    //System error code 1 denotes invalid argument from user
    private static final int SYSTEM_ERROR_CODE_INVALID_INPUT = 1;

    //Main function instantiates the default constructor
    public static void main(String[] args) {

        //Default Constructor
        MarkupController markupControllerObject = new MarkupController();

        //sets base price from MarkupController's setter method
        markupControllerObject.setBasePrice(BASE_PRICE);

        //sets number of people from MarkupController's setter method
        markupControllerObject.setNumOfPeople(NUM_OF_PEOPLE);   

        /*
         * sets the types of materials from MarkupControllers setter method & is a array as multiple types
         * of materials can be involved
         */
        for(int i=0;i< TYPE_OF_MATERIAL.length;i++)
        {
            markupControllerObject.setTypeOfMaterial(TYPE_OF_MATERIAL);
        }
        //gets base price from MarkupController's getter method
        BASE_PRICE = markupControllerObject.getBasePrice();

        //gets number of people from MarkupController's getter method
        NUM_OF_PEOPLE = markupControllerObject.getNumOfPeople();    

        /*
         * Gets the types of materials from MarkupControllers getter method & is a array as multiple types
         * of materials can be involved
         */
        for(int i=0;i< markupControllerObject.getTypeOfMaterial().length;i++)
        {
            TYPE_OF_MATERIAL = markupControllerObject.getTypeOfMaterial();
        }


        //checks if the values in constructor are valid otherwise exits system
        if(!validateNullArgument(BASE_PRICE,NUM_OF_PEOPLE,TYPE_OF_MATERIAL) || !MainCalculator.validateEmptyArgument(BASE_PRICE,NUM_OF_PEOPLE) || !MainCalculator.validateNumberFormat(BASE_PRICE, NUM_OF_PEOPLE))
        {
            //System must exit if any of the conditions are true in if condition
            exit(SYSTEM_ERROR_CODE_INVALID_INPUT);
        }
        else
        {
            /*
             * Removes $ sign & whitespace from Base Price if present
             * and returns only the double value
             */
            BASE_PRICE = validateDollarCheck(BASE_PRICE);

            /*
             * Removes keyword 'people' & whitespace from Number of 
             * people if present & returns only number
             */
            NUM_OF_PEOPLE = validateNumOfPeopleKeyword(NUM_OF_PEOPLE);

            MarkupModel.calculateMarkupSystemFormula(BASE_PRICE, NUM_OF_PEOPLE, TYPE_OF_MATERIAL);
        }
    }

    //Validates if BASE PRICE OR NUMBER OF PEOPLE or TYPE OF MATERIAL is NULL
    public static boolean validateNullArgument(String BASE_PRICE,String NUM_OF_PEOPLE, String[] TYPE_OF_MATERIAL)
    {
        //sizeTypeOfMaterial is the length of TYPE_OF_MATERIAL
        int sizeTypeOfMaterial = TYPE_OF_MATERIAL.length;

        //countNullCheck counts the number of null arguments in the string array of type of material
        int countNullCheck=0;

        for(int i=0;i< sizeTypeOfMaterial;i++)
        {
            if(TYPE_OF_MATERIAL[i] == null)
            {
                countNullCheck++;
            }
        }       

        /*
         * If one of the arguments in string array "Type of Material" 
         * is null, prints an error to the user but continues execution
         */
        if(countNullCheck > 0)
        {
            message = MSG_ARGUMENT_NULL_TYPE_OF_MATERIAL;
            System.out.println(message);

        }
        //if countNullCheck is equal to size of type of material which means all elements in array are null
        if(BASE_PRICE == null || NUM_OF_PEOPLE == null || TYPE_OF_MATERIAL == null || countNullCheck == sizeTypeOfMaterial)
        {
            message = MSG_ARGUMENT_NULL;
            System.out.println(message);
            return false;
        }

        return status;
    }

    /*
     * Validates if BASE PRICE OR NUMBER OF PEOPLE IS EMPTY
     * If empty, then returns a message or returns true 
     */
    public static boolean validateEmptyArgument(String BASE_PRICE, String NUM_OF_PEOPLE)
    {
        if(BASE_PRICE.isEmpty() || NUM_OF_PEOPLE.isEmpty())
        {
            message = MSG_INSUFFICIENT_ARGUMENTS;
            System.out.println(message);
            return false;
        }
        return status;
    }

    //Validates if BASE PRICE OR NUMBER OF PEOPLE Is in valid format
    public static boolean validateNumberFormat(String BASE_PRICE, String NUM_OF_PEOPLE) 
    {
        /*
         * Removes $ sign & whitespace from Base Price if present
         * and returns only the double value
         */
        BASE_PRICE = validateDollarCheck(BASE_PRICE);

        /*
         * Removes keyword 'people' & whitespace from Number of 
         * people if present & returns only number
         */
        NUM_OF_PEOPLE = validateNumOfPeopleKeyword(NUM_OF_PEOPLE);

        /*
         * If base price is a proper double value & number of people is a proper integer 
         * else throw exception
         */
        try
        {
            Double.parseDouble(BASE_PRICE);
            Integer.parseInt(NUM_OF_PEOPLE);
        }
        catch(NumberFormatException exception)
        {
            message = MSG_INVALID_VALUE;
            System.out.println(message);
            return false;
        }

    return status;
    }
    //validates if a '$' sign is in front of Base Price & trims it
    public static String validateDollarCheck(String BASE_PRICE)
    {
        if(BASE_PRICE.length() > 0)
        {
            if(BASE_PRICE.charAt(0) == '$')
            {
                BASE_PRICE = BASE_PRICE.trim().substring(1);
            }
        }
        return BASE_PRICE;
    } 

    //Validates if user enters 'Number of people' as '8 people' & removes keyword people with whitespace
    public static String validateNumOfPeopleKeyword(String NUM_OF_PEOPLE)
    {
        //Regular expression where s* removes whitespace & \b checks for people
        String regexPeople = "\\s*\\bpeople\\b\\s*";

        //Regular expression where s* removes whitespace & \b removes 'person'
        String regexPerson = "\\s*\\bperson\\b\\s*";

        NUM_OF_PEOPLE = NUM_OF_PEOPLE.replaceAll(regexPeople, "").replaceAll(regexPerson, "");
        return NUM_OF_PEOPLE;
    }

    //Exits the system
    public static void exit(int status)
    {
        System.exit(status);
    }

}

MarkupController

public class MarkupController
{
    /**
     * MarkupController acts as a library for getter & setter methods for
     * BASE_PRICE, NUM_OF_PEOPLE and TYPE_OF_MATERIAL
     *  
     * @author Ankita Kulkarni
     */
    private String BASE_PRICE;
    private String NUM_OF_PEOPLE;
    private String[] TYPE_OF_MATERIAL;


    /*Constructor with 3 parameters
     * @param BASE_PRICE base price of the system
     * @param NUM_OF_PEOPLE number of people in the system
     * @param TYPE_OF_MATERIAL type of materials in the system, an array since user can enter multiple materials
     */
    public MarkupController()
    {

    }

    public MarkupController(String BASE_PRICE, String NUM_OF_PEOPLE,String[] TYPE_OF_MATERIAL)
    {
        this.BASE_PRICE = BASE_PRICE;
        this.NUM_OF_PEOPLE = NUM_OF_PEOPLE;
        this.TYPE_OF_MATERIAL= TYPE_OF_MATERIAL;
    }

    //gets the base Price
    public String getBasePrice()
    {
        return this.BASE_PRICE;
    }

    //sets the base Price
    public void setBasePrice(String BASE_PRICE)
    {
        this.BASE_PRICE = BASE_PRICE;
    }

    //gets the Number of people
    public String getNumOfPeople()
    {
        return NUM_OF_PEOPLE;
    }

    //sets the number of people
    public void setNumOfPeople(String NUM_OF_PEOPLE)
    {
        this.NUM_OF_PEOPLE = NUM_OF_PEOPLE;
    }

    //Gets the array of types of material   
    public String[] getTypeOfMaterial()
    {
        return TYPE_OF_MATERIAL;
    }

    //Sets the array of types of material
    public void setTypeOfMaterial(String[] TYPE_OF_MATERIAL)
    {
        this.TYPE_OF_MATERIAL=  TYPE_OF_MATERIAL;
    }
}

MarkupModel

import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Set;
import java.math.BigDecimal;
import code.MarkupView;
public class MarkupModel {

    /**
     * MarkupModel acts as a Model where it stores values of the system obtained from
     * MarkupController & performs calculations
     * @Author Ankita Kulkarni
     */

    //Flat Markup on all jobs is 5%
    private static final String FLAT_MARKUP_ALL_JOBS = "0.05";

    //For each working person, markup is 1.2%
    private static final String MARKUP_PER_WORKING_PERSON = "0.012";


    /*
     * HashMap stores <key,value> pairs where key is the type of material & value is 
     * its respective percentage markup
     * Used HashMap as its easy to lookup any existing value stored in HashMap 
     * Runtime Complexity of finding a element is O(1) hence faster
     */

    public static HashMap<String,String> markupTypeOfMaterials = new HashMap<String,String>(){
        {
            // puts <key,value> in HashMap i.e. <type of material,markup Percent>
            put("pharmaceuticals", "0.075");
            put("drugs", "0.075");
            put("food", "0.13");
            put("electronics", "0.02");
        }       
    };

    //FlatMarkup is stored in a Big Decimal object as its a decimal value
    private static BigDecimal getFlatMarkup()
    {
        return new BigDecimal(FLAT_MARKUP_ALL_JOBS);    
    }

    //Markup for every working person is stored in a Big Decimal object for calculations
    private static BigDecimal getMarkupPerWorkingPerson()
    {
        return new BigDecimal(MARKUP_PER_WORKING_PERSON);   
    }

    /*
     * @param TYPE_OF_MATERIAL provides type of material string array from user
     * This method checks if any key of hashmap matches to a key provided by user & only returns 
     * that key
     */

    public static BigDecimal getMarkupTypeOfMaterialsValues(String TYPE_OF_MATERIAL)
    {
        /*
         * containsKey() checks if there is any matching key in hashmap & TYPE_OF_MATERIAL
         * toLowerCase() all keys in Type of material is converted to lower case to match the
         * hashmap key example: FOOD and food are identical materials
         */
        if(markupTypeOfMaterials.containsKey(TYPE_OF_MATERIAL.toLowerCase()))
        {
            //Only matched key is returned
            return new BigDecimal(markupTypeOfMaterials.get(TYPE_OF_MATERIAL.toLowerCase()));
        }

        //If key doesn't match, 0 is returned as there is 'No- Markup'
        return new BigDecimal("0");
    }   

    /*
     * Calculates the main functionality of the system
     * 
     */
    public static String calculateMarkupSystemFormula(String BASE_PRICE, String NUM_OF_PEOPLE, String[] TYPE_OF_MATERIAL)
    {
        /*
         * Removes $ sign & whitespace from Base Price if present
         * and returns only the double value
         */
        BASE_PRICE = MainCalculator.validateDollarCheck(BASE_PRICE);

        /*
         * Removes keyword 'people' & whitespace from Number of 
         * people if present & returns only number
         */
        NUM_OF_PEOPLE = MainCalculator.validateNumOfPeopleKeyword(NUM_OF_PEOPLE);
        /*
         * Converts base price, number of people into BigDecimal for
         * calculations
         */
        BigDecimal basePrice = new BigDecimal(BASE_PRICE);
        BigDecimal numOfPeople = new BigDecimal(NUM_OF_PEOPLE);        

        /*
         * The elements from Type of materials are added in a Set as
         * it helps remove duplicates
         * for eg: In condition like 'food' and 'food', only 1 type
         * 'food' markup will be calculated
         */
        Set<String> typeOfMaterialSet = null;
        for(String element:TYPE_OF_MATERIAL)
        {
            //Only adds elements which are not empty or not null to the Set
            if(!element.isEmpty() && element !=null)
            {
                typeOfMaterialSet = new HashSet<String>(Arrays.asList(element));
            }
        }
        //When Type of Material does not match hashmap keys then '0' is added
        BigDecimal totalTypeOfMaterialMarkup = new BigDecimal("0");

        /*
         * Only those elements from Type of material that match
         * the hashmap keys are added for calculations
         */
        for(String typeOfMaterial:typeOfMaterialSet)
        {
            BigDecimal typeOfMaterialMarkup = getMarkupTypeOfMaterialsValues(typeOfMaterial);
            totalTypeOfMaterialMarkup = totalTypeOfMaterialMarkup.add(typeOfMaterialMarkup);
        }

        /*
         * Formula for calculating flat markups on all jobs
         * newBasePrice = basePrice * FlatMarkup (0.05)+ basePrice
         */
        BigDecimal newBasePrice = basePrice.multiply(getFlatMarkup()).add(basePrice);

        /*
         * Formula for calculating the markup for the number of people
         * Markup for working people = num of people * markup per working person (0.012)
         */
        BigDecimal totalMarkupForWorkingPeople = numOfPeople.multiply(getMarkupPerWorkingPerson());

        /*
         * Formula for Final Base Price is:
         * finalbaseprice = newBasePrice * (1+totalMarkupForWorkingPeople+totalTypeOfMaterialMarkup)
         * 
         */
        BigDecimal finalBasePrice = newBasePrice.multiply(BigDecimal.ONE.
                                    add(totalMarkupForWorkingPeople).
                                    add(totalTypeOfMaterialMarkup));

        /*
         * returns the final base price by rounding upto 2 digits after decimal along with '$' sign
         */
        MarkupView.printInput(basePrice, numOfPeople, typeOfMaterialSet);
        String outputBasePrice = MarkupView.printOutputFormat(finalBasePrice);
        System.out.println("Final Output: "+outputBasePrice);
        return outputBasePrice;
    }
}

MarkupView

import java.math.BigDecimal;
import java.util.Set;
/**
 * MarkupView is responsible for displaying the final output to the user
 * It formats the input & output to provide a clean UI to the user
 * @author Ankita Kulkarni
 *
 */
public class MarkupView {

    /*
     * This method formats the basePrice by rounding the 2 numbers after decimal using ROUND_HALF_UP
     * BigDecimal ROUND_HALF_UP is the ideal way for performing monetary calculations
     *  & ROUND_HALF_UP providing the least bias is recommended
     * @link http://www.javapractices.com/topic/TopicAction.do?Id=13
     */
    public static String printOutputFormat(BigDecimal basePrice)
    {
        /*
         * Sets scale of base price to 2 decimal round half up 
         * example, 1591.5777570 turns to 1591.58
         */
        BigDecimal finalBasePrice = basePrice.setScale(2, BigDecimal.ROUND_HALF_UP);

        //Prepends '$' sign to base price for representing money
        String dollarBasePrice = "$"+finalBasePrice.toString();
        return dollarBasePrice;
    }
    /**
     * 
     * @param basePrice gets base price that was entered by user
     * @param numOfPeople gets number of people that was entered by user
     * @param typeOfMaterial gets the type of material
     * This method prints all the parameters values on the console along with the 
     * final base price
     * 
     */
    public static void printInput(BigDecimal basePrice,BigDecimal numOfPeople, Set<String> typeOfMaterial)
    {
        System.out.println("Base Price: $"+basePrice);
        System.out.println("Number of People: "+numOfPeople);
        System.out.println("Type of Material: "+typeOfMaterial.toString().replaceAll("\\[", "").replaceAll("\\]", "") +" ");
    }

}
\$\endgroup\$
  • 3
    \$\begingroup\$ Do not try to document the standard library in comments, as IDEs already conveniently provides that documentation, and there is the Internet for the rest. Just explain what cannot be understood by reading your code directly; and keep what you write simple so that such explanation is needed rarely. \$\endgroup\$ – abuzittin gillifirca Apr 24 '14 at 7:10
14
\$\begingroup\$

I scanned through your code and also stepped through it with a debugger, line-by-line. The more I looked, the more things I found that I wanted to point out.


Let's start by looking at some unnecessary comments:

// BASE PRICE
private static String BASE_PRICE = "$1299.99";

// Number of People
private static String NUM_OF_PEOPLE = "3 people";

(Why aren't these variables a BigDecimal and an int btw?)

Make variable names self-documenting. No need to comment them that much.

Speaking of naming... private static boolean status = true;... what status? Call it programHandledAllTestCases or something similar instead. The name "status" is very ambiguous.


There is no reason to run this code in a loop:

for (int i = 0; i < TYPE_OF_MATERIAL.length; i++) {
    markupControllerObject.setTypeOfMaterial(TYPE_OF_MATERIAL);
}

I'm not sure you know what you are doing here, or why you do it, or what you intend to do. But given what the code currently actually does, this loop is not needed.


Some of your naming don't adhere to the Java naming conventions, such as private String[] TYPE_OF_MATERIAL;


markupControllerObject.setBasePrice(BASE_PRICE);
(...)
BASE_PRICE = markupControllerObject.getBasePrice();

Why set first and get just a bit later? Expecting something different than what you put there just a few lines ago?


I want to make sure that the code is correct in terms of its design, code correctness, best practices & Junit testing.

You're not using the real JUnit at all! Your "testing" consists of checking conditions, sometimes catching exceptions, and then printing your own messages. I strongly recommend using the real JUnit, it is the best practice for doing real testing.


Your private static String message; is totally useless - and would be very dangerous if this would be a multi-threaded application, it is only used in situations like this:

message = MSG_ARGUMENT_NULL_TYPE_OF_MATERIAL;
System.out.println(message);

Instead, simply do:

System.out.println(MSG_ARGUMENT_NULL_TYPE_OF_MATERIAL);

You're calling NUM_OF_PEOPLE = MainCalculator.validateNumOfPeopleKeyword(NUM_OF_PEOPLE); three times in your code. Why not only do it once?

Or rather, why do it at all? Let NUM_OF_PEOPLE be an int instead. Removing the " people" suffix from the String and converting it to a BigDecimal is just... horrible... Why BigDecimal btw? Expecting 3.49994513 people?


Your "MVC structure" seems a bit overkill for this application. It mainly seems to be used in order to separate some methods. There are however, plenty of other things to deal with first in this code. If you want them separated, keep your "MVC structure".


You seem to use static methods a lot. Although this works in this case, I think this might be a bad habit you have. I recommend avoiding that. Java is object oriented, use objects.


This method is only used once. And it's easier to call System.exit directly than to call this method.

public static void exit(int status) {
    System.exit(status);
}

Overall, I'm very sad to tell you this but there's a whole lot of clutter in this code. Unnecessary comments, unnecessary going back and forth with getting a value that you had just set, using the wrong data types for things...

I don't like writing harsh reviews like this, but I had to. There might be other things to deal with later, but now you have some work ahead of you. Please come back with a follow up to this code, I would like to write a more positive review next time!

\$\endgroup\$
  • \$\begingroup\$ Do you think I havent used getter & setter properly? \$\endgroup\$ – fscore May 1 '14 at 23:08
  • \$\begingroup\$ @fscore No, you haven't. You seem to be over-using them. There's no need to call a setter first and then just a few lines later calling the getter, just to get back the value that you had just set. There's also not any use in calling a setter for an array in a loop (for each item in the array). If it's sets an array, you don't need to call the set in a loop. \$\endgroup\$ – Simon Forsberg May 2 '14 at 12:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.