CODEWITHSUNDEEP
ruby
MrPizzaFace
MrPizzaFace

Reputation: 8086

Syntax error? Ruby bug? Issue with instance variable incrementing with += 1?

def isPrime?(num)

    i = 2

    @isFalse = 0

    while i < num
        divisible = ((num % i) == 0)
        if divisible
            @isFalse += 1
            return false
        end
        i += 1
    end
    true
end

def primes(max)
    startTime = Time.now
        (2..max).each do |p|
            puts "#{p} #{isPrime?(p)}"
        end
    endTime = Time.now

    puts "--------------------------------------------------"
    puts "   FALSE values in range from (2 thru #{max}): #{@isFalse}  \n    TRUE values in range from (2 thru #{max}): #{max-1-@isFalse}"
    puts "\nTotal time to calculate is #{endTime - startTime} seconds!"
end

primes(10)

isPrime? checks if a given number is a prime number. primes loads a range of numbers and checks if each is a prime.

I want to know how many numbers are prime within the range and how many aren't.

I added @isFalse += 1 thinking I can increment each time false is returned so that I can determine how many numbers in the range are false and use this to subtract from max to get how many numbers are true.

Entire code is working correctly except @isFalse is not properly incrementing. Why is that? Thanks for the help.

--UPDATE--
My Output: added puts "About to increment @isFalse" before @isFalse += 1

2 true
3 true
About to increment @isFalse
4 false
5 true
About to increment @isFalse
6 false
7 true
About to increment @isFalse
8 false
About to increment @isFalse
9 false
About to increment @isFalse
10 false
--------------------------------------------------
   FALSE values in range from (2 thru 10): 1  
    TRUE values in range from (2 thru 10): 8

Upvotes: 0

Views: 107

Answers (4)

Cary Swoveland
Cary Swoveland

Reputation: 110755

As your question has been answered, I would like to suggest a more Ruby-like solution:

You want the method is_prime? to do nothing other than determine if num is prime:

def is_prime?(num)
  (2...num).each {|i| return false if num % i == 0}
  true
end

Do your counting of primes in the method nbr_primes. 

def nbr_primes(max)
  return false if max < 2
  (2..max).reduce(0) {|tot, i| tot + (is_prime?(i) ? 1 : 0)}
end

p nbr_primes(20) # => 8

A few points:

  • I removed the references to time and the output formatting as they are not central to the question.
  • It's a Ruby convention to name methods with lower case letters and underscores.
  • (2...num), because it has three dots, is the sequence from 2 to (and including) num - 1. We could instead write (2..num-1) (two dots), which is favored by some.
  • (2..max).reduce(0) {|tot, i|...} iterates over i from 2 to and including max. tot, which is reduce's accumulator, is incremented by 1 for each number between 2 and max that is found to be prime. The expression returns the value of tot, which in turn is returned by the method nbr_primes. inject is a synonym for reduce.
  • In checking whether a number n is prime, you only have to check if its divisible by numbers up to Math.sqrt(n).to_i.
  • With require 'prime', you don't have to reinvent the wheel. For example, Prime.prime?(7) #=> true, and Prime.take_while {|p| p <= 10}.size #=> [2, 3, 5, 7].size + 1 # => 4.

Upvotes: 1

steenslag
steenslag

Reputation: 80085

@isFalse = 0 is inside your isPrime? method. Get it out of there!

Upvotes: 0

Reinstate Monica -- notmaynard
Reinstate Monica -- notmaynard

Reputation: 5490

Each time isPrime? is called, @isFalse is reset to 0. So the result of @isFalse being 1 is from the last time isPrime? was called (with num equal to 10, incrementing @isFalse to 1).

Upvotes: 2

Stoic
Stoic

Reputation: 10754

It seems that you are trying to find out the number of primes in that range using @isFalse. If that is the case, you should instead use the following modification of your code (although, I do not recommend checking for primes in this way, and the code is really inefficient):

Basically, I removed the instance variable @isFalse altogether, and checked whether a number is prime or not in your second method, instead. The code is much cleaner that way, and really does what you intend it to do.

The problem with your code is that @isFalse is being reset to 0 everytime your first method isPrime? was being called, and hence, does not properly reflect the number of primes in the given range.

def isPrime?(num)
    i = 2
    while i < num
        divisible = ((num % i) == 0)
        return false if divisible
        i += 1
    end
    true
end

def primes(max)
    startTime = Time.now
        is_prime = 0
        (2..max).each do |p|
            if isPrime?(p)
                is_prime += 1
                puts p
        end
    endTime = Time.now

    puts "--------------------------------------------------"
    puts "   FALSE values in range from (2 thru #{max}): #{is_prime}  \n    TRUE values in range from (2 thru #{max}): #{max-1-is_prime}"
    puts "\nTotal time to calculate is #{endTime - startTime} seconds!"
end

primes(10)

Upvotes: 1

Related Questions