4
\$\begingroup\$

I'm learning JavaScript and have written a function that accepts a string and returns the first word with the greatest number of repeated letters. For example:

Input:"Hello apple pie" - Output:"Hello"
Input:"Hello apple pie yelllow" - Output:"yelllow"

I've tried to keep it as concise as possible. I wonder whether later on in the code it starts to become more verbose. The answer it returns seems to be correct for all tests.

Could someone give some advice about improving my code? Or is it not bad?

function whichWord(str) {

// 1. split into array with words - EG if var str = "wodrd sy hello";
var arr = str.split(' '); // puts str into array ["wodrd", "sy", "hello"]
var mdArr = arr.map(function(el){ return el.split('') }); // puts str into MD array [["w","o","d","r","d"],["s","y"],["h","e","l","l","o"]]

// 2. get each unique letter (remove duplicates)
function removeDuplicates(a) {
 return Array.from(new Set(a)) 
}
var letters = mdArr.map(removeDuplicates);

// 3. count # of each letter occurrences
var temp = [];
var numbers = [];

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

for(k=0;k < letters[i].length;k++) {temp.push(mdArr[i].filter(function(c2){return c2==letters[i][k]}).length)}

numbers.push(temp);
temp=[];

}

var letterCount = letters.map((innerArr, i) => innerArr.map((letter, ii) => ({letter: letter, instances: numbers[i][ii]}))); // puts into MD array with objects:
/* returns: [
    [{letter: "w", instances: 1},{letter: "o", instances: 1},{letter: "d", instances: 2},{letter: "r", instances: 1}],
    [{letter: "s", instances: 1},{letter: "y", instances: 1}],
    [{letter: "h", instances: 1},{letter: "e", instances: 1},{letter: "l", instances: 2},{letter: "o", instances: 1}]
] */

// 4. return word from index with largest number, & first instance if matching numbers
var highestNumbers = letterCount.map((outerArr,i) => Math.max.apply(Math,outerArr.map((innerArr,ii) => innerArr.instances)));

var highestNumber = Math.max.apply(Math,highestNumbers);

var indexToReturn = highestNumbers.findIndex(function(c) { return c == highestNumber } );

return arr[indexToReturn];

}
\$\endgroup\$
2
\$\begingroup\$

This approach seems a bit overly-complicated - there are multiple loops over the word just to remove duplicates, count the letters, etc. I would suggest a simple loop over each word to count the number of repeated numbers, maintaining the max count so far, and then using a simple object mapping (or a Set could be used) to look up the max counts for each word. See the rewritten code below for an example of this.

Additionally, the indentation could be greatly improved. Perhaps the first level inside whichWord in your code was lost because of pasting it into the question. However, most JavaScript code is indented by 2 or 4 spaces inside each block.

I noticed that arrow functions are being used (e.g. letters.map((innerArr, i) => innerArr.map((letter, ii).... This is a feature of and thus other features from that standard could be used, such as const for any variable that is never re-assigned and let for any variable that needs to be reassigned (e.g. iterator variables, cumulative counts, etc.), as well as the spread syntax instead of using function.apply().

Rewritten code

Below is my first take at simplifying the code. It can be compared for performance in this jsPerf.

function maxMode(word) {
  if (word.length == 1) {
    return 0;
  }
  const counts = {};
  for (const letter of word) {
    counts[letter] = (counts[letter] ?? 0) + 1;
  }
  return Math.max(...Object.values(counts));
}
const whichWord = str => {
  const arr = str.split(' ');
  const mappings = arr.reduce((mapping, word) => {
    mapping[word] = maxMode(word);
    return mapping;
  }, {});
  const mappingCounts = Object.values(mappings);
  const highestNumber = Math.max(...mappingCounts);
  const indexToReturn = mappingCounts.findIndex(c => c == highestNumber);
  return Object.keys(mappings)[indexToReturn];
};
const strings = ['Hello a apple pie yellow', 'axaya zza'];
for (const string of strings) {
  console.log('whichWord('+string+'):', whichWord(string));
}

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#Character_access 2https://stackoverflow.com/a/4051431/1575353

\$\endgroup\$
4
  • \$\begingroup\$ interesting and less complex approach, thank you very much. I'll definitely be using the right keywords more often (e.g. const and let). I noticed you used Objects (e.g. const mappingCounts = Object.values(mappings);) - could you link to something that I can read up about this? I haven't come across this much before \$\endgroup\$ – user8758206 Nov 20 '18 at 20:46
  • 1
    \$\begingroup\$ Great- I would start with this MDN article ... there are links at the bottom from there to similar topics like Object Oriented JS, object prototypes, etc. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 20 '18 at 20:49
  • \$\begingroup\$ This behaves differently from the O.P.'s solution in a couple ways: The O.P.'s solution didn't require the repeated characters to be consecutive (compare outputs of "axaya zza"), and if the repeated characters are at the end of the word, this solution fails to find them (count never gets set to maxCount). e.g. the string "aa byyyyy" would give back "aa" in yours. \$\endgroup\$ – Scotty Jamison Jan 12 at 2:50
  • \$\begingroup\$ @ScottyJamison thanks for bringing that to my attention in a civilized way. I have updated the post. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 12 at 17:07
1
\$\begingroup\$

I decided to take a crack at simplifying this problem myself as it sounded like a fun challenge.

@SᴀᴍOnᴇᴌᴀ already offered a bunch of good advice. The main thing I want to add is how important it is to be able to split up the logic into well-named and easily understandable helper functions. You did have one small helper function in there, but one or two more would have helped to divide up the logic further.

Here's my shot at implementing it:

function firstWithHighestScore({ array, scoreFn }) {
  const scores = array.map(scoreFn);
  const highScore = Math.max(...scores);
  return array[scores.indexOf(highScore)];
}

function countLetters(text) {
  const count = {};
  for (const char of text) {
    count[char] = (count[char] ?? 0) + 1;
  }
  return count;
}

const whichWord = text => firstWithHighestScore({
  array: text.split(' '),
  scoreFn: word => Math.max(...Object.values(countLetters(word)))
});

console.log(whichWord('Hello apple    pie'));
console.log(whichWord('Hello apple pie yelllow'));
console.log(whichWord('xwxyx zz'));

You'll notice that the main function, whichWord, is higher level. It's stating that we need to find the "firstWithHighestScore" from a list word words (text.split(' ')), scored by a function that counts letters, and finds the most of a single occurrence.

The helper functions help fill in the details.

\$\endgroup\$
3
  • 5
    \$\begingroup\$ This is a commentary on another answer. While valuable, it doesn't constitute a review of the code in the question, so doesn't belong in an anwser; sorry! \$\endgroup\$ – Toby Speight Jan 12 at 10:34
  • 1
    \$\begingroup\$ This could've been the start of a new answer, but it's not a review. Alternative implementations are only acceptable as part of a review. Pointing out what the bugs are, for example, would be a great start with that. \$\endgroup\$ – Mast Jan 12 at 16:12
  • \$\begingroup\$ @Toby and others - thanks for the feedback. I had already left a comment explaining the bug. I've stopped mentioning it in this answer and added more feedback to make it more of a code review. \$\endgroup\$ – Scotty Jamison Jan 12 at 16:56

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.