2
\$\begingroup\$

I have created this function where it detects the first letter of the string provided. I was hoping if you guys can take a look, give your opinion about it and help me to improve it.

 //This function takes a string and outputs 
 //its first letter
 function letterDetector(str){
      //Checking if string is an empty string
      if(str === "" || str === undefined) 
      {return ("Please input an argument");}

      var alphabetsArr = "abcdefghijklmnopqrstuvwxyz".split("");
     //making user input to lower case
     var string = str.toLowerCase().replace(/[^0-9a-z]\d+/gi, '');
     var result = alphabetsArr.map(letter => {
     if( string.charAt(0) === letter ){
        //logging out the letter as upper case
        console.log("Your first name letter is" + " " + letter.toUpperCase());
     }
   });
}
letterDetector("Adam"); //output A
letterDetector("£1%Adam"); //output A
letterDetector(""); or letterDetector(); //output Please input an argument
\$\endgroup\$
  • \$\begingroup\$ What if the string doesn't begin with a letter? Could you provide a few more examples, and specify exactly what the intended behavior is? \$\endgroup\$ – 200_success Dec 30 '17 at 12:40
  • \$\begingroup\$ @200_success Thanks for pointing out missing points. Now the function removes non alpha numeric chars and numbers from the string. \$\endgroup\$ – Aditya Dec 30 '17 at 17:07
  • \$\begingroup\$ Ugh! Why not just return string.charAt(0).toUpperCase() after stripping nonalpha characters (w/ special case if string ends up empty)? Why go through an unnecessary loop? \$\endgroup\$ – user1149 Dec 31 '17 at 1:06
  • \$\begingroup\$ @Barry Carter did not think through until some of you guys pointed it out. Thank you :) \$\endgroup\$ – Aditya Dec 31 '17 at 9:41
3
\$\begingroup\$

While this function does what you want, it can be greatly simplified.

function letterDetector(str) {
  if (!str) { //Handles both undefined and the empty string
    return "Please input an argument"
  }
  // Replace everything that isn't the letter, then get the first letter.
  const firstLetter = str.replace(/[^a-z]/gi, '').charAt(0)
  return "Your first name letter is " + firstLetter.toUpperCase()
}

for (const str of ["Adam", "£1%Adam", "", undefined]) {
  console.log("'" + str + "'", "-->", letterDetector(str))
}

A few things to note about your implementation:

  1. return is not a function, there is no need to wrap what you are returning with parenthesis (and generally you shouldn't).
  2. Be consistent. Should this function log to the console or return the result? Personally, I prefer returning to avoid side effects, which will help keep your code simpler when working on more complex projects.
  3. Uncovered edge case: What if I input 123?
  4. Don't use (near) keywords as variable names. string looks very close to String and could easily be mistaken for it.
  5. Though in this case it seems you aren't handling non-ascii characters, it is a good idea to use toLocaleLowerCase() and toLocaleUpperCase() instead of toLowerCase() and toUpperCase() to avoid a multitude of problems with languages other than English.
  6. Don't define variables you don't use, result is never used and thus can be dropped.
  7. Since you use arrow functions, you can use const and let instead of var. This is a good switch to make in order to easily avoid any future pain where variables aren't scoped as you expect (especially if you work with other languages like C or Java where everything is block scoped, var is function scoped)
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Another thing you can add, Array#map maps previous array values to new values, which are returned from the callback. If OP isn't returning anything, then don't use map \$\endgroup\$ – Andrew Li Dec 31 '17 at 2:29
  • \$\begingroup\$ @Geritt0 Thank you very much for your answer. I never thought in this way. As I am a beginner this will help me a lot. I'll try to write less code and be productive. Thanks once again. \$\endgroup\$ – Aditya Dec 31 '17 at 9:38
  • \$\begingroup\$ @Andrew Li Thank you for your comment. I'll keep in mind to use the new var used in the map. \$\endgroup\$ – Aditya Dec 31 '17 at 9:40
  • \$\begingroup\$ @Gerrit0 Your code has an Uncovered edge case: What if I input 123? \$\endgroup\$ – Roland Illig Jan 1 '18 at 14:29
  • \$\begingroup\$ @RolandIllig that's purposeful. I mentioned it for the OP to consider (#3) but didn't want to assume what it should do. \$\endgroup\$ – Gerrit0 Jan 1 '18 at 17:36
1
\$\begingroup\$

It seems unnecessary to remove characters you are not interested in. Why no just search for the character you want? Also your solution does not handle the case where there are no 'normalish' letters in the result. I would write it as:

function letterDetector(str) {
  if (!str)
    return "Please input an argument"

  const match = str.match(/[a-z]/i);
  if (!match)  
    return "The string contains no letters";

  return "Your first name letter is " + match[0].toUpperCase();
}

console.log(letterDetector("Adam")); //output A
console.log(letterDetector("£1%Adam")); //output A
console.log(letterDetector("£1%adam")); //output A
console.log(letterDetector("£1%123")); //The string contains no letters
console.log(letterDetector("")); //or
console.log(letterDetector()); //output Please input an argument

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your answer. All of you guys inputs are helping me learn more to be efficient. Thanks once again. \$\endgroup\$ – Aditya Jan 1 '18 at 23:08

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.