2
\$\begingroup\$

I am a newbie in Ruby. I want to learn Ruby and I want to learn clear code in Ruby. I coded a number guessing game but my code is smelling bad. How I can do better code?

class NumberGuess
  attr_accessor :num_of_guess, :random_num, :name

  def initialize(random_num, num_of_guess, name)
    @name = name
    @random_num = random_num
    @num_of_guess = num_of_guess
  end

  def start
    while num_of_guess < 10
      puts ""
      puts "1 ile 100 arasında sayı tut"
      guess_number = gets.to_i
      @num_of_guess += 1
      guesses_left = (10 - @num_of_guess)

      if random_num > guess_number
        puts "#{@name}, Tahminin tutulan sayıdan daha düşük. Kalan hakkin #{guesses_left} "
      elsif guess_number > random_num
        puts "#{@name}, Tahminin tutulan sayıdan daha büyük. Kalan hakkin #{guesses_left} "
      end
      break if guess_number == random_num
    end

    if guess_number == random_num
      puts "Tebrikler #{@name}! #{num_of_guess} denemede bildin"
    else
      puts "Üzgünüm #{@name}! Cevap: #{random_num} "
    end
  end

end

puts ("Merhaba,Ismini ogrenebilir miyim?")
name = gets.chomp.capitalize
game = NumberGuess.new(rand(100), 0, name)
game.start
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Consistency

Sometimes you use accessor methods, sometimes you access instance variables directly. Sometimes you use parentheses around message send arguments, sometimes you don't.

You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.

Most communities have developed standardized community style guides. In Ruby, there are multiple such style guides. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (single quotes or double quotes).

In general, if you use two different ways to write the exact same thing, the reader will think that you want to convey a message with that. So, you should only use two different ways of writing the same thing IFF you actually want to convey some extra information.

For example, some people always use parentheses for defining and calling purely functional side-effect free methods, and never use parentheses for defining and calling impure methods. That is a good reason to use two different styles (parentheses and no parentheses) for doing the same thing (defining methods).

Single-quoted strings

If you don't use string interpolation, it is helpful if you use single quotes for your strings. That way, it is immediately obvious that no string interpolation is taking place.

"Merhaba,Ismini ogrenebilir miyim?"

should instead be

'Merhaba,Ismini ogrenebilir miyim?'

Note that it is perfectly fine to use double quoted strings if you otherwise needed to use escapes, e.g. here:

"Congratulations, you've guessed the word!"

reads much better than

'Congratulations, you\'ve guessed the word!'

Frozen string literals

Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:

# frozen_string_literal: true

It is generally preferred to add this comment to all your files.

No space before argument list

In a message send, there should be no whitespace between the message and the argument list, e.g. this:

puts ("Merhaba,Ismini ogrenebilir miyim?")

should be

puts("Merhaba,Ismini ogrenebilir miyim?")

This will bite you as soon as you have more than one argument. Ruby allows you to write the argument list to a message send without parentheses like this:

foo.bar baz, quux

instead of

foo.bar(baz, quux)

The problem arises because the whitespace triggers the first mode, so

foo.bar (baz, quux)

is not interpreted as two arguments baz and quux but as passing arguments with whitespace instead of parentheses and thus as a single argument (baz, quux). Except (baz, quux) is not legal Ruby code, and thus you get an error.

Linting

You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.

Rubocop was able to detect all of the style violations I pointed out above (plus some more), and also was able to autocorrect all of the ones I listed.

Let me repeat that: I have just spent two pages pointing out how to correct tons of stuff that you can actually correct within milliseconds at the push of a button. I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit "save".

In particular, running Rubocop on your code, it detects 9 offenses, of which it can automatically correct 6.

Here's what the result of the auto-fix looks like:

# frozen_string_literal: true

class NumberGuess
  attr_accessor :num_of_guess, :random_num, :name

  def initialize(random_num, num_of_guess, name)
    @name = name
    @random_num = random_num
    @num_of_guess = num_of_guess
  end

  def start
    while num_of_guess < 10
      puts ''
      puts '1 ile 100 arasında sayı tut'
      guess_number = gets.to_i
      @num_of_guess += 1
      guesses_left = (10 - @num_of_guess)

      if random_num > guess_number
        puts "#{@name}, Tahminin tutulan sayıdan daha düşük. Kalan hakkin #{guesses_left} "
      elsif guess_number > random_num
        puts "#{@name}, Tahminin tutulan sayıdan daha büyük. Kalan hakkin #{guesses_left} "
      end
      break if guess_number == random_num
    end

    if guess_number == random_num
      puts "Tebrikler #{@name}! #{num_of_guess} denemede bildin"
    else
      puts "Üzgünüm #{@name}! Cevap: #{random_num} "
    end
  end
end

