Markup calculator application using MVC

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 input

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!

StackExchange Code Review Q#47623, answer score: 14