1
\$\begingroup\$

I would like to know how I can make the following more readable/maintainable, reduce side effects and account for edge cases, unit test cases and things I need to be aware of with extreme values.

Example:

tradeValue = 99999999999

I am trying to estimate profit after commission from a list of Market Trades Data provided in CSV file format. There are multiple closed trades in against each of the market:

  1. First part of the string shows tradeValue
  2. Second part is associated with marketID

Trade value after commission is calculated per trade, NOT the total. If tradeValue < 1000 then there will be a 10 commission charge.

  • Test Case: valueAfterCommission = 6 - 10 = - 4;
  • Test Case: valueAfterCommission = 999 - 10;

If tradeValue >= 1000 for every 1000 inc partial changes then commission charge is 20:

  • Test Case: valueAfterCommission = 1000 - 20 = 980;
  • Test Case: valueAfterCommission = 1001 - 40 = 961;

From the provided list of market data, the top performing market with the highest totalTradeValue is excluded from calculations.

(()=>{

    function getTotalTradeValue(marketDataCSV) {
        const marketDataCollection = getMarketData(marketDataCSV.split(/\n/));
        const dataArray = Object.values(marketDataCollection);
        const marketsSortedArray = sortMarketData(dataArray);

        const totalTradeValue = totalTradeValueCalc(marketsSortedArray);
        const totalComission = getTotalCommission(marketsSortedArray);

        console.log(marketDataCollection, marketsSortedArray, totalTradeValue, totalComission);
        return tradeValueAfterCommission = totalValueAfterCommission(marketsSortedArray);

    }

    // Get the market data and then conver it to a collection
    function getMarketData(dataStr) {
        let marketMap = {};

        for (let i = 0; i < dataStr.length; i++) {

            const marketID = parseInt(dataStr[i].split(/\,/)[1]);
            const tradeValue = parseInt(dataStr[i].split(/\,/)[0]);
            const valueAfterCommission = getValueAfterCommission(tradeValue);
            const commission = getCommission(tradeValue);

            let market = !!marketMap[marketID] ? marketMap[marketID] : marketMap[marketID] = {
                trades: [],
                totalTradeValue: 0,
                totalValueAfterCommission: 0,
                getCommission: 0
            };

            market.totalTradeValue += tradeValue;
            market.marketID = marketID;
            market.totalValueAfterCommission += valueAfterCommission;
            market.commission += commission

            // Not really a requirement. But good to maintain list of trades 
            // associated with each market?        
            market.trades.push({
                tradeValue: tradeValue,
                valueAfterCommission: valueAfterCommission,
                commission: commission
            });
        }

        return marketMap;
    }

    // Get value of trade after commission
    function getValueAfterCommission(val) {
        let valAfterCommission = val;

        if (val < 1000) {
            valAfterCommission -= 10
            return valAfterCommission;

        } else {
            const multiplier = val % 1000;
            valAfterCommission -= Math.floor(val / 1000) * 20;
            valAfterCommission -= val % 1000 !== 0 ? 20 : 0;
            return valAfterCommission;
        }
    }

    function getCommission(val) {
        let commission = 0;

        if (val < 1000) {
           return commission = 10;

        } else {
            commission += Math.floor(val / 1000) * 20;
            commission += val % 1000 !== 0 ? 20 : 0;
            return commission;
        }
    }

    // Sort by totalTradeValue then marketID
    function sortMarketData(data) {
        return [].concat(data).sort((a,b)=>{ 
            return sortVal = b.totalTradeValue - a.totalTradeValue || a.marketID - b.marketID;
        });
    }

    // Get Total Comission
    function getTotalCommission(data) {
        let dataList = [].concat(data);
        dataList.shift();
        // Remove top performer
        return dataList.reduce((a,b)=>{
            return a += b.commission;
        }, 0);
    }

    // calculate Total Trade value
    function totalTradeValueCalc(data) {
        let dataList = [].concat(data);
        dataList.shift();
        // Remove top performer
        return dataList.reduce((a,b)=>{
            return a += b.totalTradeValue;
        }, 0);
    }

    // calculate Total Trade Value after commission
    function totalValueAfterCommission(data) {
        let dataList = [].concat(data);
        dataList.shift();
        // Remove top performer
        const val = dataList.reduce((a,b)=>{
            return a += b.totalValueAfterCommission;
        }
        , 0);

        return val;
    }

    var str = `30,99734
   6,400615882
   1650,99734
   1650,99762
   1650,400767052`;

    var str2 = `900,99758
   270,99716
   1800,99716`;

    var val2 = getTotalTradeValue(str2);

    var val1 = getTotalTradeValue(str);

    console.log(val1);
    console.log(val2);
})();
\$\endgroup\$
  • 1
    \$\begingroup\$ First impressions: Inconsistent naming of identifiers - partly Hungarian notaion as in xyArray, xyCollection, xyStr, partly inconsistent verb position as in getTotalX, totalYCalc, partly inconsistent abbreviations as in valAfterX, valueAfterY. Second impression: Confusing or rather absence of higher level data structures; might want to declare classes and methods operating on attributes instea of functions which operate on 'data', allows for statingand testing class invariants. \$\endgroup\$ – le_m Mar 27 '17 at 22:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.