While loop overflowing the stack

Hi Guys,

I’m building a split expense app and right now it is at the basic level where I’m just trying to get the MVP running before scaling it. I have this array of users:

users: [
      { id: 0, name: "Will", userTotalExpenses: 270, amountUserOwes: -180 },
      { id: 1, name: "Mike", userTotalExpenses: 0, amountUserOwes: -90 },
      { id: 2, name: "Eleven", userTotalExpenses: 0, amountUserOwes: -90 }
    ]

I have a function which keeps track of value of amountUserOwes key.

let isAmountUserOwesZero = () => {
  return demoState[0].users.every(user => user.amountUserOwes === 0);
};

Now I’m running a while loop which will run until value of amountUserOwes key equals zero. It goes something like this:

while(!isAmountUserOwesZero()) {}

Inside the while loop, I’m manipulating the value of amountUserOwes key for each user. Somehow I’m not able to break the while loop and it is running indefinitely. Can someone figure out what am I doing wrong here?

Here is the codesandbox link for the same: https://codesandbox.io/s/go-dutch-app-p3jit .
Please check the javaScript code from line 44 onwards where all the action is. Ignore the code above line 44.

Thanks in advance guys :slight_smile:

It looks like in your array those amounts aren’t 0 so your function returns false and in the while loop you check for !isAmountUserOwesZero() which is !false so it s always true

That’s what I’m not able to figure. The logic inside the while loop should at some point make every amountUserOwes zero but it’s not.

Some users don’t want to pay what’s owed? :slight_smile:

I have a problem understanding your logic, in your initial state no one owes anything. Then you run splitExpenses and the naming of this function suggests that you want to distribute expenses so the outcome would be users that owe something, right?

The thing is in the while loop, setting the amount to 0 doesn’t work for all of them it only sets for one of them. That logic must be revisited. try to create a for loop that iterates 3 times and put all the logic in the while loop in the for loop and throw a console at the end after the for loops is done and check the state of the app and you will see the amount of each user.

so the demoState variable is just something I created to play and work around before is starts populating dynamically. Few lines later, I fill the amountUserOwes programmatically

And yes, splitExpenses split expenses among the users.

I can try that. revisit the code once more to see why it is setting the value to 0 for all the users.

How exactly you want to get the amount 0 for each user(like, the process behind it)? I noticed that some user even has negative value.

so negative value of amountUserOwes represents that other users owe him/her some money.

My logic behind splitting expenses goes like this:

  1. Find the user who owes the maximum amount(HighestDebtAmount) and user to whom maximum amount is owed(highestLenderAmount).
    Highest positive amountUserOwes vale would represent HighestDebtAmount and lowest negative would be highestLenderAmount.
  2. Settle the HighestDebtAmount against highestLenderAmount and keep repeating until amountUserOwes becomes 0 for one of them.
  3. Recalculate the new HighestDebtAmount or highestLenderAmount whichever turned to 0 in step 2.
  4. Repeat step 2

I think I solved your issue. The problem was highestLenderAmount it was 90 at the beginning instead of -180.
Replace that function body you have with this one:

let highestLenderAmtFn = () =>
    demoState[0].users.reduce((a, b) =>{
      //return Math.min(a, b.amountUserOwes);
      return a < b.amountUserOwes ? a : b.amountUserOwes
    },0);

The idea is that I gave the accumulator(a) an initial value of 0 so it s a number. The problem was that you compared the value of 2 properties of 2 objects and it returned a number but in your code it was a.amountUserOwes so I turned the accumulator into a number and check against each value of the property of each remaining object. Now the infinite loop is gone and I checked the state at the end and all amounts are 0.

1 Like

Couple remarks from my side, as it looks like you’re quite serious about this app :slight_smile:

  1. You might want to keep users outside trips. For example, what if one user has multiple trips? So your trip would look something like:
const trip = {
  users: ['06', '03', '14', '04', '02', '34', '21'],
  expenses: [
    {
      type: 'travel',
      paidBy: '21',
      amount: 249.99
    },
    {
      type: 'leisure',
      paidBy: '02',
      amount: 20.03,
    },
    {
      type: 'hotel',
      paidBy: '03',
      amount: 149.45,
    },
    {
      type: 'car',
      paidBy: '02',
      amount: 114.13,
    }
  ]
};
  1. It’s generally healthier to keep balance sheet for the trip outside users array - it will cause less confusion and would be easier to recalculate in the case if any user pulls out of the trip during the process:
trip.totalCost = +(trip.expenses.reduce((total, { amount }) => total + amount, 0)).toFixed(2);

trip.totalCostPerUser = +(Math.floor(trip.totalCost / trip.users.length * 100) / 100).toFixed(2);

trip.balance = trip.users.map((id) => +(trip.expenses.reduce((total, { paidBy, amount }) => paidBy === id ? total + amount : total, 0) - trip.totalCostPerUser).toFixed(2));
  1. The correct data structure for the payment distribution between users would always be a graph, for example in the form of 2D matrix:
trip.paymentsGraph = [...Array(trip.users.length)].map(() => Array(trip.users.length).fill(0));
  1. Your splitExpenses() should fill out this graph, but I’m not sure about selecting max value and min value and repeating the process, like you do. At first it seems like a right thing to do, but taking into account amount of linear checks you need to make, I would just go with quadratic for loop instead:
// make a copy of balance
const balance = [...trip.balance];

for (let y = 0; y < balance.length; y++) {
  if (balance[y] >= 0) continue;

  for (let x = 0; x < balance.length; x++) {
    if (balance[y] >= 0) break;
    if (y === x || balance[x] <= 0) continue;

    const amount = +(Math.min(balance[y] * -1, balance[x])).toFixed(2);

    trip.paymentsGraph[y][x] += amount;
    balance[y] += amount;
    balance[x] -= amount;
  }
}

Hope this will help

1 Like

Thanks @Osiris: It did solved the issue and I learnt one more way of using reduce. Thanks a ton! It does make sense now to keep an initial value. :slight_smile:

@snigo I’m trying to soak in all the logic that you have shared. Thank you for the detailed explanation.

I would be adding multiple events like “Wedding gift”, “trip to new york” etc. and so wants to keep users for each event separate. Each trip/event would have it’s own balance sheet, expenses etc.

I was keeping this as a feature to be added later on once the MVP is up and running. Do you think I should incorporate it as part of MVP?

I’ll come back again with more questions once I have comprehended the code you share.