puts('Merhaba,Ismini ogrenebilir miyim?')
name = gets.chomp.capitalize
game = NumberGuess.new(rand(100), 0, name)
game.start

And here are the offenses that Rubocop could not automatically correct:

Inspecting 1 file
C

Offenses:

number_guess.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
class NumberGuess
^^^^^
number_guess.rb:12:3: C: Metrics/AbcSize: Assignment Branch Condition size for start is too high. [<3, 16, 11> 19.65/17]
  def start ...
  ^^^^^^^^^
number_guess.rb:12:3: C: Metrics/MethodLength: Method has too many lines. [18/10]
  def start ...
  ^^^^^^^^^

1 file inspected, 3 offenses detected

Similar to Code Formatting, it is a good idea to set up your tools such that the linter is automatically run when you paste code, edit code, save code, commit code, or build your project, and that passing the linter is a criterium for your CI pipeline.

In my editor, I actually have multiple linters and static analyzers integrated so that they automatically always analyze my code, and also as much as possible automatically fix it while I am typing. This can sometimes be annoying (e.g. I get almost 30 notices for your original code, lots of which are duplicates because several different tools report the same problem), but it is in general tremendously helpful. It can be overwhelming when you open a large piece of code for the first time and you get dozens or hundreds of notices, but if you start a new project, then you can write your code in a way that you never get a notice, and your code will usually be better for it.

Print empty line

The idiomatic way of printing an empty line is just

puts

instead of

puts ''

Unnecessary parentheses

There is no need for these parentheses:

guesses_left = (10 - num_of_guess)

It should just be

guesses_left = 10 - num_of_guess

Inconsistent use of parentheses for message send argument lists

You are sometimes using parentheses around arguments to message sends, and sometimes not. Remember what I wrote in the beginning: if you use different styles to write the same thing, people will assume that you want to draw attention to some sort of difference. So, you should only do this in case you actually want to point out some difference.

For example, there are actually a couple of style conventions around parentheses for argument lists. One style convention is to use parentheses for purely functional methods and no parentheses for impure procedural methods. Another convention is to use parentheses for almost all methods, and no parentheses for "global" kernel procedures that aren't really used as methods at all (e.g. Kernel#puts or Kernel#require), or for methods that feel like language keywords (Module#include or Module#attr_accessor).

But in your case you are, for example, using no parentheses here:

puts '1 ile 100 arasında sayı tut'

but you are using parentheses here:

puts('Merhaba,Ismini ogrenebilir miyim?')

Inconsistent use of attribute methods and instance variables

You are sometimes accessing instance variables directly e.g. here

@num_of_guess += 1

and sometimes you use the attribute method, e.g. here

while num_of_guess < 10

This is inconsistent. Choose one or the other.

I personally prefer to always use the attribute methods, because methods are more flexible: they can be overridden in subclasses or their implementation can be changed, without having to change any of the client code.

So, you should either change the second example to

while @num_of_guess < 10

or (my preference) the first example to

self.num_of_guess += 1

and the same in a couple of other places.

attr_reader vs. attr_accessor

In your original code, you are never writing to any of your attributes, so they should all be Module#attr_readers instead of Module#attr_accessors:

attr_reader :num_of_guess, :random_num, :name

However, if you follow my suggestion above, and always use the writers as well, then they need to stay accessors.

Access Restrictions

None of your attribute accessors are intended to be used by other objects. In fact, they shouldn't be used by other objects! They are the private internal state of the number guesser object. Therefore, they should not be part of the public API, they should be private:

private

attr_accessor :num_of_guess, :random_num, :name

Inconvenient API

The API of your object places a lot of unnecessary burden on the client. For example: why is it the client's responsibility to pass in the random number? Why is it the client's responsibility to pass in 0 to initialize num_of_guess? Your object should be able to do that on its own.

Unnecessary instance variables

The instance variables @random_num and @num_of_guess are only ever used in one method. They should be local variables instead.

Overcomplicated logic

As I mentioned above, the linters and static analyzers are still complaining about some stuff. Mostly, they are complaining about the length and the complexity of the start method.

However, there is one specific complaint by Reek:

[30, 33]:DuplicateMethodCall: NumberGuess#start calls 'guess_number == random_num' 2 times

The simplest possible way to get rid of this warning would be to assign the result of guess_number == random_num to a variable (for example won) and use it in both places.

However, this actually hints at some greater simplification that we can do: not only remove the duplicated expression guess_number == random_num but actually remove the duplicated check altogether! If we return from the whole method from inside the loop as soon as we know the player has won, there is no need to check again after the loop.

And since we are now checking for equality earlier, we don't need to check for both greater-than and less-than. One of the two is enough.

Iterators

In Ruby, you almost never use loops. You would normally use at least an low-level iterator such as Kernel#loop, #each, or Integer#times. Really, you want to use higher-level iterators such as Enumerable#map, Enumerable#select, Enumerable#group_by, Enumerable#flat_map, Enumerable#inject, etc.

In this case, we are going to use Integer#upto for the loop.

The method now looks like this:

def start
  random_num = rand(100)

  1.upto(10) do |num_of_guess|
    puts
    puts '1 ile 100 arasında sayı tut'
    guess_number = gets.to_i
    guesses_left = 10 - num_of_guess

    if guess_number == random_num
      puts "Tebrikler #{name}! #{num_of_guess} denemede bildin"
      return
    end

    if random_num > guess_number
      puts "#{name}, Tahminin tutulan sayıdan daha düşük. Kalan hakkin #{guesses_left} "
    else
      puts "#{name}, Tahminin tutulan sayıdan daha büyük. Kalan hakkin #{guesses_left} "
    end
  end

  puts "Üzgünüm #{name}! Cevap: #{random_num} "
end

Naming

There are a number of names that are somewhat confusing, misleading, or could be expressed better.

For example, random_num doesn't really tell us what this variable is for. It tells us that it is a random number, but what does it do? I would name this target, for example, since it is the target the player is trying to hit.

guess_number can be just guess. We know it's a number guessing game, so we can probably assume that the user's guess is a number. num_of_guess could be just try. guesses_left would then be tries_left.

Also, the start method does a lot more than just start the game. It is the game. So, it should probably be called game.

Magic numbers

There are some hard-coded magic numbers in your code, which show up multiple times. If you ever want to change those magic numbers, you will need to change them in multiple places, and you might forget one. (This actually happened to me: I wanted to be able to test the game faster, so I reduced the number of tries and the bound of the target number, but I forgot one place.)

The two magic numbers are 10, the maximum number of tries, and 100 the upper bound of the target number. We could make those constants of the class, attributes, or something else.

Complexity

Your start method does a lot of work. In particular, it doesn't actually do what it claims it does: it doesn't just start the game, it is the entire game.

It also mixes input/output with computation. It is generally a good idea to separate input/output from computation. One obvious advantage is that you can test your computation by simply calling methods and passing arguments instead of having to type inputs and read outputs.

We should probably break this up in some smaller methods.

I would suggest, for example, a method for the actual game loop, a method for asking for input, a method for determining the result, and some methods for printing the result.

Proposal

# frozen_string_literal: true

class NumberGuess
  private

  DEFAULT_TRIES = 10
  private_constant :DEFAULT_TRIES

  attr_accessor :name, :tries, :bound, :target

  def initialize(name, tries: DEFAULT_TRIES, bound: tries**2, target: rand(bound))
    self.name = name
    self.tries = tries
    self.bound = bound
    self.target = target
  end

  public

  def game
    1.upto(tries) do |try|
      case result(ask)
      when :high
        high(try)
      when :low
        low(try)
      when :won
        return won(try)
      end
    end

    lost
  end

  private

  def ask
    puts
    puts "1 ile #{bound} arasında sayı tut"
    gets.to_i
  end

  def result(guess)
    if guess > target
      :high
    elsif guess < target
      :low
    else
      :won
    end
  end

  def high(try) = puts("#{name}, Tahminin tutulan sayıdan daha büyük. Kalan hakkin #{tries_left(try)}")

  def low(try) = puts("#{name}, Tahminin tutulan sayıdan daha düşük. Kalan hakkin #{tries_left(try)}")

  def won(try) = puts("Tebrikler #{name}! #{try} denemede bildin")

  def lost = puts("Üzgünüm #{name}! Cevap: #{target}")

  def tries_left(try) = tries - try
end

puts 'Merhaba,Ismini ogrenebilir miyim?'
name = gets.chomp.capitalize
game = NumberGuess.new(name)
game.game

There are still two complaints by our static analyzer tools, unfortunately:

  1. There is no documentation.
  2. The game method is still too long.

#1 can easily be fixed. (I will leave that up to you.) #2 would require some more extensive refactoring of the game logic, and maybe also some rethinking: for example, why limit the number of tries at all? With an upper bound of 100, for example, a clever player will need at most 7 tries to guess the number. So, we could just let them play until their guess is correct. Or, we allow them to abort the game at any point.

\$\endgroup\$
2
\$\begingroup\$

I'm not a Ruby programmer, but there are a couple of general improvements I can think of for this code:

  • Better initialization: There is no need to pass the random number and num_of_guess to the class. Generating a random number is part of the core definition of this game, and I will put it in the class. For num_of_guess, just set it to zero. There's no need to pass it to class constructor.
  • I try to avoid conditional statements as much as possible. They make the code hard to read. So in the loop, instead of break when you find the solution, print the success message and return. In this way, after the loop, you know that the player didn't find the answer, and you just print a failure message.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.