Problem
Friends A, B, C, D go for a trip.
They spend on various expenses.
Cost of the expense is shared.
Example :
- A spends 100 for breakfast for A, B, C and D
- D spends 500 for cab for B and C
- B spends 300 for lunch for A, B and C
Write a program to calculate how much each should get or each should give to one another. App should be scalable that number of friends can change
For the above problem I have written the below code. I need help to know whether I have written quality code utilising an optimal OOP design.
public class SpendDetect
{
Map<String, PayDetails> outputMap = new HashMap<String, PayDetails>();
public void getPayDetails(String payer, int amount, List<String> beneficiary) {
List<String> consolidatedBenificiary = beneficiary;
int dividableCount = consolidatedBenificiary.size();
int amountToBePaid = 0;
/**
* Check if the payer is also a beneficiary
*/
boolean IfPayerIsBenificiary = checkIfPayerIsBenificiary(payer,
beneficiary);
/**
* If the payer is beneficiary remove the payer from the list of
* beneficiaries
*/
if (IfPayerIsBenificiary) {
consolidatedBenificiary.remove(payer);
}
/**
* Calculate the amount to be payed back by the benificiary
*/
amountToBePaid = amount / dividableCount;
/**
* List the details to be paid by the beneficiary to the payer
*/
for (String benificiaryIndividual : beneficiary) {
System.out.println(benificiaryIndividual + " should be paying "
+ amountToBePaid + " to " + payer);
}
}
/**
* Method to check if the payer is a benificiary
*
* @param payer
* @param beneficiary
* @return
*/
public boolean checkIfPayerIsBenificiary(String payer,
List<String> beneficiary) {
boolean IfPayerIsBenificiary = false;
if (beneficiary.contains(payer)) {
return true;
}
return IfPayerIsBenificiary;
}
/**
* Set whether the amount is payable
*
* @param isPayable
* @param amount
* @param payDetails
* @return
*/
public PayDetails setPayableOrToBePayed(boolean isPayable, int amount,
PayDetails payDetails) {
if (isPayable) {
payDetails.Payable = payDetails.Payable + amount;
} else {
payDetails.toBePayed = payDetails.toBePayed + amount;
}
return payDetails;
}
public static void main(String[] args) {
SpendDetect spd = new SpendDetect();
List<String> benificiaryList = new ArrayList<String>();
benificiaryList.add("A");
benificiaryList.add("B");
benificiaryList.add("C");
benificiaryList.add("D");
spd.getPayDetails("A", 100, benificiaryList);
benificiaryList.clear();
benificiaryList.add("B");
benificiaryList.add("C");
spd.getPayDetails("D", 500, benificiaryList);
benificiaryList.clear();
benificiaryList.add("A");
benificiaryList.add("B");
benificiaryList.add("C");
spd.getPayDetails("B", 300, benificiaryList);
}
}
Solution
Comments
Inline commend are usually no JavaDoc comment (/* vs /**).
Please check your comments, if they add any new information, that is not covered by the method or variables names, e.g:
/**
* If the payer is beneficiary remove the payer from the list of
* beneficiaries
*/
if (IfPayerIsBenificiary) {
consolidatedBenificiary.remove(payer);
}
could be a oneliner to improve (opinion!) readability:
if (ifPayerIsBenificiary) consolidatedBenificiary.remove(payer);
As mentioned by the other reviewers, and I can only stress this point: comments are not used to explain what you are doing but why.
Variables/Method
Variables starts lower case in java.
In java it is not that common to define/initialize variables at the beginning of the method. Just do it when you need them. (e.g amountToBePaid)
Naming a method getPayDetails and return void is confusing. Just call it print...
Generics
List<String> benificiaryList = new ArrayList<String>();
Since Java 7 you can omit the second <String> and use the diamond operator <>
Simplification
checkIfPayerIsBenificiary could be written as:
public boolean checkIfPayerIsBenificiary(String payer, List<String> beneficiary)
{
return beneficiary.contains(payer);
}
If this don’t need to be public, just inline the one usage. Your variables is telling everything IfPayerIsBenificiary in that place.
Unused code
setPayableOrToBePayed and outputMap are not used. If there is no code missing in your question, just delete it.
consolidatedBenificiary is redundant and can be inlined.
(to be continued after you completed your code)
Thanks for sharing your code!
OOP
payDetails.Payable = payDetails.Payable + amount;
This violates the most important OO principle; information hiding / encapsulation. It also incorporates the feature envy code smell.
Cluttering comments
The comments you have look like they are generated and do not contain any information beyond the code itself.
But comments should explain why your code is like it is.
@mheinzerling is probably going to mention everything already there is about your code lol. So I’ll just point out one thing in your code they probably haven’t discussed.
I discourage the use of comments to explain the what your method is doing or the variables you use. You have already have expressed the intents of some of your methods through their names. Adding a comment to explain said intent is redundant.
If you’re going to use comments, they should explain the why’s and not the what’s of your code. I’m not sure on what’s the best practice in using JavaDoc comments, but I think we’re better off without them.
Express yourself in code!
Your code is not straight to the point. There should be a section of the code that tells the reader what happened on the trip. It should look like this:
SpendingManager manager = new SpendingManager();
manager.spend("A", 100, "breakfast", "A", "B", "C", "D");
manager.spend("D", 500, "cab", "B", "C");
manager.spend("B", 300, "lunch", "A", "B", "C");
manager.printDebts();
That’s as simple as it gets. You should now write your code so that the above snippet works. To do this, you should write a test. The test should look like the above code, but instead of printing, you have to check the results. So the test might look like this:
static void testExampleFromTheTask() {
SpendingManager manager = new SpendingManager();
manager.spend("A", 100, "breakfast", "A", "B", "C", "D");
manager.spend("D", 500, "cab", "B", "C");
manager.spend("B", 300, "lunch", "A", "B", "C");
assertThat(manager.getDebt("A"), is(1234567));
assertThat(manager.getDebt("B"), is(1234567));
assertThat(manager.getDebt("C"), is(1234567));
assertThat(manager.getDebt("D"), is(1234567));
}
This test will of course fail because I just made up the number 1234567. Calculate the appropriate numbers for yourself, using pen and paper, and fill them into the test. Then write the code so that the test succeeds.
Now to your code. I just went over it quickly and left some comments, from top to bottom.
public class SpendDetect
{
By convention, every class in a Java program is named after a noun. SpendDetect is not a noun. I chose SpendingManager, which is a noun, it’s even someone active, doing things. Since code does something, the name of the class should reflect who does it and the method names should reflect what is done.
Map<String, PayDetails> outputMap = new HashMap<String, PayDetails>();
public void getPayDetails(String payer, int amount, List<String> beneficiary) {
A method whose name starts with “get” must not have a void return type. That’s against the convention.
List<String> consolidatedBenificiary = beneficiary;
Be consistent. Either write bene or beni, but don’t mix them. If you are unsure, use a dictionary.
Variables of type List or Map often have a plural name. In your case that would be beneficiaries.
int dividableCount = consolidatedBenificiary.size();
int amountToBePaid = 0;
/**
* Check if the payer is also a beneficiary
*/
boolean IfPayerIsBenificiary = checkIfPayerIsBenificiary(payer,
beneficiary);
/**
* If the payer is beneficiary remove the payer from the list of
* beneficiaries
*/
if (IfPayerIsBenificiary) {
This just doesn’t sound right. Read it aloud. If if, that’s not even close to correct grammar. Variable names should not start with if.
consolidatedBenificiary.remove(payer);
}
/**
* Calculate the amount to be payed back by the benificiary
*/
amountToBePaid = amount / dividableCount;
/**
* List the details to be paid by the beneficiary to the payer
*/
for (String benificiaryIndividual : beneficiary) {
System.out.println(benificiaryIndividual + " should be paying "
+ amountToBePaid + " to " + payer);
}
}
/**
* Method to check if the payer is a benificiary
*
* @param payer
* @param beneficiary
* @return
*/
public boolean checkIfPayerIsBenificiary(String payer,
List<String> beneficiary) {
The above comment is completely redundant.
- I already know that the comment describes a method.
- I already know from the method name that it “checks if the payer is a beneficiary”.
- I already know from the method that it takes three parameters and what their names are.
- I already know from the code that the method returns.
Delete this useless comment, it’s an insult to the readers of your code. You are wasting their time for no reason.
By the way, your code doesn’t even compile since you didn’t show us how the PayDetails class is defined. Therefore it’s off-topic for this site.