1
\$\begingroup\$

I have started this project basically for learning perspective and wanted to learn good object oriented design. What I am trying to do is making clone of something like this but this is a command line version.

There are lots of bits and pieces of this application that I would like to be reviewed in detail.

  1. Separating I/O logic from the business logic.
  2. Deciding the roles and responsibilities of each object involved (Separation of Concern?).
  3. Loose coupling
  4. Open for maintainability and flexibility.

And would love to hear any other reviews too.

Disclaimer:

The code is nearly 500 lines long but I needed to give enough examples to be understood by others.

Code:

'use strict';

const GROUPS   = new Set();
const ACCOUNTS = new Set();

class GroupRepository {
  add(group) {
    GROUPS.add(group);
  }

  remove(group) {
    return GROUPS.delete(group);
  }

  filter(id) {
    return [...GROUPS].filter(g => g.name === id);
  }

  all() {
    return [...GROUPS];
  }
}


class AccountRepository {
  add(account) {
    ACCOUNTS.add(account);
  }

  filter(id) {
    return [...ACCOUNTS].filter(acc => acc.name === id);
  }

  remove(account) {
    return ACCOUNTS.delete(account);
  }

  all() {
    return [...ACCOUNTS];
  }
}


class Group {
  constructor(name) {
    this.name = name;
    this.accounts = new Set();
  }

  getName() {
    return this.name;
  }

  add(account) {
    this.accounts.add(account);
  }

  delete(account) {
    return this.accounts.delete(account);
  }

  getAccounts() {
    return [...this.accounts];
  }

  toString() {
    return `Group: ${this.name}`;
  }
}


class Transaction {
    constructor(account, amount, linkId, description='') {
      this.timestamp = new Date();
      this.amount  = amount;
      this.account = account;
      this.linkId  = linkId;
      this.description = description;
    }

    toString() {
      let dd = this.timestamp.getDate();
      let mm = this.timestamp.getMonth();
      let yy = this.timestamp.getFullYear();
      if (this.amount < 0) {
        return `${dd}/${mm}/${yy} ${this.description}- You get back ${Math.abs(this.amount)}`;
      } else {
        return `${dd}/${mm}/${yy} ${this.description}- You pay back ${Math.abs(this.amount)}`; 
      }
    }
}


class Account {
    constructor(name, balance) {
      this.name    = name;
      this.balance = balance;
    }

    debit(amount) {
      this.balance -= amount;
    }

    credit(amount) {
      this.balance += amount;
    }

    toString() {
      return `${this.name.charAt(0).toUpperCase() + this.name.slice(1)}`;
    }
}

class AccountManager {

  constructor(groupRepository, accountRepository) {
    this.groups   = groupRepository;
    this.accounts = accountRepository;
    this.transactions = [];
  }

  register(account) {
    this.accounts.add(account);
  }

  registerAll(iterable) {
    for (let account of iterable) {
      this.register(account);
    }
  }

  get(name) {
    return this.accounts.filter(name);
  }

  addGroup(group) {
    this.groups.add(group);
  }

  getGroup(name) {
    return this.groups.filter(name);
  }

  transfer(amount, from, to) {
    const accounts = this.accounts.all();
    if (accounts.indexOf(from) === -1 || accounts.indexOf(to) === -1) {
      throw new Error(`Invalid account entry ${from} ${to}`);
    }
    if (from === to) return;

    from.debit(amount);
    to.credit(amount);
    this.transactions
        .push(new Transaction(from, -amount, to));
    this.transactions
        .push(new Transaction(to, +amount, from));
  }

  history(user) {
    console.log('Transaction history for ' + user);
    return this.transactions.filter((t) => {
      return t.account === user;
    }).map(t => t.toString());
  }

  balance() {
    const accounts = this.accounts.all();
    function recur(accounts) {
      let maxCredit = findMax(accounts);
      let minCredit = findMin(accounts);

      if (maxCredit.balance === 0 && minCredit.balance === 0) {
        return;
      }

      let maxOfTwo = minCredit.balance > maxCredit.balance ? minCredit : maxCredit;
      console.log(`${maxCredit} owes ${minCredit} ${maxOfTwo.balance.toFixed(2)}`);

      minCredit.credit(maxOfTwo.balance);
      maxCredit.debit(maxOfTwo.balance);
      recur(accounts);
    }
    recur(accounts);    
  }
}

function findMax(accounts) {
  let max = accounts[0];
  for (let account of accounts) {
    if (account.balance >= max.balance) {
      max = account;
    }
  }
  return max;
}

function findMin(accounts) {
  let min = accounts[0];
  for (let account of accounts) {
    if (account.balance <= min.balance) {
      min = account;
    }
  }
  return min;
}


const accountRepository = new AccountRepository();
const groupRepository   = new GroupRepository();

