CODEWITHSUNDEEP
rubyrefactoring
Nappstir
Nappstir

Reputation: 995

Any idea on how I would refactor this class?

class Die
  def initialize(labels)
     if labels.length < 1
       raise ArgumentError.new('Please put one letter of the alphabet.')
     end

     @labels = labels
  end

  def sides
    @labels.length
  end

  def roll
    @labels.sample
  end
end

p die = Die.new(['A', 'B', 'C', 'D', 'E', 'F'])

p die.roll

Just so all are aware, this code should return the number of sides the die has if you call the sides method. It should return a random side of the die if you call the roll method. If no arguments are passed to the array it should return the customized ArgumentError message.

Upvotes: 0

Views: 79

Answers (2)

Dbz
Dbz

Reputation: 2761

This class should not be refactored. The purpose of refactoring is to keep code clean, readable, efficient, and maintainable. That can mean a lot of things from finding ways to reuse code instead of writing it twice, or to split up methods that are longer than ten lines into multiple smaller methods.

I highly recommend reading the (unofficial) ruby style guide.

You'll notice that your class and method names are descriptive of what the object is and what it does, and the methods are all under ten lines. These are good signs that the code doesn't need to be refactored. If anything you might want to change the method name sides to num_sides to be more descriptive or @labels to @faces.

EDIT: You can always turn an if statement into one line and will not require an end

raise ArgumentError.new('Please put one letter of the alphabet.') if labels.length < 1

TL;DR This is clean code and doesn't need refactoring.

Upvotes: 1

Mark Swardstrom
Mark Swardstrom

Reputation: 18080

You could do this.

class Die < Array
  def initialize(labels)
     if labels.length < 1
       raise ArgumentError.new('Please put one letter of the alphabet.')
     end

     super
  end

  alias_method :sides, :length
  alias_method :roll, :sample

end

Upvotes: 0

Related Questions