CODEWITHSUNDEEP
rubyreturn-value
HolyMoly
HolyMoly

Reputation: 2080

why are my returns messing up my results?

The objective of this piece of code (it's part of a larger code) is to determine if a year has duplicate numbers in it. Here is my code:

def no_repeat?(year)
  year = year.to_s
  string = ''

  year.each_char{|i| string << year[i] unless string.include?(year[i])}
  year.length == string.length ? (return false) : (return true)
  end 

puts no_repeat?(1993)

It ALWAYS returns true, and I can't see why that is happening. I have tried expanding the ternary into a full if statement...still returns true. I have tried writing this whole method out as a while loop (with two indexes that compare one index to the other)

def no_repeat?(year)
  year = year.to_s

i = 0
while i < year.length 

    i2 = i + 1
    while i2 < year.length   

      if year[i] == year[i2]
        return false
      else
        return true
      end

      i2 += 1
    end
  i += 1
end

...still returns true. I have tested each thing independently and they all work fine until I put the returns in. What is it about the returns? I need fresh eyes on it.

Upvotes: 1

Views: 54

Answers (2)

Michael Berkowski
Michael Berkowski

Reputation: 270767

The way you've structured the ternary is incorrect. Since your method is attempting to ensure nothing is repeated, it should return true when the == is true. A ternary itself is intended to return a value, not really to execute an expression like (return false) inside its result. That works, but is unconventional to being practically non-existent.

The ternary should look like 

return year.length == string.length ? true : false

Which can of course be simplified because the == expression already returns a boolean.

return year.length == string.length

Next, your use of year[i] isn't quite right. String#each_char is assigning the character value into i, so you can use i directly. It appears that the way you've used it actually does work, but that's not how the iterator variable i is meant to be used.

This makes your method into:

def no_repeat?(year)
  year = year.to_s
  string = ''

  # i represents the character in this iteration
  # so you may just directly reference i here
  year.each_char{|i| string << i unless string.include?(i)}
  # If the lengths match, return true because there's no repeating character
  return year.length == string.length

  # You could omit the 'return' keyword too which is preferred by convention
  # since Ruby returns the last expression's value implicitly
  # year.length == string.length
end

Upvotes: 2

Matt Brictson
Matt Brictson

Reputation: 11102

You have the true and false statements flipped. Otherwise the code works.

This works:

def no_repeat?(year)
  year = year.to_s
  string = ''

  year.each_char{|i| string << year[i] unless string.include?(year[i])}
  year.length == string.length ? (return true) : (return false)
end

no_repeat?(1993) # => false
no_repeat?(2015) # => true

However there are many ways that you should improve this code. The return keyword is rarely used in Ruby. In fact, it is completely superfluous in your example. These two methods are equivalent:

# with `return`
def no_repeat?(year)
  year = year.to_s
  string = ''

  year.each_char{|i| string << year[i] unless string.include?(year[i])}
  year.length == string.length ? (return true) : (return false)
end

# without `return`
def no_repeat?(year)
  year = year.to_s
  string = ''

  year.each_char{|i| string << year[i] unless string.include?(year[i])}
  year.length == string.length
end

Second, using a negative ("no") in a method name makes code hard to follow. I suggest flipping the logic and calling the method repeat? or even better repeat_chars?.

Finally, there are more concise ways to express the logic you have written using built-in Ruby methods. Here is one alternative implementation (I'm sure other Rubyists can chime in with even more elegant solutions):

def repeat_chars?(year)
  year = year.to_s
  year.length != year.chars.uniq.length
end

Upvotes: 2

Related Questions