Skip to main content
added 512 characters in body
Source Link
user229044
  • 853
  • 5
  • 15

Some points about making your code look more like idiomatic Ruby:

  • Use nil.

      choice = -1
    

    should be

      choice = nil
    
  • Don't wrap your conditional in brackets:

      while(choice != 3)
      if(choice == 1)
    

    should be

      while choice != 3
      if choice == 1
    
  • Instead of initializing a variable to an empty array, use map to convert one array directly into the new form and use that to initialize your variable:

    Instead ofInstead of this: Hashes != arrays, sorry. The principal still applies, even if this example isn't equivalent:

       #create empty hashes
       plain_alphabet  = {}
       cipher_alphabet = {} 
    
      "a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
      "A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }
    

    do this:

      plain_alphabet  = ("a".."z").map { |x| (x[0] - "a"[0]) % 26 }
      cipher_alphabet = ("A".."Z").map { |x| (x[0] - "A"[0]) % 26 }
    

    And instead of this:

      ciphertext = ""
    
      #fill in the ciphertext with mapping information in the caesar_map
      plaintext.split("").each do |c|
        ciphertext += $caesar_map[c]
      end
    

    do this:

      ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join
    
  • Use case when appropriate; rewrite your if/elsif/elsif/else as:

      case choice
      when 1:
        # ...
      when 2:
        # ...         
      when 3:
        # ...
      else
        # ...
      end
    

Some points about making your code look more like idiomatic Ruby:

  • Use nil.

      choice = -1
    

    should be

      choice = nil
    
  • Don't wrap your conditional in brackets:

      while(choice != 3)
      if(choice == 1)
    

    should be

      while choice != 3
      if choice == 1
    
  • Instead of initializing a variable to an empty array, use map to convert one array directly into the new form and use that to initialize your variable:

    Instead of this:

       #create empty hashes
       plain_alphabet  = {}
       cipher_alphabet = {} 
    
      "a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
      "A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }
    

    do this:

      plain_alphabet  = ("a".."z").map { |x| (x[0] - "a"[0]) % 26 }
      cipher_alphabet = ("A".."Z").map { |x| (x[0] - "A"[0]) % 26 }
    

    And instead of this:

      ciphertext = ""
    
      #fill in the ciphertext with mapping information in the caesar_map
      plaintext.split("").each do |c|
        ciphertext += $caesar_map[c]
      end
    

    do this:

      ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join
    

Some points about making your code look more like idiomatic Ruby:

  • Use nil.

      choice = -1
    

    should be

      choice = nil
    
  • Don't wrap your conditional in brackets:

      while(choice != 3)
      if(choice == 1)
    

    should be

      while choice != 3
      if choice == 1
    
  • Instead of initializing a variable to an empty array, use map to convert one array directly into the new form and use that to initialize your variable:

    Instead of this: Hashes != arrays, sorry. The principal still applies, even if this example isn't equivalent:

       #create empty hashes
       plain_alphabet  = {}
       cipher_alphabet = {} 
    
      "a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
      "A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }
    

    do this:

      plain_alphabet  = ("a".."z").map { |x| (x[0] - "a"[0]) % 26 }
      cipher_alphabet = ("A".."Z").map { |x| (x[0] - "A"[0]) % 26 }
    

    And instead of this:

      ciphertext = ""
    
      #fill in the ciphertext with mapping information in the caesar_map
      plaintext.split("").each do |c|
        ciphertext += $caesar_map[c]
      end
    

    do this:

      ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join
    
  • Use case when appropriate; rewrite your if/elsif/elsif/else as:

      case choice
      when 1:
        # ...
      when 2:
        # ...         
      when 3:
        # ...
      else
        # ...
      end
    
added 512 characters in body
Source Link
user229044
  • 853
  • 5
  • 15

Some points about making your code look more like idiomatic Ruby:

  • Use nil.

      choice = -1
    

    should be

      choice = nil
    
  • Don't wrap your conditional in brackets:

      while(choice != 3)
      if(choice == 1)
    

    should be

      while choice != 3
      if choice == 1
    
  • UseInstead of initializing a variable to an empty array, use map to convert one array directly into another array;the new form and use that to initialize your variable:

    Instead of this,:

       #create empty hashes
       plain_alphabet  = {}
       cipher_alphabet = {} 
    
      "a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
      "A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }
    

    do this:

      plain_alphabet  = ("a".."z").map { |x| (x[0] - "a"[0]) % 26 }
      cipher_alphabet = ("A".."Z").map { |x| (x[0] - "A"[0]) % 26 }
    

    And instead of this:

      ciphertext = ""
    
      #fill in the ciphertext with mapping information in the caesar_map
      plaintext.split("").each do |c|
        ciphertext += $caesar_map[c]
      end
    

    do this:

      ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join
    
  • Don't wrap your conditional in brackets:

      while(choice != 3)
      if(choice == 1)
    

    should be

      while choice != 3
      if choice == 1
    
  • Use map to convert one array into another array; Instead of this,

      ciphertext = ""
    
      #fill in the ciphertext with mapping information in the caesar_map
      plaintext.split("").each do |c|
        ciphertext += $caesar_map[c]
      end
    

    do this:

      ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join
    

Some points about making your code look more like idiomatic Ruby:

  • Use nil.

      choice = -1
    

    should be

      choice = nil
    
  • Don't wrap your conditional in brackets:

      while(choice != 3)
      if(choice == 1)
    

    should be

      while choice != 3
      if choice == 1
    
  • Instead of initializing a variable to an empty array, use map to convert one array directly into the new form and use that to initialize your variable:

    Instead of this:

       #create empty hashes
       plain_alphabet  = {}
       cipher_alphabet = {} 
    
      "a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
      "A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }
    

    do this:

      plain_alphabet  = ("a".."z").map { |x| (x[0] - "a"[0]) % 26 }
      cipher_alphabet = ("A".."Z").map { |x| (x[0] - "A"[0]) % 26 }
    

    And instead of this:

      ciphertext = ""
    
      #fill in the ciphertext with mapping information in the caesar_map
      plaintext.split("").each do |c|
        ciphertext += $caesar_map[c]
      end
    

    do this:

      ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join
    
Source Link
user229044
  • 853
  • 5
  • 15

  • Don't wrap your conditional in brackets:

      while(choice != 3)
      if(choice == 1)
    

    should be

      while choice != 3
      if choice == 1
    
  • Use map to convert one array into another array; Instead of this,

      ciphertext = ""
    
      #fill in the ciphertext with mapping information in the caesar_map
      plaintext.split("").each do |c|
        ciphertext += $caesar_map[c]
      end
    

    do this:

      ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join