Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to create a randomly generated array of x numbers with no duplicates. I am choosing numbers, one at a time, then comparing each new number to the numbers already in the array.

Here is my code. At the end I should have an array of seven numbers, none that match, but sometimes I get a nil:

array = [] #DEFINES THE ARRAY

array[0] = rand(10)  #THIS CHOOSES FIRST NUMBER (FN) AND PUTS IT IN THE FIRST ARRAY SPOT
p "fn = #{array[0]}"

for i in 1..6  #START OF FOR LOOP TO GET 6 NUMBERS IN THE ARRAY - WHY 6?  THE FIRST NUMBER THAT POPPED IN MY HEAD

  p "Iteration #{i} -------------------------"  # THIS IS JUST SO I KNOW WHERE I AM IN THE LOOPS

  @x = rand(10)  #THIS CHOOSES THE NEXT NUMBER AND ALL NUMBERS AFTER

  array.each do |uc|  # THIS IS THE LOOP THAT COMPARES ALL NUMBERS

    @type = @x == uc ? "yes" : "no"  #DOES THE CHOSEN NUMBER EQUAL ANY NUMBER IN THE ARRAY

    p "does #{uc} = #{@x}? #{@type}"
    if @type == "yes"  # IF THE COMPARE IS TRUE, I DON'T WANT ANYTHING DONE.  IT WILL CYCLE THRU AND GET A NEW NUMBER

      i = 1 
      p "YES #{@x} = #{uc}"
      break
    end  #END OF IF YES
  end  #END OF ARRAY EACH

  if @type == "no"  #IF NO, PUT NEXT NUMBER (@X) INTO THE NEXT ARRAY SPOT.

    p "in last if type= #{@type}"  #THESE STATEMENTS JUST PRINT OUT THE DIFFERENT VARIABLES SO I KNOW I AM GETTING WHAT I EXPECT

    p "in last if x = #{@x}"
    p "in last if i = #{i}"
    @x = array[i]  #THIS "SHOULD" FILL THE NEXT ARRAY SPOT - BUT DOESN'T SEEM TO

    p "#{array[i]} is in the array"  #THIS PRINT OUT IS BLANK - STATEMENT ABOVE DID NOT WORK.

    p array[i]
  end  #END OF IF NO

end  #END OF FOR LOOP

p array #PRINTS OUT THE CONTENTS OF THE ARRAY

I know there are probably quicker and easier ways to do this, but I am starting out with what I know, and building up.

share|improve this question
add comment

3 Answers

up vote 4 down vote accepted

I don't understand what you really want, but this simple one-liner generates an array of ten elements (from 1 to 10) with a random order. Tweak it to meet your needs:

(1..10).to_a.shuffle
#=> [4, 10, 1, 7, 3, 5, 8, 2, 9, 6]
share|improve this answer
    
This is great, and I thank you. –  Kyzer Jan 15 '13 at 17:09
    
Actually it had to be (0...10).to_a.shuffle.take(7). –  Nakilon Jan 15 '13 at 18:54
    
+1 "zen-like". Nicely done. –  the Tin Man Jan 16 '13 at 3:56
    
Why did u append @steenslag answer? –  Nakilon May 22 '13 at 14:10
1  
@Nakilon. Ok, deleted. –  tokland May 22 '13 at 18:01
show 2 more comments

The sample method does what you want:

(1..10).to_a.sample(7) #=> [2, 9, 1, 6, 8, 10, 4]
share|improve this answer
add comment
  1. Why do you put empty lines everywhere? They don't make your code more readable.
  2. If you used @ not because you are writing classes, but just to fix scope problems, you may try either use $ instead or even better define type variable outside the nested loop (before array.each do |uc|).
  3. And the main thing. Ruby's loops are more highlevel, than in, for example, C. And you can't directly change the i value. So you should use keyword redo to repeat the iteration. http://www.ruby-doc.org/docs/ProgrammingRuby/html/tut_containers.html
  4. You may use puts instead of p to get rid of "", but it may need STDOUT.sync = true fix under Windows.
  5. Also you need array[i] = x instead of x = array[i]. If you aren't experienced in programming, I would recommend to start with C/C++.

So here is your program. Not refactored yet, but fixed.

array = []
array[0] = rand 10
puts "fn = #{array[0]}"
for i in 1..6
  puts "Iteration #{i} -------------------------"
  x = rand 10
  array.each do |uc|
    $type = x == uc ? "yes" : "no"
    puts "does #{uc} = #{x}? #{$type}"
    if $type == "yes"
      puts "YES #{x} = #{uc}"
      break
    end
  end
  if $type == "no"
    puts "in last if type= #{$type}"
    puts "in last if x = #{x}"
    puts "in last if i = #{i}"
    array[i] = x
    puts "#{array[i]} is in the array"
    p array[i]
  else
    redo
  end
end
p array

Now refactoring:

You need to know, that Ruby arrays have a method #include?, which you may use instead of loop. We will lose debugging ability a bit, but it makes program more pretty, and now we don't need to make type variable global.

array = []
array[0] = rand 10
puts "fn = #{array[0]}"
for i in 1..6
  puts "Iteration #{i} -------------------------"
  x = rand 10
  type = array.include?(x) ? "yes" : "no"
  puts "does array include #{x}? #{type}"
  if type == "no"
    puts "in last if type=#{type}, x=#{x}, i=#{i}"
    array[i] = x
    puts "#{array[i]} is in the array"
  else
    redo
  end
end
p array

Then get rid of "yes" and "no", and use native Ruby true and false. And you may add tap to any object for debugging purposes - this universal method returns the object itself after execution, so you will be able to comment it later via putting # before .tap (when you don't need debug output) without breaking the program.

array = []
array[0] = rand 10
puts "fn = #{array[0]}"
for i in 1..6
  puts "Iteration #{i} -------------------------"
  x = rand 10
  if (type = array.include? x).tap{ puts "does array include #{x}? #{type : "yes" : "no"}" }
    redo
  else
    puts "in last if type=#{type}, x=#{x}, i=#{i}"
    array[i] = x
    puts "#{array[i]} is in the array"
  end
end
p array

But actually you print the type the second time only when it is false, so don't do it. Also we don't need else in this case, because redo will throw us away from this iteration. And actually we can start from iteration 0 and then change for to times. But we will lose the puts "fn=" (I don't know, whether you really need it).

array = []
7.times do |i|
  puts "Iteration #{i} -------------------------"
  x = rand 10
  redo if array.include?(x).tap{ |type| puts "does array include #{x}? #{type ? "yes" : "no"}" }
  puts "x=#{x}, i=#{i}"
  array[i] = x
  puts "#{array[i]} is in the array"
end
p array
share|improve this answer
    
Outstanding. Thank you very much for showing me how my code was wrong in the first place, and then going farther by simplifying it in easy to understand steps. I learned much, and I greatly appreciate it. –  Kyzer Jan 15 '13 at 17:12
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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