My solution for Caesars Cipher project. Would love you feedback

I had two ideas to do it through an array or a string and also through the ASCII conversion(charcodeat/charcode from etc) way. But I chose to do it this way
Would love to have your feedback and also your solutions .

Solution
  **Your code so far**

function rot13(str) {
var alphabets = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
console.log(alphabets[25])
str = str.split("");
function shiftLetters(a) {
  let letterIndex = alphabets.indexOf(a);
  if (letterIndex == -1) { return a; }
  else {
    letterIndex = letterIndex - 13;
    if (letterIndex < 0) { letterIndex = letterIndex + 26; }
    return alphabets[letterIndex];
  };
}
return str.map(a => shiftLetters(a)).join("");;
}

rot13("SERR PBQR PNZC");
  **Your browser information:**

User Agent is: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.212 Safari/537.36 OPR/76.0.4017.177

Challenge: Caesars Cipher

Link to the challenge:

Hello @ankurchaulagain,

Nice it works - I used the ASCII method myself but only because if the information is there already I tend to like to use it rather than recreate it somewhere else (but not always, all depends on readablity, optimisation and other factors).

Anyhow here are some niggle possible pointers and suggestions.

syntax, extra semi-colon [;] are being used, it doesn’t break it, but not usually done.

if(){
  //...
}
else{
  //...
}; //this one
//and
return ... ;;

ease of reading, the if statement using a oneline and then multiline for the else:

if(){}
else{
}
//multiline for both
if(){
}
else{
}

Manipulation of str, one at top and another at bottom separated by function shiftLetters, putting them together may help when issues arise and you need to debug.
Also the return using a oneliner, but also manipulating str above with split - why not combine them

str = ...
function (){
  //many lines
}
return str....

//put them together
function (){
  //many lines
}
str = //...
return str....

//or oneliner them all
function (){
  //many lines
}

return str.split('').map ...

//for readablity possibly split up the oneliner notation:
return str
  .split("")
  .map(a => shiftLetters(a))
  .join("")
;
//but may think str is getting return but is being processed below

within shiftLetters function, possibly use += and -=

letterIndex = letterIndex - 13;
letterIndex = letterIndex + 26;
//+= and -=
letterIndex -= 13;
letterIndex += 26;

Naming of variable and function, alphabets and shiftLetters they both end with s meaning plural, more than one.
alphabets variable is only one alphabet but consists of multiple letters, suggest using var alphabet instead of var alphabets.
shiftLetters function is only shifting one letter and not multiple letters,s uggest using function shiftLetter instead of function shiftLetters.

May consider using === instead of == in the comparision:

Anyhow nice work, these are just some niggly things you might want to consider.

Happy coding :slight_smile:

1 Like

I prefer arithmetic based on the character codes, but this solution is pretty straightforward. You know my opinion - simpler, smaller code is better.

I recommend the use of a formatter and following standard formatting conventions.

1 Like

Will you please take a look at my post here and share your recommendations. Need help with Telephone number validator How to use if...then in RegEx like if one is true another must be true - #2 by lincore

Will you please take a look at this. I am so close but stuck here Need help with Telephone number validator How to use if...then in RegEx like if one is true another must be true - #2 by lincore

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.