1
\$\begingroup\$

say that:

var stringToTest = "Hello i am walking"

And

var searchText = "Hello walking"

I have the following javascript function for filtering.

var searchSplit = searchText.split(" ");
var num_split = searchSplit.length;
var correctTested = 0;
searchSplit.forEach(function (testString) {
    if (stringToTest.indexOf(testString) >= 0) {
        correctTested++;
    }
});

//Decides if the value are close enough
if (correctTested > 0) {
    var percentageCorrect = correctTested / num_split * 100;
    return percentageCorrect >= 50;
}
else {
    return false;
}

I have never done anything like this and i would like if you would rate or even come with suggestions on how to improve the above code.

\$\endgroup\$
1
  • \$\begingroup\$ The expected outcome here is somewhat unclear, for example if the search text is "Hello king", do you expect a match? \$\endgroup\$
    – hallvors
    May 8 '17 at 7:25
1
\$\begingroup\$

My suggestions are mostly focused on making the code as concise and clear as possible.

  1. Simplify the return statement.

    if (correctTested > 0) {
        var percentageCorrect = correctTested / num_split * 100;
        return percentageCorrect >= 50;
    } else {
        return false; 
    }
    

    can be reduced to

    return correctTested >= num_split * 0.5;
    

    This eliminates both the condition as well as concern about div by 0.

  2. Since you do not need a post-increment, choose pre-increment. ++correctTested or correctTested += 1 rather than correctTested++.

  3. Choose clear names.

    searchSplit might be clearer as searchTokens, correctTested as matchedTokens, num_split as tokenCount.

  4. Consistent names. searchCase is camel case while num_split is snake case.

  5. You have not articulated the functional spec, but consider whether the search should be case-insensitive. Also consider whether partial word matches are in fact acceptable or whether you require full token matches.

For ease of testing and for the ability to add different decision criteria you could consider decomposing match counting and the decision criteria into two separate functions.

\$\endgroup\$
2
  • \$\begingroup\$ Agreed, except for the remark on post-increment. In languages other than C++ it is more common to write i++ instead of ++i. \$\endgroup\$ May 8 '17 at 6:48
  • \$\begingroup\$ @RolandIllig I believe that even in C++ post increment is more common. However, personally I think preincrement is a desirable habit both for clarity and because it doesn't rely on the interpreter to optimize out the unused assignment. Arguably the optimization is a micro-optimization but I find the habit costs me nearly nothing and helps reduce cognitive load. \$\endgroup\$
    – user650881
    May 8 '17 at 21:26

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.