2
\$\begingroup\$

I'm preparing for an interview doing the preparation questions on Hacker Rank and I want to become better at this. Is it possible to get some feedback on this? How can I improve my code? How did you solve this question?

function getCount(array){
    let counts = {}
    for(let word of array){
        let count = counts[word]
        counts[word] = count ? counts[word] + 1: 1;
    }
    return counts
}
// Complete the checkMagazine function below.

function compareNoteMag(note,mag){
    let noteKeys = Object.keys(note)
    let string = 'Yes'
    for(let key of noteKeys){
        if(!mag[key]) string = 'No'
        if(mag[key] < note[key]){
            string = 'No'
        }
    }
     console.log(string)
}

function checkMagazine(magazine, note) {
    let magazineCount = getCount(magazine);
    let noteCount = getCount(note);
    compareNoteMag(noteCount,magazineCount)
};

\$\endgroup\$

closed as unclear what you're asking by hjpotter92, Graipher, Heslacher, t3chb0t, Toby Speight Jul 23 '18 at 12:22

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ Welcome to Code Review. Please describe the problem being solved, and add a link to it if possible. \$\endgroup\$ – 200_success Jul 22 '18 at 16:24
1
\$\begingroup\$

Performance

There are several performance issues in the posted code:

  1. The loop over noteKeys continues even after it knows that a word is missing. It should stop.
  2. The map of counts of words in the magazine is unnecessary internal storage. A likely question for discussion during an interview is "what if the magazine doesn't fit in memory, how would you change your implementation?". Food for thought.

Program organization

It's best when a function does one thing. compareNoteMag does two: computes if there are enough words in the magazine, and prints "Yes" or "No". This should have been a function returning boolean, with an appropriate descriptive name.

Safety

The second if should have been an else if here:

if(!mag[key]) string = 'No'
if(mag[key] < note[key]){
    string = 'No'
}

Not only this is inefficient (the second if is sometimes evaluated unnecessarily), the expression in the second if is sometimes a comparison between undefined and a number, which can be confusing and lead to mistakes.

Naming

Some of the names could be better:

  • getCount returns a map of counts (plural), so I would rename to getCounts (plural).
  • getCount takes an array of words: words would be a natural and intuitive name instead of array.
  • compareNoteMag computes the answer to print: answer would be a natural and intuitive name instead of string.

Technique

Instead of this:

let count = counts[word]
counts[word] = count ? counts[word] + 1: 1;

A common technique in JavaScript:

counts[word] = (counts[word] || 0) + 1;

Attention to detail

This is very minor, but I notice some sloppiness:

  • Semicolons are mostly omitted but not always
  • The semicolon is unnecessary at the end of function checkMagazine(...) { ... };
  • The comment // Complete the checkMagazine function below. should not be there
  • The common convention is to put spaces around the parentheses in for(...){ and if(...){
\$\endgroup\$
  • \$\begingroup\$ Regarding the spaces and semicolons, is it okay to say that I will use beautify with whatever settings decided in the project in a real situation? \$\endgroup\$ – Slotmachine Jul 22 '18 at 18:31

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