FeedBack Please.

[TokenHerbz]

Being Very new to Ruby I'm unsure of the best ways to go about using certain loops and when i should compact code, etc etc..

I'm going to upload a picture which contains a challenge I've done, along with my code for you guys to give me feed back on.

Any tips or helpfulness would be wonderful, I want to learn the correct way and books/etc only take me so far, skilled programmers opinions are another valued source of learning.

First: the CHALLENGE: http://i.imgur.com/ieyDP.jpg

Second: The CODE (please excuse the really long named variables, I got carried away!)

Ruby:

number1 = 0 #numbers the user will enter number2 = 0 between_number_max_cycle_length = 0 #the value of the biggest cycle between number1 and number2 #will attempt to make it break down the number that is entered in, and inside it #will count the number of cycles it goes threw, And this value will be returned #to the number_cyle for the users number. def break_down_number (number_,cycle_length_) #cycle_length is for users number (SET TO ZERO)   if number_ % 2 == 0 #number is even     number_ = number_ / 2 #divide it by 2   else #odd number     number_ = (number_ * 3) + 1 #times by 3 and add 1   end   cycle_length_ += 1 #add 1 to length of cycle each time we break down the number   if number_ != 1 #if number not broken down to 1 yet then     break_down_number(number_,cycle_length_) #algorithm continues   else     return (cycle_length_ + 1) #add 1 to cycle_length to include the last value   end end #gets users numbers in range of 1 to 999,999 until number1 <= 999_999 and number2 <= 999_999 and number1 >= 1 and number2 >= 1   puts "Please enter in 2 numbers from 1 to 999,999."   number1 = gets.chomp.to_i   number2 = gets.chomp.to_i #make the string INTS   puts "" end #we go from number 1 up to number 2 and check each number for its cycle length #if its larger then the MAX_length it is updated and kept... number1.upto(number2) {|x|   if break_down_number(x,0) > between_number_max_cycle_length     between_number_max_cycle_length = break_down_number(x,0)   end } #the output displayed exactly as challenge asks for... puts "#{number1} #{number2} #{between_number_max_cycle_length}"

[Insectoid]

Your break_down_number method can be reduced a fair bit.

Ruby:

def break_down_number(number_, cycle_length)     number_ = number_%2 ? number_*3+1 : number/2     cycle_length += 1     if number != 1         break_down_number(number_, cycle_length_)     end     cycle_length_ += 1 end

I may have a syntax error or two, I didn't bother running this.

Whenever you have something like

code:

if whatever then    var = something else     var = something_else end if

It's (in my opinion) better to use the ? : structure, ie

code:

var = whatever ? something : something_else

[jcollins1991]

I'd suggest tying to write it with a while loop, I'm not sure how efficient Ruby is with tail recursion.

[Tony]

code:

if number != 1         break_down_number(number_, cycle_length_)     end

can also be written as

code:

break_down_number(number_, cycle_length_) unless number == 1

[wtd]

Why not implement this as a method on the Fixnum class?

code:

class Fixnum    def algorithm       c = 1       n = self       while n != 1          if n.even?             n /= 2          else             n = n * 3 + 1          end          c += 1       end       c    end end

[Tony]

Now that I have some more time to look over this:

- there is no need to initialize variables
- the typical convention for internal variables is _name instead of name_
- return doesn't need brackets for expressions
- the question states that the input would be valid, so the check isn't required, but if you must -- checking against ranges works too

code:

>> (1..999_999).include?(42) => true

- from the performance standpoint, you are evaluating [break_down_number(x,0)] twice (once in a conditional and once more to actually save the value). Also, the same x will always produce the same sequence, so a lot of work can be shortcircuited with dynamic programming.

My solution (because reading code is essential for writing code):

Ruby:

@results = {1 => 1} def max_cycle_for(n)    while(@results[n].nil?)       @results[n] = max_cycle_for(n % 2 == 0 ? n / 2 : n * 3 + 1) + 1    end    @results[n] end num_1 = gets.to_i num_2 = gets.to_i puts "#{num_1} #{num_2} #{(num_1..num_2).collect{|a| max_cycle_for(a)}.max}"

[TokenHerbz]

Thank you everyone for the feed back.

Insectoid : I like how to minimize those If statements and will be doing that more. I wonder though, does it work with elsif too? or should I use a case? Or case is best suited for MULTIPLE ELSEIF's only? What I've read on case is that it's really helpful or best used in really really large if structures but I'm sure it can be used for small ones to. But it doesn't say what the Ideal Situation would be to use them is... While learning ruby and coding I want to learn the correct way so I don't have poor programming habits later.

wtd : I've not even read on the ruby class's yet, but I will take a look in due time. I feel I have way to much of the ruby basics to learn still. thanks tho. I'm curious though, Do you add that to the actual FIXNUM class?... for future use in all programs?

Tony : Awesomeness. I'll be going threw my ruby reads because I have no idea how some of that code works. 1 quick example would be

code:

@results (1 => 1)

[/color]

[Insectoid]

That conditional structure only supports binary conditionals.

It's basically,
<condition> ? <run if condition is true> : <run if false>

So, If/else structures only.

