Skip to content
This repository was archived by the owner on May 18, 2022. It is now read-only.

SplitWise#4

Open
dharani1997 wants to merge 2 commits intoworkattech:masterfrom
dharani1997:master
Open

SplitWise#4
dharani1997 wants to merge 2 commits intoworkattech:masterfrom
dharani1997:master

Conversation

@dharani1997
Copy link
Copy Markdown

No description provided.

System.out.println("inside equal");
splitWiseService.distributeMoneyIfEqual(borrowerList,lender,amount);
}
else if(Expensetype.equals("PERCENT"))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use switch case for better readability of code.

int amount=Integer.parseInt(inputarray[2]);
int noOfborrowers=Integer.parseInt(inputarray[3]);
ArrayList<String> borrowerList=new ArrayList<String>();
int i=4;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proper variable names?

else if(Expensetype.equals("PERCENT"))
{
ArrayList<Integer> percentList=new ArrayList<>();
int m=i+2;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proper variable names?

import com.workattech.sw.models.Money;
import com.workattech.sw.models.User;

public class PrintService {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better name?


public class PrintService {

Map<String,ArrayList<Money>> listOfUserWhoOwesToUser;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why global variable?

public class PrintService {

Map<String,ArrayList<Money>> listOfUserWhoOwesToUser;
public void printMoneyOwnedByParticularUser(String user,SplitWiseService splitWiseService)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is not very readable

ArrayList<User> userList=new ArrayList<User>();
CalculateExpense calculateExpense=new CalculateExpense();
public Map<String,ArrayList<Money>> listOfUserWhoOwesToUser=new HashMap<>();
public Map<String,ArrayList<Money>> getlistOfUserWhoOwesToUser()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use proper camelCase

public void distributeMoneyIfEqual(ArrayList<String> borrowers,String lender,int amount)
{

double amountToBeDistributed=calculateExpense.CalculateMoneyEqual(amount,borrowers.size()+1);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CalculateMoneyEqual?
Should be calculateMoneyEqual

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment for other methods

subtractMoneyFromBorrowers(borrowers.get(i),lender,amountToBeDistributed);
}
}
public void distributeMoneyIfExact(ArrayList<String> borrowers,String lender,ArrayList<Integer> exactShare)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of exposing multiple methods from here. You could have created a single method like addExpense and taken an expense object as param. And deciding which distribution to choose could be done here

double amount;
public Money(String moneyGivenBy,String moneyTakenBy,double amount)
{
this.personA=moneyGivenBy;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Money should not know about who paid to whom. A better name would have been expense

this.amount-=amountTakenBypersonA;
if(this.amount<0)
{
this.amount=0; //No money is owned by personA from personB
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??

@gcnit
Copy link
Copy Markdown
Collaborator

gcnit commented Oct 4, 2019

@dharani1997 sorry for the delay! Was busy at work.

Gave a few comments. Please check.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants