# Function that returns the number of items that appear more than once

Posted on

Problem

This is the question:

Write a method that takes in a string and returns the number of letters that appear more than once in the string. You may assume the string contains only lowercase letters. Count the number of letters that repeat, not the number of times they repeat in the string.

How can I make this better?

``````function count(str){
var nArr = [];
var result = [];
var obj = {};
var arr = str.split("");
// count all occurances in arr
for(var i = 0; i < arr.length; i++){
var letter = arr[i];
obj[letter] === undefined ? obj[letter] = 1 : obj[letter] += 1;

}
for(var prop in obj){
if(obj[prop] !== 1){
nArr.push(prop);
}
}
return nArr.length;

}

``````

Solution

The name `count` is too vague for such a specific task. I suggest `countReappearingChars` instead.

Avoid big blocks of variable definitions. Code is easier to read and maintain when variables are declared and initialized near where they are used. For example, with a block like this, it’s harder to notice that `result` is never used.

`nArr` does not need to be an array. You can just keep a count.

`obj` is cryptically named. I suggest calling it `charCount`.

`var arr = str.split("")` is unnecessary; you can iterate over `str` directly.

“occurances” is misspelled in the comment.

This line is hard to read:

``````obj[letter] === undefined ? obj[letter] = 1 : obj[letter] += 1;
``````

It would be more clearly be written using if-else, and therefore should be:

``````if (obj[letter] === undefined) {
obj[letter] = 1;
} else {
obj[letter] += 1;
}
``````

I would consider your ternary operator to be abusive, since you are not using it as as an expression. In contrast, this expression could be considered appropriate, since the right-hand side of the assignment is an expression:

``````obj[letter] = (obj[letter] === undefined) ? 1 : obj[letter] + 1;
``````

But that still looks cumbersome. If you want to get clever with it, you could use `|| 0`, as shown below.

``````function countReappearingChars(str) {
// Count all occurrences in str
var charCount = {};
for (var i = 0; i < str.length; i++) {
charCount[str[i]] = 1 + (charCount[str[i]] || 0);
}

// Loop over and filter charCount
var result = 0;
for (var char in charCount) {
if (charCount[char] > 1) result++;
}
return result;
}
``````

The @200_success’s answer is essentially a code review, strictly speaking, and I agree with its recommendations.

In the other hand it keeps a 2-steps strategy like in the OP, so I suggest a slightly different approach, using only 1 loop:

``````function countRepeatedChars(str){
var repChars = '';
for (var i = 0, n = str.length; i <n; i++) {
var letter = str[i];
if (!repChars.includes(letter) && str.includes(letter, i + 1)) {
repChars += letter;
}
}
return repChars.length;
}