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;
}
count("adsaaaffda");
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;
}
countRepeatedChars("adsaaaffda");
If not already registered as repeated, each character is checked to appear after its own position in the original string, then registered if yes.
It results in a reduced code, and might be a bit faster.