Code Review: JavaScript Syntax, Part III - Cipher Class

Hello,

I was hoping for some review on a part I struggled with for hours only to return what I thought was a very bloated code that seemed to pass. Just wondering what I could have done better.

This was for the challenge code where we had to create a Cipher Class to encrypt and decrypt messages. The below is from the file I saved as I didnt want to throw away hours of work, even if the uses were limited.

Many thanks all!

// Write class below
class ShiftCipher {
    constructor(shiftVal){
      this._shiftVal=shiftVal;}
  
      get shiftVal(){
        return this._shiftVal
      }
  
      encrypt(string){
        const uncodify = (val)=>{
        //console.log(val);
        finalArray.push(String.fromCharCode(val))
        }
        let letterArray=[];
        let updatedArray=[];
        let finalArray=[];
        for(let i=0;i<string.length;i++){
          let upperStr=string.toUpperCase()//changes to UPPERCASE
          let letterCode=upperStr[i].charCodeAt()//CONVERTS TO CODE
          letterArray.push(letterCode)
    } for (let j=0 ; j<letterArray.length; j++){
        if (letterArray[j] < 65){
          updatedArray.push(letterArray[j])
          continue
        }
        if (letterArray[j] > 90){
          updatedArray.push(letterArray[j])
          continue
        }
        if (letterArray[j]+this._shiftVal < 65) { 
          let under = (letterArray[j]+this._shiftVal)-65
          updatedArray.push(91+under)                
        }
        if (letterArray[j]+this._shiftVal > 90){
          let over = (letterArray[j]+this._shiftVal)-90
          updatedArray.push(64+over) 
        }
        if (letterArray[j]+this._shiftVal >= 65 && letterArray[j]+this._shiftVal <= 90){
          updatedArray.push(letterArray[j]+this._shiftVal)
        }
    } updatedArray.forEach(uncodify)
    let finalString = finalArray.join("");
    console.log(finalString)
    return finalString;
  
  
      }
      decrypt(string){
        const uncodify = (val)=>{
          //console.log(val);
          finalArray.push(String.fromCharCode(val))
        }
        let letterArray=[];
        let updatedArray=[];
        let finalArray=[];
        for(let i=0;i<string.length;i++){
          let upperStr=string.toUpperCase()//changes to UPPERCASE
          let letterCode=upperStr[i].charCodeAt()//CONVERTS TO CODE
          letterArray.push(letterCode)
          //console.log("decrypt letterArray = "+letterArray)
      }
       for(let j=0 ; j < letterArray.length; j++){
        if (letterArray[j] < 65){
          updatedArray.push(letterArray[j])
          continue
        }
        if (letterArray[j] > 90){
          updatedArray.push(letterArray[j])
          continue
        }
        if (letterArray[j]-this._shiftVal < 65) { 
          let under = (letterArray[j]-this._shiftVal)-65
          updatedArray.push(91+under)                
        }
        if (letterArray[j]-this._shiftVal > 90){
          let over = (letterArray[j]-this._shiftVal)-90
          updatedArray.push(64+over) 
        }
        if (letterArray[j]-this._shiftVal >= 65 && letterArray[j]-this._shiftVal <= 90){
          updatedArray.push(letterArray[j]-this._shiftVal)
        } //console.log("updatedArray = "+updatedArray)
    } updatedArray.forEach(uncodify)
    let finalString = finalArray.join("").toLowerCase();
    
    console.log(finalString)
    return finalString;
  }
  }
  
  //here, create a new Class with 1 parameter, an integer which is the amount to be shifted
  //methods are .encrypt(string) and .decrypt(string)
  //encrypt CAPITALISES, shifts each letter by the integer value and returns the new string
  //decrypt shifts each letter by the integer value (the opposite way) then UNCAPITALISES and returns the new string
  
  //const x = new ShiftCipher(3);
  //x.encrypt("Your String Here!")

Your code works, but it’s definitely more complex than necessary. Here’s a breakdown of what can be improved:

Issues & Improvements

  1. Redundant Arrays:

    • You’re using three arrays (letterArray, updatedArray, and finalArray), but you could process the string in one pass.
  2. Unnecessary Uppercase Conversion Inside the Loop:

    • You’re converting string.toUpperCase() in every iteration. Instead, you should do it once before the loop.
  3. Too Many Conditional Checks:

    • The logic for handling ASCII shifts could be simplified using modulo arithmetic (%).
  4. Repeated Code in encrypt and decrypt:

    • Both functions share similar logic. A helper function could eliminate redundancy.

Refactored Code

class ShiftCipher {
    constructor(shiftVal) {
        this._shiftVal = shiftVal;
    }

    encrypt(string) {
        return this._shift(string, this._shiftVal, true);
    }

    decrypt(string) {
        return this._shift(string, -this._shiftVal, false);
    }

    _shift(string, shiftAmount, toUpperCase) {
        return [...string]
            .map(char => {
                let code = char.charCodeAt(0);
                if (code >= 65 && code <= 90) { // Uppercase A-Z
                    return String.fromCharCode(((code - 65 + shiftAmount + 26) % 26) + 65);
                }
                if (code >= 97 && code <= 122) { // Lowercase a-z
                    return String.fromCharCode(((code - 97 + shiftAmount + 26) % 26) + 97);
                }
                return char; // Keep punctuation and spaces unchanged
            })
            .join("")
            [toUpperCase ? "toUpperCase" : "toLowerCase"]();
    }
}

// Example usage:
const cipher = new ShiftCipher(3);
console.log(cipher.encrypt("Your String Here!")); // "BRXU VWULQJ KHUH!"
console.log(cipher.decrypt("BRXU VWULQJ KHUH!")); // "your string here!"

Key Improvements

:white_check_mark: One Function Handles Both Encryption & Decryption
:white_check_mark: No Extra Arrays
:white_check_mark: More Readable and Concise
:white_check_mark: Handles Both Uppercase and Lowercase Correctly
:white_check_mark: Preserves Spaces & Punctuation

This version is shorter, faster, and easier to read.

1 Like