5
\$\begingroup\$

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

    }
}
\$\endgroup\$
  • \$\begingroup\$ Please add your PaymentDetails class. \$\endgroup\$ – mheinzerling Oct 12 '17 at 9:51
  • 1
    \$\begingroup\$ Is this actually what the question wants? I assumed from reading that it wants the net payments after multiple transactions, rather than the result of each transaction in sequence \$\endgroup\$ – Ben Aaronson Oct 12 '17 at 12:15
  • 1
    \$\begingroup\$ I rolled back changes to the code in the question, since there are already answers referring to the old version. That also meant rolling back the change to the title, and for some reason I'm not able to edit it back again. Maybe someone else can fix? \$\endgroup\$ – Ben Aaronson Oct 12 '17 at 17:13
  • \$\begingroup\$ It's extremely minor, but you have misspelt 'beneficiary' in a few instances, in particular the method checkIfPayerIsBenificiary. \$\endgroup\$ – Alex Oct 13 '17 at 8:04
  • \$\begingroup\$ To my mind, in object oriented (OO) design, payer should be its own class, and an instance of it should be able to tell you if it's a beneficiary. As a rule of thumb, if you're passing an object into a method, it should never be to get information about that object. Each object should be able to tell you everything about itself. Think of it as an autonomous character. Show it respect. Ask it nicely for things. \$\endgroup\$ – Aron Dec 4 '17 at 19:33
4
\$\begingroup\$

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)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ comments section Even your derived comment does not add any information byond the code itself. Comments should not explain what the code does but why it is like it is... \$\endgroup\$ – Timothy Truckle Oct 12 '17 at 10:10
  • 6
    \$\begingroup\$ could be a oneliner to improve readability: Shouldn't it improve readability to avoid oneliner if statements? \$\endgroup\$ – Clijsters Oct 12 '17 at 12:02
  • \$\begingroup\$ As usual: It depends. If you have a well speaking single variable name in the condition and one well speaking method name (etc.) and no else branch, I prefer to omit the braces and have the feeling that it is more readable. \$\endgroup\$ – mheinzerling Oct 12 '17 at 12:28
  • 3
    \$\begingroup\$ "could be a oneliner to improve readability" Uh, no, that does not at all improve readability, at best it is equal. The words "one liner" and "improve readability" should pretty much only be used in one statement when "does not" is used between them. Also one liners are prone to bugs where the one liners are inspired from C one liner syntax. \$\endgroup\$ – whn Oct 12 '17 at 13:34
  • 1
    \$\begingroup\$ Every code standard I have ever read, including the Java Code Conventions, requires braces for every if/for/while block. I guess you could say it’s “highly opinion based”: nearly every opinion is that omitting braces is poor practice. \$\endgroup\$ – VGR Oct 12 '17 at 17:49
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I also though about complaining about the data object, but without the remaining code that it using it, I would not reject such an approach by default. If you need multiple return values as a private inner static class, it is not that wrong and might be a valid practical solution. \$\endgroup\$ – mheinzerling Oct 12 '17 at 12:31
2
\$\begingroup\$

@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!

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ To quickly code review your code review, SpendingManager isn't a good design. Any "-er" class is a bad design. It's also needlessly mutable. I would have an immutable Trip which takes a Collection<Expense> in its constructor. \$\endgroup\$ – Michael Oct 17 '17 at 9:58

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.