Skip to content

Instantly share code, notes, and snippets.

@naimrajib07
Created February 11, 2018 21:58
Show Gist options
  • Save naimrajib07/4e85347755cfeea7164e7f993c8375ba to your computer and use it in GitHub Desktop.
Save naimrajib07/4e85347755cfeea7164e7f993c8375ba to your computer and use it in GitHub Desktop.
def rot13(secrete_messages)
alphabetSet = {1 => 'a', 2 => 'b', 3 => 'c', 4 => 'd', 5 => 'e', 6 => 'f', 7 => 'g',
8 => 'h', 9 => 'i', 10 => 'j', 11 => 'k', 12 => 'l', 13 => 'm', 14 => 'n',
15 => 'o', 16 => 'p', 17 => 'q', 18 => 'r', 19 => 's', 20 => 't', 21 => 'u',
22 => 'v', 23 => 'w', 24 => 'x', 25 => 'y', 26 => 'z'}
secrete_messages.map do |elm|
if (elm == '' || !alphabetSet.has_value?(elm.downcase))
if(elm.length > 1)
rot13(elm.split('')).join()
else
elm
end
else
alpha_index = alphabetSet.key(elm.downcase)
alpha_msg_index = alpha_index % 13
if (alpha_index < 13)
msg = alphabetSet.values_at(alpha_msg_index + 13).first
else
msg = alphabetSet.values_at(roundedIndex(alpha_msg_index)).first
end
msg = msg.upcase if elm == is_uppercase?(elm).to_s
msg
end
end
end
def roundedIndex(index)
index == 0 ? index+ 1 : index
end
def is_uppercase?(elm)
/[[:upper:]]/.match(elm)
end
@benj2240
Copy link

Your primary issue is the translation of m and z. They should be translating to each other, but instead they are both translating to a.

They get alpha_index 13 and 26, and both get alpha_msg_index 0. That means that they are both retrieving

alphabetSet.values_at(roundedIndex(0)).first
alphabetSet.values_at(1).first
['a'].first
'a'

There's a two-step fix to this: Note that the expression x % 13 will give a number from 0-12, never 13. So if you adjust your alphabetSet to

{0=>'a', 1=> 'b' ... 25=>'z'}

You will find that that bug is mostly fixed. The other issue, then is roundedIndex. You should have heard some warning bells in your head when you wrote this function, because the formula you are using to calculate index in the rot13 function gives the correct result most of the time... but some of the time, it just needs to arbitrarily change from 0 to 1? Examining this a little more closely might have led you to make the 0..25 indexing change I've just suggested.

At any rate, you should no longer have need of the roundedIndex function at all, so get rid of it and I believe your code will be functioning. correctly. To verify, try using it to translate the string "ABCDEFGHIJKLMNOPQRSTUVWXYZ" and confirm you get "NOPQRSTUVWXYZABCDEFGHIJKLM".


Beyond getting the code functioning, there are a few other changes I'd recommend, if you're interested. I'd encourage you to consult the ruby-docs as you read through these.

  1. It must have been a pain to type out that entire dictionary. You can save yourself some typing (reduce the possibility of typos) using Range and Enumerable#zip. (0..25) will give you the Range of numbers from 0 to 25, ('a'..'z') will give you the range of letters from a to z, and zip will transform the two ranges into an array of pairs. This can be passed directly the Hash class for pain-free initialization: alphabetSet = Hash[(0..25).zip('a'..'z')]
  2. You're making use of Hash#values_at, but you built the hash, so you know for a fact that each key has only one value. Save some time and just access alphabetSet[alpha_msg_index + 13] or alphabetSet[alpha_msg_index].
  3. You could save some more work with your case-handling code, and get a little fancy with the ternary operator: elm == elm.upcase ? msg.upcase : msg. This would let you remove the is_uppercase function as a trivial one-liner.
  4. It's a little bit awkward that your function has to handle mult-character strings, single-character strings, and arrays of strings. I would probably replace the second-level call with the code directly. If you've been following along, your code will now look like this:
def rot13(secrete_messages)
  alphabetSet = Hash[(0..25).zip('a'..'z')]

  secrete_messages.map do |string|
    string.chars.map do |elm|
      if !alphabetSet.has_value?(elm.downcase)
        elm
      else
        alpha_index = alphabetSet.key(elm.downcase)
        alpha_msg_index = alpha_index % 13
  
        if (alpha_index < 13)
          msg = alphabetSet[alpha_msg_index + 13]
        else
          msg = alphabetSet[alpha_msg_index]
        end
  
        elm == elm.upcase ? msg.upcase : msg
      end
    end.join
  end
end
  1. You should also feel a little awkward performing backwards lookups in alphabetSet. Hashes are made to be very fast in one direction (key -> value, or has_key), but they're no better than any other collection (and perhaps worse) at looking the other way (value -> key, or has_value). So if you want your code to be very fast, it's a good idea to avoid that. So you could make one hash mapping letters to their indexes, and another hash mapping indexes back to letters. indexes = {'a'=>0, 'b'=>1 ...} and letters={ 0=>'a', 1=>'b' ...}. Then the main part of your code would look something like
index = indexes[elm]
index = (index + 13) % 26
msg = letters[index]
  1. But you can do even better! With that code, you're performing two hash lookups and some modular arithmetic for every single character that you translate... and you know that 'a' always translates to 'n', and that 'b' always translates to 'm', and so on. So you might as well make a hash translation = {'a'=>'n', 'b'=>'m' ...}, so that your main code is only msg = translation[elm]. The only question is, what is the best way to initialize that hash? I recommend using Array#rotate:
def rot13 secrete_messages
  alphabet = ('a'..'z').to_a
  translation = Hash[alphabet.zip(alphabet.rotate 13)]
  secrete_messages.map do |string|
    string.chars.map do |elm|
      msg = translation[elm.downcase]
      elm == elm.upcase ? msg.upcase : msg
    end.join
  end
end
  1. Now we're getting close, but that function has a bug: It cannot handle non-alpha characters, like spaces or punctuation. But that has an easy fix, thanks to Hash#default_proc. You can tell the hash, "If I ask about a key you don't have, just give me the key back instead of looking for a value". Here's how: translation.default_proc = proc{|hash, key| key }
  2. As a finishing touch, I'd rename some variables, just to make the function a little easier on the eyes.
def rot13 secret_messages
  alphabet = ('a'..'z').to_a
  translation = Hash[alphabet.zip(alphabet.rotate 13)]
  translation.default_proc = proc{|_, key| key }

  secret_messages.map do |message|
    message.chars.map do |char|
      translated = translation[char.downcase]
      char == char.upcase ? translated.upcase : translated
    end.join
  end
end

Now we've got a nice readable function, that handles rot13 translation on an array of strings, while preserving whitespace and leaving non-alpha characters unchanged. Can we do it in less code? Absolutely. Can we make it faster? You bet. But I'd argue that we would need to sacrifice readability & beauty in order to do either, so I think we're at a pretty happy place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment