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

Question

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 << alphaHash.key(letter)}

    #Create deck array, for this example in ascending numerical order; clubs, diamonds, hearts, spades
    deck = (1..54).to_a

    #Set indexes of two jokers
    jkr_a_idx = deck.index 53
    jkr_b_idx = deck.index 54

    convertedKeys = Array.new

    #This uses the solitaire cipher to generate the keys the message was encrypted with
    while convertedKeys.length < convertedMessage.length

        #Joker A down one card
        jkr_a_idx = deck.index 53
        jkr_a_idx += 1

        #check if it returns to front of deck
        if jkr_a_idx >= 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
        jkr_b_idx = deck.index 54
        jkr_b_idx += 2

        #check if Joker B must return to front of deck
        if jkr_b_idx >= 54
            jkr_b_idx -= 54       #Reset index to beginning of deck
            jkr_b_idx += 1        #Joker can never be first card so it skips index 0.
        end

        #Remove and insert Joker B at new index
        deck.delete(54)
        deck.insert(jkr_b_idx, 54)

        #Triple cut around jokers, exchange cards above first joker with cards below second joker.

        #determine top and bottom jokers
        topJoker = deck.detect {|e| e == 53 or e == 54}
        if topJoker == 53
            bottomJoker = 54
        end

        if topJoker == 54
            bottomJoker = 53
        end

        #Make the cuts
        topCut = deck.slice!(0...deck.index(topJoker))
        if bottomJoker != deck.last         #if a joker is the last card, there is no bottom cut
            bottomCut = deck.slice!((deck.index(bottomJoker) + 1)..-1) #cuts cards after bottom joker to the last one
            deck.unshift bottomCut          #Inserts the bottomCut at the front
            deck.flatten!
        end
        deck << topCut
        deck.flatten!  #deck must be flattened as cuts are inserted as nested arrays

        #Count cut:  take last card's value, cut this many cards from top and insert before last card
        if deck.last == 53 or deck.last == 54           #Either joker's value is always 53
            countCut = deck.slice!(0...53)          #If either joker is the last card, we cut 53 cards
        else 
            countCut = deck.slice!(0...deck.last)
        end
        deck.insert(deck.index(deck.last), countCut)  #inserts the countCut before the last card
        deck.flatten!



        #Take first card's value, count this many cards, convert the facing card to a letter, this is the letter for the keystream
        if deck.first == 54         #All jokers get value 53
            if deck[53] != 53 and deck[53] != 54            #If a joker is the facing card, there is no output to the keystream for this iteration
                convertedKeys << alphaHash.key((alphaHash[deck[53]])) #Any other facing card is converted to a letter, then back to numeric
            end
        else
            if deck[deck.first] != 53 and deck[deck.first] != 54  #Step is skipped if the facing card is a joker
                convertedKeys << alphaHash.key((alphaHash[deck[deck.first]]))
            end
        end 
    end #while loop

    decodedMessage = String.new 

    #Decodes the message
    #Both convertedMessage and convertedKeys are numeric values 1-26
    convertedMessage.each { |value|     
            #When decoding, subtract key from the encoded value for the decoded message
            if convertedKeys[decodedMessage.length] >= value  #If this operation is 0 or negative, add 26 to value
                decodedMessage << alphaHash[((value + 26) - convertedKeys[decodedMessage.length])]
            else
                decodedMessage << alphaHash[(value - convertedKeys[decodedMessage.length])]
            end                     
        }   

    decodedMessage
end  #decode

puts decode(input)
end

puts solitaire(input)

Answer 1

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 < 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