3
\$\begingroup\$

This actually belongs to a larger interview question. But here it is just the "tally up" part:

var i, a, tally = {}, arr = [1, 3, 7, 9, 3, 1, 6, 3, 3, 2, 11, 2];

for (i = 0; i < arr.length; i++) {
    a = arr[i];
    tally[a] = (tally[a] || 0) + 1;
}
console.log(tally);

I had some feeling the interviewers were not impressed or pleased (or maybe they didn't understand it), when I said I would use the line tally[a] = (tally[a] || 0) + 1; instead of using if... else or the ternary operator to do it.

\$\endgroup\$
2
  • \$\begingroup\$ Can you post the actual question? It may help in finding out why the interviewers were against your solution. I have some assumptions but they are based on your answer and not the interview question. \$\endgroup\$
    – SoEzPz
    Commented Apr 20, 2017 at 15:43
  • \$\begingroup\$ hm, it is this question: softwareengineering.stackexchange.com/questions/340547/… and I used the above form to add the workload. I also had the alternative form in that question, but I think I used the above form in the interview \$\endgroup\$ Commented Apr 21, 2017 at 6:00

4 Answers 4

4
\$\begingroup\$

This is perfectly fine JavaScript, performant even. However...

var i, a, tally = {}, arr = [1, 3, 7, 9, 3, 1, 6, 3, 3, 2, 11, 2];

For better visibility, it's recommended you split up each variable to its own var. Also, to avoid gotchas and IDEs screaming "potentially uninitialized variable" (Intellij does this), initialize them with values.

for (i = 0; i < arr.length; i++) {
  a = arr[i];
  tally[a] = (tally[a] || 0) + 1;
}

Perfectly fine, except it can be better if you use let on i. That way, it strongly indicates that i lives only within the loop body and not anywhere else.

An alternate way to do this is to use array.reduce and carry around the tally object until done. This way, you avoid having to define temporary storage variables and counters.

const arr = [1, 3, 7, 9, 3, 1, 6, 3, 3, 2, 11, 2];
const tally = arr.reduce(function(tally, value){
  tally[value] = tally[value] ? tally[value] + 1 : 1;
  return tally;
}, {});

or more concisely, but entirely unreadable depending on who you ask

const arr = [1, 3, 7, 9, 3, 1, 6, 3, 3, 2, 11, 2];
const tally = arr.reduce((t, v) => (t[v] = (t[v] || 0) + 1, t), {});
\$\endgroup\$
7
  • \$\begingroup\$ Could you elaborate a little bit on your preference of const? Regarding your first statement about giving each variable declaration its own var, I would say one var followed by comma separated declarations each on its own new line is fine, too. \$\endgroup\$
    – le_m
    Commented Apr 17, 2017 at 14:21
  • 1
    \$\begingroup\$ @le_m It's mostly preference. const gives a strong signal that whatever was assigned to it has not changed since assignment. You should not give a variable more than one meaning. Otherwise, it gets confusing. Also, comma-separated vars are hard to move around without. You cannot simply copy-paste lines of variables to different places without having to deal with the commas. \$\endgroup\$
    – Joseph
    Commented Apr 17, 2017 at 14:27
  • \$\begingroup\$ The reduce() method applies a function against an accumulator and each element in the array (from left to right) to reduce it to a single value (MDN). Map should be used here??? \$\endgroup\$
    – SoEzPz
    Commented Apr 20, 2017 at 4:37
  • \$\begingroup\$ @SoEzPz array.map accepts an array and returns an array. \$\endgroup\$
    – Joseph
    Commented Apr 20, 2017 at 12:18
  • 1
    \$\begingroup\$ maybe @SoEzPz wanted to use map with a side effect of setting tally... if so, then side effect probably is not as preferred... how about just using each instead of reduce if there is each support \$\endgroup\$ Commented Apr 21, 2017 at 6:03
2
\$\begingroup\$

(may_be_num || 0) + 1 is perfectly fine, idiomatic in JavaScript, so that shouldn't be an issue.

The name a is not great for a number. num would be better.

Other than this, I doubt there can be significant problems with this simple code, so if the interviewers were not pleased about something, that's probably somewhere else.

\$\endgroup\$
0
0
\$\begingroup\$

Was your interviewer perhaps botherer by the fact that you did not handle type conversion? When mapping those values to an object, you make and implicit cast of the numeric value to a string, so if you are later asked to output the original values with tally for each, you might have needed to convert back to numeric.

\$\endgroup\$
1
  • \$\begingroup\$ oh ok... although sometimes I wonder if they care about such little things on the whiteboard... \$\endgroup\$ Commented Apr 21, 2017 at 5:54
0
\$\begingroup\$

In regards to the original question, I would point out that you are using a general for loop, yet there are other more succinct ways of building this logic.

Perhaps we could assume, that an interviewee that leverages more of the available language is more qualified for a development position.

As a developer on a large project, if I kept running into for loops all over the stack instead of MAP, REDUCE, FILTER, etc... I would voice that some training is needed;

Because less code that is still readable is always the target: less variables, less lines, less time to comprehend...

Below is my answer to Joseph's comment to post an answer. I am posting two, one with MAP and one with REDUCE.

var array = [1, 3, 7, 9, 3, 1, 6, 3, 3, 2, 11, 2];

// MAP -----------------------------------------|

function countDuplicatesMap(num){
  this[num] = (++this[num] || 1);
  return this;
}
   
console.log(array.map(countDuplicatesMap, {}).pop());

// REDUCE -----------------------------------------|

function countDuplicatesReduce(obj, num){
  obj[num] = (++obj[num] || 1);
  return obj;
}
   
console.log(array.reduce(countDuplicatesReduce, {}));

After comparing both approaches of MAP and REDUCE, I would go with REDUCE as shared by Joseph The Dreamer. Using MAP we get an array back that has a pointer to the collection object, but for each element of the original array, thus the POP(), whereas with REDUCE we are only left with a simple object.

NOTE: I reduced the ternary slightly by using the "prefix increment operator".

\$\endgroup\$
3
  • \$\begingroup\$ you sure the code is as readable? maybe for programmers who are quite familiar with reduce? I personally had a hard time knowing too exactly what was going on without taking some guess... I did know inject in Ruby, and map/reduce in MongoDB... but not too familiar with reduce in JS \$\endgroup\$ Commented Apr 21, 2017 at 5:56
  • \$\begingroup\$ Operators are not readable. \$\endgroup\$
    – SoEzPz
    Commented Apr 21, 2017 at 6:37
  • \$\begingroup\$ Also, readable within the context of programming. If you are not familiar with operators, or MAP, REDUCE, FILTER, SOME, EVERY for the array class then please review them. \$\endgroup\$
    – SoEzPz
    Commented Apr 21, 2017 at 16:04

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