364d0e86994a268906392f6b6146af38

Ouch... this guy badly needs (but doesn't deserve) help! Original article on RubyFleebie is here : http://www.rubyfleebie.com/rubyize-this-6th-edition

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
class VotingSystem  
  #Hello, I am Rodger the old and unhappy programmer. the variable nbrOfVotes is an array of 2 dimensions. The first dimension contains the number of votes for the answer "YES, IT SUCKS"... and the other dimension contain the number of votes for the answer "NO, IT DOESN'T SUCK". In the near future there will be other possible answers... but I don't care! I retire in 2 days!
  @@nbrOfVotes = Array.new
  
  #The users who sent their vote arrive in this very top secret method!! (I retire in 2 days!)
  def receiveAVote(theVote)
    if theVote == "YES, IT SUCKS"
     @@nbrOfVotes[0] = 0 if @@nbrOfVotes[0].nil?
     @@nbrOfVotes[0] = @@nbrOfVotes[0] + 1
    else
      if theVote == "NO, IT DOESN'T SUCK"
        @@nbrOfVotes[1] = 0 if @@nbrOfVotes[1].nil?
        @@nbrOfVotes[1] = @@nbrOfVotes[1] + 1
      else
          puts "THIS IS NOT A VALID ANSWER YOU MORON... btw i retire in 2 days!"
      end
    end
  end

  #This is the function that compile ALL the votes... I retire in 2 days!
  def compileAllTheVotes
    for i in (0..1)
      if i == 0
        @@nbrOfVotes[0] = 0 if @@nbrOfVotes[0].nil?
        puts "HERE IS THE NUMBER OF VOTES FOR 'YES IT SUCKS' : " + @@nbrOfVotes[0].to_s
      else
        if i == 1
          @@nbrOfVotes[1] = 0 if @@nbrOfVotes[1].nil?
          puts "HERE IS THE NUMBER OF VOTES FOR 'NO IT DOESN'T SUCKS' : " + @@nbrOfVotes[1].to_s
        end
     end
    end
  end
end

Refactorings

No refactoring yet !

16a1b183460e8059f5bd3fd86320a737

Vince

April 17, 2008, April 17, 2008 00:37, permalink

No rating. Login to rate!
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
class VotingSystem
  def initialize
    @votes = {
      "yes, it sucks" => 0,
      "no, it doesn't suck" => 0,
    }
  end
 
  def vote(vote)
    vote.downcase!
    if @votes.has_key? vote
      @votes[vote] += 1
    else
      puts "THIS IS NOT A VALID ANSWER YOU MORON… btw i retire in 2 days!"
    end
  end
 
  def compile_votes
    @votes.each do |vote, count|
      puts "HERE IS THE NUMBER OF VOTES FOR '#{vote}': #{count}"
    end
  end
end
45596033957b2b6d7ef8fe6545e0b7e7

Sam Figueroa

April 17, 2008, April 17, 2008 12:39, permalink

No rating. Login to rate!

almost the same as Vince. But he did a hell of a job, so there isn't much left to change.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
class VotingSystem  
  def initialize
    @votes = {  "yes, it sucks" =>  0, "no, it doesn't suck" => 0, "other"  => 0}
  end
  
  def process_vote(vote)
    begin
      @votes[vote.downcase] += 1
    rescue
      @votes['other'] += 1
    end
  end
  
  def output
    total = @votes.values.inject(0) {|sum, n| sum + n }
    @votes.each do |answer, number|
      percent = number.to_f / total * 100.0
      puts "Here is the number of votes for '#{answer}' : #{number}  => #{percent}%"
    end
  end
end
16a1b183460e8059f5bd3fd86320a737

Vince

April 17, 2008, April 17, 2008 13:08, permalink

No rating. Login to rate!

Cool I didn't know about the begin/rescue for the hash

364d0e86994a268906392f6b6146af38

FrankLamontagne

April 17, 2008, April 17, 2008 13:13, permalink

No rating. Login to rate!

Here is my solution... nothing revolutionary but it does the job. I also added the possibility to vote on multiple questions

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
class VotingSystem
  def vote(answer, question = "is 'the flinstones' a boring t.v. show?")				
    if @@votes.has_key?(question.downcase) && @@votes[question.downcase].has_key?(answer.downcase)
      @@votes[question][answer.downcase] += 1
      puts "There are #{@@votes[question.downcase][answer.downcase]-1} people who think like you!"
    else
      puts "Inexistant question or answer"
    end		
  end	
	
  private
  @@votes =  {	
    "is 'the flinstones' a boring t.v. show?" => {"yes, it sucks!" => 0, "no, it rocks!" => 0},
    "what is your name?" => {"frank" => 0, "what's the point to vote on that?" => 0}		
  }	
end
D86c9b2640edf72f4f002fd066a7b1f2

jswanner

April 17, 2008, April 17, 2008 13:53, permalink

No rating. Login to rate!

not much different than what has already been submitted.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
class VotingSystem  
  @@votes = {  "yes, it sucks" =>  0, "no, it doesn't suck" => 0, "other"  => 0}

  def cast_vote(vote)
    @@votes[vote.downcase] += 1
  rescue
    @@votes['other'] += 1
  end

  def results 
    @@votes.each do |vote, count|
      puts "there are #{count} vote(s) for '#{vote}'"
    end
  end
end
Dd9b278833b2db936814ac37a546a55f

fifoo

April 18, 2008, April 18, 2008 22:00, permalink

No rating. Login to rate!

Maybe not quite a refactoring...

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
# A choice in a Voting System
class VotingChoice
  attr_reader :label

  def initialize(label = "Empty Choice")
    @label = label
  end

  def to_s
    @label
  end
end

# THE Voting System
class VotingSystem
  # Constructor
  # [choices] an array of available choices to the user (you know what they are on opening the vote and they cannot change!)
  def initialize(choices)
    choices << VotingChoice.new("Abstention") # If the user does not know what to vote for

    # A hash of the available choices, used for the UI
    @choices = choices.collect { |c| c.label.to_sym }

    # A hash to record the votes
    @votes = Hash.new
    @choices.each_index do |idx|
      @votes[@choices[idx]] = 0
    end
    # Let's not forget the invalid ones
    @votes[:invalid] = 0
  end

  # Display the choices and record user's vote
  def vote
    @choices.each do |choice|
      puts "  " + choice_id(choice).to_s + ". " + choice.to_s
    end
    print "Please vote: "

    if choice = STDIN.gets
      record_vote(choice.to_i)
    end
  end

  # Display the results
  def results
    puts "\nResults of the vote:"
    # Go through the choices rather than votes so we get a consistent order
    @choices.each do |choice|
      puts "  " + choice.to_s + ": " + @votes[choice].to_s
    end
    # And the invalids
    puts "  Invalid: " + @votes[:invalid].to_s
  end

  private

  def record_vote(choice_id)
    if valid_choice?(choice_id)
      @votes[@choices.at(choice_id-1)] += 1
    else
      puts "THIS IS NOT A VALID ANSWER YOU #*%!... I could do that for an extra 2 years!"
      @votes[:invalid] += 1
    end
  end

  # Return a unique id for the choice. We could have used the array index directly
  # but starting at 0 is not user friendly and dangerous ( "string".to_i => 0 )!
  def choice_id(choice)
    @choices.index(choice) + 1
  end

  def valid_choice?(choice_id)
    ( choice_id > 0 ) && !@choices.at(choice_id-1).nil?
  end
end

vc = [ VotingChoice.new("YES, IT SUCKS"),
       VotingChoice.new("NO, IT DOESN'T SUCK"),
       VotingChoice.new("THERE MUST BE A BETTER WAY") ]
vs = VotingSystem.new(vc)
3.times { vs.vote }
vs.results
4bb774de244da2d6e7f39a189b905077

we4tech

April 19, 2008, April 19, 2008 07:29, permalink

No rating. Login to rate!

most of the suggestions are great, here is my one.
while i complied this suggestion i had the following points in mind -
1. extensibility
2. simplicity in terms of exposing api

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
# voting system related code
class Topic
  attr_accessor :name, :answers
  
  def initialize(p_topic_name)
    @name = p_topic_name
    @answers = {}
  end
  
  def add_answer(p_answer)
    @answers[p_answer.downcase] = 0
    return self
  end
  
  def select_answer(p_answer)
    answer = p_answer.downcase
    verify_valid_answer(answer)
    @answers[answer] = @answers[answer] + 1
  end
  
  def count_response(p_answer)
    answer = p_answer.downcase
    verify_valid_answer(answer)
    return @answers[answer]
  end
  
  private
  def verify_valid_answer(p_answer)
    raise "You have selected an unknown answer - #{p_answer}" if !@answers.has_key?(p_answer)
  end
end

class VotingSystem
  
  @@topics = {}
  
  def self.add_topic(p_topic)
    @@topics[p_topic.name.downcase] = p_topic
    return self
  end
  
  def self.cast(p_topic_name, p_answer)
    topic_name = p_topic_name.downcase
    verify_valid_topic(topic_name)
    @@topics[topic_name].select_answer(p_answer)
  end
  
  # display all results if p_topic_name is not specified
  def self.result(p_topic_name = nil)
    if p_topic_name.nil?
      @@topics.each do |e_topic_name, e_topic|
        show_result_from(e_topic)
      end
    else
      topic_name = p_topic_name.downcase
      verify_valid_topic(topic_name)
      show_result_from(@@topics[topic_name])
    end
  end
  
  private
  def self.verify_valid_topic(p_topic_name)
    raise "You have selected an unknown topic - #{p_topic_name}" if !@@topics.has_key?(p_topic_name)
  end
  
  def self.show_result_from(p_topic)
    puts "Topic - '#{p_topic.name}'"
    p_topic.answers.each do |e_answer, e_count|
      puts "#{e_count} people agreed on '#{e_answer}'"
    end
  end
end

# usages 

# setup voting system with topics
VotingSystem.add_topic(Topic.new("is 'the flinstones' a boring t.v. show?").add_answer("yes, it sucks!").add_answer("no, it rocks!"))
# yet another topic to vote
VotingSystem.add_topic(Topic.new("should i ask again?").add_answer("yes, go on!").add_answer("no, its ok!"))

# add few casts
VotingSystem.cast("is 'the flinstones' a boring t.v. show?", "yes, it sucks!")
VotingSystem.cast("should i ask again?", "no, its ok!")

# view all results
VotingSystem.result

# view specific result
puts "--------------"
VotingSystem.result("is 'the flinstones' a boring t.v. show?")
880cbab435f00197613c9cc2065b4f5a

danielharan

May 4, 2008, May 04, 2008 00:11, permalink

1 rating. Login to rate!

System is a vile word in a class name. It is meaningless and encourages a pile up of concerns that should remain separated.

Since that dude retired 2 weeks ago, I think it's safe to scrap that piece of code. Normally it's better to refactor than re-write... but this is egregious.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
class Poll
  attr_accessor :question
  def initialize(question, answers=[])
    @question, @answers = question, {}
    answers.each {|key| @answers[key] = 0}
  end
  
  def vote_for(answer)
    raise 'unknown answer' unless @answers.has_key?(answer)
    @answers[answer] += 1
  end
  
  def results
    @answers.each { |answer, votes| puts "#{votes} : #{answer}" }
    puts "Out of #{@answers.values.inject {|memo,v| v + memo}} votes"
  end
end

>> p = Poll.new("can haz chezburger?", ["laterz", "nao"])
=> #<Poll:0x35cb2c @answers={"laterz"=>0, "nao"=>0}, @question="can haz chezburger?">
>> p.vote_for "nao"
=> 1
>> p.vote_for "nao"
=> 2
>> p.vote_for "laterz"
=> 1
>> p.results
1 : laterz
2 : nao
Out of 3 votes

Your refactoring





Format Copy from initial code

or Cancel