Is my "Caesars Cipher" solution good enough?

Hi there, i just wonder, is my solution of this challange is good? or i can make it better?

function rot13(str) {
  const rot = {
    normal: "ABCDEFGHIJKLMNOPQRSTUVWXYZ",
    caesar: "NOPQRSTUVWXYZABCDEFGHIJKLM"
  }
  let result = "";
  str.split("").forEach( function(element) {
    let index = rot.normal.indexOf(element);
    if ( index >= 0) {
      result+=rot.caesar[index]
    } else {
      result+=element;
    }
  });
  return result;
}

Sorry if such topics not allowed here :frowning:

Define “good enough”? It solves the problem.

There are one line solutions to this, I’m sure. Does that make it better? The problem with super short answers like that is that they are often hard to read and understand. Yours is straight and too the point.

The only think I don’t like about yours is that you keep concatenating the string on each pass. JS needs to reallocate memory on each pass to do that. I might have put them in and array and then joined at the end, or just done it in situ in the array you created.

But still, it’s a good solution. But when you solve these, it’s often good to go out on the interwebs and see how other people solved them - it can be very instructive.

2 Likes

Thanks! “good enough” in my mind its something that don’t use bad practics, cuz when you study you will keep your solutions in mind and reuse them, so i dont want to memorize bad practics. Now i see that use arrays better, than using strings. Thanks :slight_smile:

Sure, it’s a decent approach and decently coded. Is it the best? I don’t know.

If I were doing your approach, I might have done it like this:

function rot13(str) {
  const rot = {
    normal: "ABCDEFGHIJKLMNOPQRSTUVWXYZ",
    caesar: "NOPQRSTUVWXYZABCDEFGHIJKLM"
  }
  return str.split("")
    .map(function(char) {
      const index = rot.normal.indexOf(char)
      return index === -1 ? char : rot.caesar[index];
    })
    .join("");
}

It gets rid of the concatenation problem and (subjectively) tightens up the logic.

1 Like