patternrubyMinor

Is my solitaire cipher too imperative for an Object Oriented language like Ruby?

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

solitaireliketoolanguageimperativeforrubyorientedobjectcipher

Problem

I am teaching myself Ruby and Ruby-on-rails, as part of this regimen I thought it would be a good idea to do some Ruby quiz exercises and so I started with the solitaire cipher. The basic functionality of decoding a properly formatted message is there but only just so. I've come to the realization that I've written this like Ruby has no object-oriented functionality, and instead it's a big imperative function full of clever expressions that make it hard to read.

At this point I would like to pad it out with a fuller feature set and "unclever" some of the logic, and I wanted to do so via TDD. Is this program hardly unit-testable because of it's imperative design?

As someone who wants to be a great ruby programmer, is it imperative that I refactor this code to utilize classes, methods and objects? If not, will unit-testing be of very limited value as a result? Am I overreacting and this is fine for what it was designed?

```
input = String.new
input = ARGV[0].dup

def solitaire(input)

def decode(msg)

#Creates a hash with keys 1-54
abc = ('A'..'Z').to_a
alphaHash = Hash.new
alphaHash.default = ""
(1..54).to_a.each {|x| alphaHash[x] = (abc[x - 1])} #assigns 1-26 a letter in alphabetical order
abc.each {|x| alphaHash[abc.index(x) + 27] = x} #assigns letters in order to 27-52
#All non-joker card values 1-52 can be resolved to their letter

#Creates array in which each letter from msg is added as a number to the array, A = 1, B = 2 etc.
msg.delete! ' '
convertedMessage = Array.new
msg.each_char {|letter| convertedMessage = 54
jkr_a_idx -= 54 #Reset index to beginning of deck
jkr_a_idx += 1 #Joker can never be first card so it skips index 0
end

#Remove and insert Joker A at new index
deck.delete(53)
deck.insert(jkr_a_idx, 53)

#Joker B down two cards

Solution

Some notes on your code:

alphaHash = Hash.new, each, delete, +=, ...: This shows that you think in imperative terms (init, update, remove, insert, destroy, change, ...), in Ruby is more idiomatic a functional style (see this) with immutable data structures.

or/and are used for flow control, for logic you should use &&/||.

The real problem of your code is that is not declarative. You have a bunch of code put together doing things, but it's difficult to relate each step with the specifications (if you have to insert comments for that, it's a signal something is wrong). The way to solve this is by using abstractions (functions/methods) that capture the specifications.

I'll show the skeleton of my solution, I think it's more useful than going into full detail (ask if you want to see the complete code). Note how every step (in decode,encode` and the deck re-arranging) has its own abstraction, the code is a composition of them:

class Deck  GLNCQ MJAFF FVOMB JIYCB
decoded = SolitaireCipher.decode(encoded)
puts decoded #=> CODEI NRUBY LIVEL ONGER

Code Snippets

class Deck < Array
  def move(card, offset)
  end

  def triple_cut_around(card1, card2)
  end

  def count_cut_last
  end

  def get_output_letter
  end

  def self.value_from_card(card)
  end
end

class SolitaireCipher
  CharsToDigits = Hash[("A".."Z").map.with_index(1).to_a]
  DigitsToChars = CharsToDigits.invert

  def self.gen_keystream
    initial_cards = Deck.new((1..52).to_a + [:joker_a, :joker_b])
    ...
      shuffled_cards = cards.
        move(:joker_a, +1).
        move(:joker_b, +2).
        triple_cut_around(:joker_a, :joker_b).
        count_cut_last
      letter = shuffled_cards.get_output_letter
      [letter, shuffled_cards]
    ...
  end

  def self.chars_to_digits(chars)
  end

  def self.digits_to_chars(digits)
  end

  def self.encode(string)
    s0 = string.upcase.gsub(/[^A-Z]/, '')
    s = s0.ljust((s0.size / 5) * 5, "X")
    digits1 = chars_to_digits(s.chars)
    digits2 = chars_to_digits(gen_keystream.take(s.length))
    digits_encoded = digits1.zip(digits2).map { |d1, d2| (d2 + d1) % 26 }
    digits_to_chars(digits_encoded).each_slice(5).map(&:join).join(" ")
  end

  def self.decode(string)
  end
end

encoded = SolitaireCipher.encode("Code in Ruby, live longer!")
puts encoded #=> GLNCQ MJAFF FVOMB JIYCB
decoded = SolitaireCipher.decode(encoded)
puts decoded #=> CODEI NRUBY LIVEL ONGER

Context

StackExchange Code Review Q#26449, answer score: 3

Revisions (0)

No revisions yet.