function example1(input) {
  const sender  = input.split(':')[0]; 
  const message = input.split(':')[1];
  const amount = message.split('|')[0];

  const RE = /([A-Z]{2,13})(, \1)*( "(.*)")?/g;
  console.log(message.match(RE)[3]);

  console.log(`
  LQ: 40.00|LQ,FP,MD,GR "Dinner out"

  FP owes LQ 10.00
  MD owes LQ 10.00
  GR owes LQ 10.00
  `);


  let AM = new AccountManager(groupRepository, accountRepository);
  let LQ = new Account('LQ', 0);
  let FP = new Account('FP', 0);
  let MD = new Account('MD', 0);
  let GR = new Account('GR', 0);
  AM.registerAll([LQ, FP, MD, GR]);

  let res = getCreditTransferInfo(amount, parseCreditInfo(AM, message.split('|')[1]));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  AM.balance();
  console.log(AM.history(LQ));
  console.log(AM.history(FP));
}
example1('LQ: 40.00|LQ,FP,MD,GR "Dinner out"');


function example2() {
  let sender  = 'LQ'; 
  let message = 'LF';
  let amount  = 10;

  console.log(`
    LQ: 10|LF
    LF: 10|GR

    GR owes LQ 10.00
  `);

  const AM = new AccountManager(groupRepository, accountRepository);
  let LQ = new Account('LQ', 0);
  let LF = new Account('LF', 0);
  let GR = new Account('GR', 0);
  AM.registerAll([LQ, LF, GR]);

  let res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  sender  = 'LF'; 
  message = 'GR';
  amount  = 10;
  res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }
  AM.balance();
}
example2();

function example3() {
  let sender  = 'SC'; 
  let message = 'EM,SC,RC';
  let amount  = 16.5;

  console.log(`
    SC: 16.5|EM,SC,RC
    RC: 8.00|EM,GP

    O/P
    EM owes SC 9.50
    GP owes SC 1.50
    GP owes RC 2.50
  `);

  const AM = new AccountManager(groupRepository, accountRepository);
  let SC = new Account('SC', 0);
  let RC = new Account('RC', 0);
  let EM = new Account('EM', 0);
  let GP = new Account('GP', 0);
  AM.registerAll([SC, RC, EM, GP]);

  let res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  sender  = 'RC'; 
  message = '8|EM,GP';
  amount  = 8;
  res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  AM.balance();
}
example3();

function example4() {
  let sender  = 'LQ'; 
  let message = 'LQ,FP,MD,GR';
  let amount  = 40.00;

  console.log(`
    LQ: 40.00|LQ,FP,MD,GR "Dinner out"
    GR: 15.00|GR,LQ,MD "Uber"

    MD owes LQ 15.00
    FP owes LQ 10.00
  `);

  const AM = new AccountManager(groupRepository, accountRepository);
  let LQ = new Account('LQ', 0);
  let FP = new Account('FP', 0);
  let MD = new Account('MD', 0);
  let GR = new Account('GR', 0);
  AM.registerAll([LQ, FP, MD, GR]);

  let res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  sender  = 'GR'; 
  message = 'GR,LQ,MD';
  amount  = 15.00;

  res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  AM.balance();
}
example4();


function example5() {
  let sender  = 'LQ'; 
  let message = 'MM+2*3,LQ*2,FP';
  let amount  = 62.00;

  console.log(`
    LQ: 62|MM+2*3,LQ*2,FP
  `);

  const AM = new AccountManager(groupRepository, accountRepository);
  let MM = new Account('MM', 0);
  let LG = new Account('LG', 0);
  let PB = new Account('PB', 0);
  AM.registerAll([MM, LG, PB]);

  let res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));

  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  AM.balance();
}
example5();

function groupExample() {
  let sender  = 'LQ'; 
  let message = 'SUSHILOVERS+6,MD*2';
  let amount  = 132.00;

  console.log(`
    LQ: 132|SUSHILOVERS+6,MD*2

    FP owes LQ 28.80
    MB owes LQ 28.80
    MD owes LQ 45.60
  `);

  let AM = new AccountManager(groupRepository, accountRepository);
  let LQ = new Account('LQ', 0);
  let FP = new Account('FP', 0);
  let MB = new Account('MB', 0);
  let MD = new Account('MD', 0);
  AM.registerAll([LQ, FP, MB, MD]);

  const G = new Group('SUSHILOVERS');
  G.add(LQ);
  G.add(FP);
  G.add(MB);
  AM.addGroup(G);

  let res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  AM.balance();
}
groupExample();

function duplicateGroup() {
  let sender  = 'LQ'; 
  let message = 'SUSHILOVERS,MOVIEBUFFS';
  let amount  = 210.00;

  console.log(`
    LQ: 210|SUSHILOVERS,MOVIEBUFFS

    !!Should Fail!!
  `);


  let AM = new AccountManager(groupRepository, accountRepository);
  let AC = new Account('AC', 0);
  let LQ = new Account('LQ', 0);
  let FP = new Account('FP', 0);
  let MB = new Account('MB', 0);
  let MD = new Account('MD', 0);
  AM.registerAll([AC, LQ, FP, MB, MD]);

  const G = new Group('SUSHILOVERS');
  G.add(LQ);
  G.add(FP);
  G.add(MB);
  AM.addGroup(G);

  const G2 = new Group('MOVIEBUFFS');
  G2.add(AC);
  G2.add(MB);
  AM.addGroup(G2);

  let res = getCreditTransferInfo(amount, parseCreditInfo(AM, message));
  for (let a of res) {
    AM.transfer(a.credit, AM.get(sender)[0], AM.get(a.account)[0]);
  }

  AM.balance();
}
duplicateGroup();


function getCreditTransferInfo(amount, creditInfo) {
  const mul = [];
  const add = [];
  const res = [];
  const accounts = [];

  for (let c of creditInfo) {
    accounts.push(c.substring(0, 2));
    if (c.indexOf('*') !== -1) {
      mul.push(parseFloat(c[c.indexOf('*')+1]));
    } else {
      mul.push(1);
    }
    if (c.indexOf('+') !== -1) {
      add.push(parseFloat(c[c.indexOf('+')+1]));
    } else {
      add.push(0);
    }
  }
  let each  = (amount - add.reduce((x, sum) => x + sum, 0)) / mul.reduce((x, sum) => x + sum, 0);

  for (let i = 0; i < accounts.length; i++) {
    let amount = each * mul[i] + add[i];
    res.push({
      account: accounts[i],
      credit: each * mul[i] + add[i]
    });
  }

  //console.log("Transfer Info"); 
  //console.log(res); 
  return res;
}

function getGroupAccounts(AM, name) {
  let group = AM.getGroup(name)[0];
  return group.getAccounts().map(a => a.name);
}

function parseCreditInfo(AM, message) {
  const RE = /([A-Z]{2, 12})/g;
  const duplicates = new Set();
  const accounts   = [];

  let name  = '';
  let count = 0;
  let expr  = '';
  for (let i = 0; i < message.length; i++) {
    let char = message.charAt(i);
    if (char >= 'A' && char <= 'Z') {
      name += char;
      count = name.length;
    }
    else if (char === ',') {
      if (count === 2) {
        accounts.push(name + expr);
        duplicates.add(name);
      }
      else if (count > 2) {
        for (let account of getGroupAccounts(AM, name)) {
          duplicates.add(account);
          accounts.push(account+expr);
        }
      }

      name = '';
      expr = '';
      count = 0;
    }
    else if (char === '*' || char === '+' || (char >= '0' && char <= '9')) {
      expr += char;
    }
  }
  //console.log('Parse Result');
  // if last name is group
  let group = AM.getGroup(name);
  if (group.length) {
    for (let account of getGroupAccounts(AM, name)) {
      duplicates.add(name);
      accounts.push(account+expr);
    }
  }
  else {
    duplicates.add(name);
    accounts.push(name+expr);
  }
  // Throw if duplicate entry
  if (duplicates.size < accounts.length) {
    throw new Error('Duplicate entry');
  }
  //console.log(accounts);
  return accounts;
}
\$\endgroup\$
1
  • \$\begingroup\$ How do you model a situation, when a user is part of multiple groups? Do you create a separate account for each group, he's part of? \$\endgroup\$
    – peterpepo
    Jul 9, 2018 at 14:56

1 Answer 1

1
\$\begingroup\$

From staring at the code, this is my feedback:

  • GroupRepository, AccountRepository are so similar, they should share code
  • delete is a reserved syntax word, you should not use it as a function name
  • I like class Group, very clean
  • Transaction does not support multi-currency, something to consider
  • I see trouble in the horizon if you allow zero amount transactions, I would kick those back
  • GroupRepository.filter(id) should really be GroupRepository.filter(name) since the caller does provide a name
  • Not sure a silent return without UI message is the way to go here: if (from === to) return;
  • I would declare accountRepository and groupRepository all the way at the top
  • Not sure what example11 does in production code, are you making sure we are paying attention ;) Please put your test(ing) code in a different file, this will bite you otherwise at some point
  • Your code is very clean, I checked on jshint.com. You only have 2 unused variables (amount and RE)
  • I am not sure what balance really does, needs more comments

  • There are no obvious UI functions, so it is hard to look at separating I/O, except to say perhaps good job ;)

  • Separation of Concern looks okay, I probably would have had a Transaction class
  • This looks pretty good for maintainability/flexibility so far
\$\endgroup\$
4
  • \$\begingroup\$ balance function shows the minimum number of transactions needed for settlement. \$\endgroup\$
    – CodeYogi
    Dec 13, 2016 at 13:29
  • \$\begingroup\$ The code I presented above is for review purpose obviously I would separate codes in different file while writing in production. \$\endgroup\$
    – CodeYogi
    Dec 13, 2016 at 13:30
  • \$\begingroup\$ Actually I see that there are many stand alone functions so, the question is have I separated the responsibilities of each object correctly? \$\endgroup\$
    – CodeYogi
    Dec 13, 2016 at 13:32
  • \$\begingroup\$ I only see helper functions that are standalone, you could assign them to objects, but frankly for me it keeps the design cleaner. \$\endgroup\$
    – konijn
    Dec 13, 2016 at 14:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.