4
\$\begingroup\$

I have two JS functions that return a total number for me based on some properties. It works fine but feels a bit 'clunky'. Would there be a cleaner way of doing this?

getTotalTrucks: function (data) {
    var noOfTrucks = 0;

    if (data.truck1 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck2 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck3 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck4 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck5 == "1") {
        noOfTrucks += 1;
    }

    return noOfTrucks ;
},

getTotalCars: function (data) {
    var noOfCars = 0;

    if (data.car1 == "2") {
        noOfCars += 1;
    }
    if (data.car2 == "2") {
        noOfCars += 1;
    }
    if (data.car3 == "2") {
        noOfCars += 1;
    }
    if (data.car4 == "2") {
        noOfCars += 1;
    }
    if (data.car5 == "2") {
        noOfCars += 1;
    }

    return noOfCars ;
}
\$\endgroup\$
  • \$\begingroup\$ Can you provide examples of the arguments that are being passed to these functions? \$\endgroup\$ – Shaggy Nov 17 '16 at 8:50
3
\$\begingroup\$

As already noticed by @Sumurai8 and @Mike Brant, your data structure is not pretty suitable for what you want to get.

But if for some reason you can't change it, it's even possible to use it as is, with a quite reduced code like this:

function getTotal(data, searchedPrefix, searchedValue) {
  var regex = new RegExp('^' + searchedPrefix + '\\d+$'), total = 0;
  for (prop in data) {
    if (regex.test(prop) && data[prop] == searchedValue) {
      total++;
    }
  }
  return total;
}

var data = {
  truck1: '1',
  truck2: 'xxx',
  truck3: '0',
  truck4: '1',
  car1: 'zzz',
  car8: 2,
  car88: 2,
  car888: 2
};

console.log('truck & "1":', getTotal(data, 'truck', '1'));
console.log('car & 2:', getTotal(data, 'car', 2));

This way, a unique function works for properties beginning with "truck", "car", or anything else, followed by digits only.
It also allows to look for a value which is any string or number.

\$\endgroup\$
  • \$\begingroup\$ I would not recommend the comma notation, but otherwise an okay answer. \$\endgroup\$ – Sumurai8 Nov 16 '16 at 4:52
  • \$\begingroup\$ @Sumurai8 Glad you approve the rest, but I don't understand what you mean about "comma notation". Is it about the two var declarations? And what might be harmful with that? \$\endgroup\$ – cFreed Nov 16 '16 at 18:18
  • \$\begingroup\$ The comma notation has two main problems. When the declaration is on two or more separate lines, it is prone to errors where you drop one or more variables into the global scope when making a typo (e.g. type a ; instead of ,). It also messes with source control. A change made to one variable can collide with a change made for an other variable. This is especially true when they are declared on the same line, but holds up when they are declared on different lines. \$\endgroup\$ – Sumurai8 Nov 17 '16 at 8:51
  • \$\begingroup\$ @Sumurai8 Thanks for the clarification. I agree with your observations, though I don't attach them as much importance as you. Actually, this is rather matter of how we feel concerned about the debugging efforts: personally I use to consider it's merely my responsibility to be tidy enough, so I don't really mind. \$\endgroup\$ – cFreed Nov 17 '16 at 12:19
4
\$\begingroup\$

Magic values

Your code compares to "1" and "2". What are these values supposed to convey? Use constants to signify what they mean.

Data structure

Your data structure is not great for the task you are trying to accomplish. Why not use a list containing car types? Why not at least split the data based on cars and trucks, so you can get away with just using Object.keys(data.cars).length or even a simple reduce?

{
    "cars" : {
         "car1": "2",
         "car2": "0"
    },
    "trucks" : {
         "truck1": "0",
         "truck2": "1"
    }
}

or

{
    "cars": [
        {
            "type": "car1",
            "status": "broken"
        },
        {
            "type": "car2",
            "status": "contains 4 wheels"
        }
    ],
    "trucks": [
        //you get the point
    ]
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Agree here that data structure is the key problem. Anytime you find yourself naming variables or keys like something*, this should immediately raise a red flag in your mind that you are employing an anti-pattern. If for example you used an array, then it would be simple to use Array.reduce() here to perform aggregation. \$\endgroup\$ – Mike Brant Nov 15 '16 at 20:06

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.