counter.rb: Code Review

[scott-steadman]

I'm experimenting with making each review point a separate comment...

[scott-steadman]

Rename the class/file from Counter to Hand. Reason: Use terms from the problem domain. This class looks like it does what a card hand does.

[scott-steadman]

Rename the hand attribute to cards Reason: A hand contains cards.

[scott-steadman]

Rename calculate_score to score and memoize result. Reason: You only need to calculate the score once. Example:

def score
  @score ||= pairs_score + flush_score + fifteen_score + right_jack_score + runs_score
end

[scott-steadman]

Privatize other *_score methods. Reason: Keep your cards interface simple by not exposing more than you have to.

[scott-steadman]

I think Spaid should be spelled Spade

[scott-steadman]

Create a private all_cards method. Reason: DRY.

Example:

def all_cards
  @all_cards ||= cards + top_card
end

Then you can get rid of all your all_cards = cards << top_card calls, and just use all_cards wherever you need them.

[joncodo]

Wow. Thanks so much for doing this.

[scott-steadman]

My pleasure. I'm a big fan of code reviews. :)

[scott-steadman]

Use a has to accumulate suit counts in flush_score. Reason: You can do the same thing in less code.

Cons: This may be less readable to you.

def flush_score
  counts_by_suit = Hash.new(0)

  all_cards.each do |card|
    counts_by_suit[card.suit] += 1
  end

  score = counts_by_suit.values.max
  ...
end

[scott-steadman]

Clean up fifteen_score method Reason: improve readability.

Example code:

def fifteen_score
  two_card_combo_score + three_card_combo_score + four_card_combo_score + five_card_combo_score
end

def two_card_combo_core
...
end
...

If calculating the combo scores can be parameterized, then you can try the following:

def fifteen_score
  [2,3,4,5].inject(0) {|score, combo| score += calculate_combo_score(combo)}
end

def calculate_combo_score(combo, sum=15)
...
end

[scott-steadman]

Don't use hand.each use hand.any? in right_hand_jack score Reason: Needless looping through cards after match found

Example code:

def right_hand_jack_score
  return 1 if hand.any? {|card| card.jack? and card.same_suit?(top_card.suit)}
  return 0
end