5
\$\begingroup\$

This code is supposed to count each character in a given string and account for error handling and type casting as well, and it should do it in the most efficient manner possible, with O(n) being an upper limit for both time and memory. I have gone through my process for problem solving which involves explaining the problem in my own words, looking at the inputs, the outputs, if the answer is even possible given the I/O, and then refactoring my code. Please, if anyone sees a better way to accomplish this then let me know.

function characterCounter(str) {
    // Error Handling
    // No input
    if (!str) {
        return "String input required";
    }

    // attempt to type cast non strings
    if (typeof str !== "string") {
        str = str.toString();
        // input could not be successfully type casted
        if (typeof str !== "string") {
            return "Invalid input";
        }
    }

    const stringLen = str.length;
    const result = {};
    let i = 0;

    // loop through each character for O(n) time complexity and O(n) space complexity
    // since the object grows based on input size
    for (; i < stringLen; i++) {
        // If string not in object as a key then add it and set value to 1
        if (result[str[i]] === undefined && str[i] !== " ") {
            result[str[i].toLowerCase()] = 1;
        } else {
            if (str[i] !== " ") {
                // add one to value at current key
                result[str[i]]++;
            }
        }
    }
    return result;
}

// Returns an object which allows O(1) for insert, deletion, access, and O(n) for searching...
console.log(characterCounter("Hello, World!"));

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Some minor problems

  • The function counts characters in a string and is called characterCounter yet that is not what it does.

    It checks if the argument is not falsy like and can be coerced to a string (which is already known to be true) and if so counts the number of lowercase characters, not spaces, and returns an object containing counts.

    This is very problematic as any code that calls the function must also check that the result it returns is not an Object containing character counts but a string telling the caller that he does not know what a string is. (rude)

  • It is a semantic grinch that I would never use in my code as its name would make meaningless gibberish out of my perfect code (Yes all coders write perfect code, me more so :P).

    Maybe mapCharacterCounts would restore the balance.

  • Object.toString is inherited from Object, any argument passed to the function will have a toString function, thus the return "Invalid input" will only happen if for some reason toString() returned a non string

    Yes possible if someone changes the function but that is not your function's problem.

    If we imagine that a argument could not be converted to a string then there would not be a toString function would there?

  • The function incorrectly counts the number of characters in

    • "" gets ("String input required" ... wtf "" is a string :( me confuseded ),
    • 0 gets ("String input required")
    • "0" gets ({"0":1}),
    • true gets ("String input required"),
    • false gets ({f:1,a:1,l:1,s:1,e:1})

    to name a few.

  • If I pass something that is not string like and that would be coerced to a non representative string, eg {} is [object Object] that would return a Object that counts a very knowable quantity that does not represent the question asked of the function.

    Personally if I called the function characterCounter({}) I would expect an empty object as the return.

    Hang on now I am confused, maybe I should only call the functions with something I know to be string like?

  • Using objects as maps is very ECMAScript5 and is hard to use. It would be much more useful if it returned a Map of character counts

  • You don't count spaces, but you count tabs, form feed, returns and all the other white space characters.

    Why is a space uncountable yet a tab can count (in my book tab is always 4 spaces)

What is a string?

So to make the function practical we first remove the problem of defining what is a string. Its too complicated and can change depending on need.

If you want you can create additional functions to do the vetting and coercion that callers can use to help them work out what type their variables are!!

The function has one role and that is Map Character Counts.

Maybe it would be more practical as?

// Function count characters in the  str returning a map by character
// as {character, count}
// It's the caller's responsibility to ensure the correct type
function mapCharacterCounts(str) {
    str = (str + "").replace(/\s/g,"").toLowerCase();
    const result = new Map();
    var i;

    for (i = 0; i < str.length; i++) {
        const c = str[i];
        if (result.has(c)) { result.get(c).count ++ }
        else { result.set(c, {character : c, count : 1}) }
    }
    return result;
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the detailed answer. Using a Map didn't cross my mind but that makes 100% sense now that I see it! Thanks again. \$\endgroup\$
    – Brian Ruff
    Commented Dec 4, 2018 at 4:53
4
\$\begingroup\$

The algorithm is not working quite as you may expect:

   if (result[str[i]] === undefined && str[i] !== " ") {
        result[str[i].toLowerCase()] = 1;
    } else {
        if (str[i] !== " ") {
            // add one to value at current key
            result[str[i]]++;
        }
    }

If you enter "Hello, World! - Hi!" the two upper case 'H' are only counted as one, because they both evaluate to true in the first if-statement - because you test for an upper case 'H' but save a lower case 'h'.

In the same place you filter spaces out both in true and false, so you could do that as the first thing to do in the loop.

All in all that could be like:

  for (; i < stringLen; i++) {
    let ch = str[i].toLowerCase();
    if (ch !== " ") {
      if (result[ch] === undefined) {
        result[ch] = 1;
      } else {
        result[ch]++;
      }
    }
  }

Use comments to explain why you do what you do (if not obvious), and not what you do - the code shows that.


The whole ting could also be done like this:

function characterCounter(str) {
  if (!str) {
    return "String input required";
  }

  if (typeof str !== "string") {
    str = str.toString();
    if (typeof str !== "string")
      return "Invalid input";
  }

  const result = {};

  for (var i in str) {
    var ch = str[i].toLowerCase();
    if (ch !== " ") {
      if (result[ch] === undefined) {
        result[ch] = 1;
      } else {
        result[ch]++;
      }
    }
  }
  return result;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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