I generally don't use case unless there's 3 or more unrelated values or if I need to run a different function based on input. An example of this is a basic calculater- there isn't any efficient way (I think) to assign *, /, + and - characters to their operations without a case or large, unwieldy if.

Also, I'm not quite sure but I think Ruby evaluates zero as false and anything else as true, so instead of <var> == 0 you can just do !<var>.

[jcollins1991]

Insectoid @ Thu Dec 09, 2010 10:46 am wrote:

That conditional structure only supports binary conditionals.

It's basically,
<condition> ? <run if condition is true> : <run if false>

So, If/else structures only.

I generally don't use case unless there's 3 or more unrelated values or if I need to run a different function based on input. An example of this is a basic calculater- there isn't any efficient way (I think) to assign *, /, + and - characters to their operations without a case or large, unwieldy if.

Also, I'm not quite sure but I think Ruby evaluates zero as false and anything else as true, so instead of <var> == 0 you can just do !<var>.

The only false things in Ruby are 'nil' and 'false'

EDIT: Though, you can just do 0.zero? to get the falseness of 0 in other languages

[wtd]

Yes. Zero is a perfectly cromulent number, and in no way false.

[Tony]

results is a Hash -- http://ruby-doc.org/core/classes/Hash.html

it stores all the values that have already been calculated, to be used for future computations. This idea is the core of Dynamic Programming (bad naming, it's more of "programming while having a table of stored values"). Try to see what's stored inside after each call to max_cycle_for.

{1 => 1} is the base base. We know that the max cycle for n=1 is 1, per definition, so we initialize to this pair so that recursive loops terminate.

[wtd]

Dynamic programming is about doing as little work as possible by remembering where you've been before.

[TokenHerbz]

Another Programming Challenge Needing Feedback! : I feel there's improvements, But I'll let you, the experts decide.

Here is the challenge: http://i.imgur.com/SZ8Dh.jpg
However, I went ahead and changed a few things myself: I made it so you can adjust the grid size inside the code, And adjust the MINE PERCENTAGE for that grid. The mines are also Randomly generated.

Please, be very critical and show me how how sloppy this is
I put if's inside the loops and the bottom if's i couldn't figure out how to compact that any way. I gave it a lot of thought and this is the best i could come up with.

Ruby:

ROWS, COLUMNS = 5, 5 #change to edit the MINESWEEPER GRID SIZE MINE_PERCENTAGE = 0.10 # 0.10 = 10%  // 0.50 = 50% (Make sure its ZERO . PERCENT mine_tile = Array.new(ROWS) {Array.new(COLUMNS){"."}} #2D array with no mines MINES = (MINE_PERCENTAGE * (ROWS * COLUMNS)).round #rounded amount of mines in grid MINES.times{|mines| mine_tile[rand(ROWS)][rand(COLUMNS)] = "*"} #init random mines #print out the grid with the mines and spaces ROWS.times{|rows| COLUMNS.times{|columns|print mine_tile[rows][columns]}   puts} puts #solves the grid using numbers representing the amount of mines near it by # going threw the rows, each row it goes threw the columns (2D array). #it finds the mines "*" and then it runs a BOX around the mine making it number 1 # for each time that tile is hit.  I added if's to make sure the BOX check wont # go outside the array range. ROWS.times {|rows| COLUMNS.times{|columns| if mine_tile[rows][columns] == "*"       ((rows - 1) <= 0 ? 0 : (rows - 1)).upto((rows + 1) >= (ROWS - 1) ? (ROWS - 1) : (rows + 1)){|solve_row|         ((columns - 1) <= 0 ? 0 : (columns - 1)).upto((columns + 1) >= (COLUMNS - 1) ? (COLUMNS - 1) : (columns + 1)){|solve_column|           if mine_tile[solve_row][solve_column] != "*"             if mine_tile[solve_row][solve_column] == "."               mine_tile[solve_row][solve_column] = 1             else               mine_tile[solve_row][solve_column] += 1             end           end         }       }     end   } } #prints the grid with mines after its SOLVED ROWS.times{|rows| COLUMNS.times{|columns|print mine_tile[rows][columns]}   puts}

[Tony]

code:

MINES.times{|mines| mine_tile[rand(ROWS)][rand(COLUMNS)] = "*"} #init random mines

you don't need |mines| if you're not going to use it.

code:

3.times {puts "ruby"}

Also, strictly speaking, there is a chance of filling the same location with a mine twice. Meaning that at 100% MINE_PERCENTAGE, there could still be empty spaces.

code:

ROWS.times{|rows| COLUMNS.times{|columns|

sometimes it's easier to read do-end blocks

code:

ROWS.times do |row|    COLUMNS.times do |column|       print ...    end end

code:

((rows - 1) <= 0 ? 0 : (rows - 1)).upto((rows + 1) >= (ROWS - 1) ? (ROWS - 1) : (rows + 1))

One trick to avoid such checks is to have a buffer around your array, so that you could safely go out of bounds. (Another would be to treat edges in a special way and use those as a buffer to safely work with everything else).

[TokenHerbz]

so have the array go from -1 to 5.

0,1,2,3,4 being the used array elements.
-1, 5 being NIL?

i'll try it out, i like that idea.