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 a high-school freshman who is kinda new to Ruby, and I am doing a small project on Ruby. One of the big things that I want to get out of this project is how to follow the "Ruby standards" that programmers should follow. Being as new as I am, I have no clue what I should/shouldn't do with this program. Can anybody tell me what I could do to improve it to fit the community's standards?

require 'tk'

$point_A = [0,0]
$point_B = [750,750]
$rate = 1.5
$i=0
circs=Array.new

def before_drawing()
    $point_A = []
    temp_a = $point_B[0]**1/$rate
    temp_b = $point_B[1]**1/$rate
    $point_A << temp_a
    $point_A << temp_b
end

def after_drawing()
   $point_B = []
   $point_B = $point_A
end

canvas = TkCanvas.new(:width=>800, :height=>800).pack('fill' => 'both', 'expand'=>true)

while $i<10 do
    before_drawing()
    circs[$i] = TkcOval.new(canvas, $point_A, $point_B)
    if $i%2==0 then
        circs[$i][:fill] = 'blue'
    else
        circs[$i][:fill] = 'red'
    end
    after_drawing()
    $i+=1
end

Tk.mainloop
share|improve this question
    
To reinforce a few points below, two spaces for indentation, no more no less; don't prefix your variables with $, you're making global variables needlessly. –  meagar Jan 6 '13 at 3:24
add comment

3 Answers 3

up vote 5 down vote accepted

You should define arrays with [] not Array.new

circs should be $circs in case you wrap your loop in some function.

Before drawing can be turned into this :

def before_drawing()
    temp_a = $point_B[0] ** 1 / $rate
    temp_b = $point_B[1] ** 1 / $rate
    $point_A = [temp_a, temp_b]
end

You should turn $i into a local variable for the loop. There is no need for it to be global.

Then replace the loop with upto.

0.upto(10) do |i|
    before_drawing()

    circs[i] = TkcOval.new(canvas, $point_A, $point_B)
    # As suggested using ternary operator
    # circs [i] [:fill] = i % 2 == 0 ? 'blue' : 'red'

    if i % 2 == 0 then
        circs[i][:fill] = 'blue'
    else
        circs[i][:fill] = 'red'
    end

    after_drawing()
end

You seem to use too much global variables. My advice would be to prefer local variables whenever you can (Like i in the loop). All your global variables can also be made local to the loop (or function in case you wrap the loop in some function).

share|improve this answer
    
Alrighty, that makes sense.. I had no idea that something like upto existed, and I was afraid to use x.times{} –  fr00ty_l00ps Sep 24 '12 at 14:07
    
I would recommend to change if i % 2 == 0 then circs[i][:fill] = 'blue' else circs[i][:fill] = 'red' end to circs[i][:fill] = (i % 2 == 0 ? 'blue' : 'red') or even circs[i][:fill] = %w{blue red}[i % 2]. –  Nakilon Jan 4 '13 at 20:07
    
Using the ternary operator isn't a bad idea. –  Aleksandar Jan 5 '13 at 10:38
add comment

Some notes:

  1. Use tabspace=2.

  2. Don't use global variables. When programming you should use functions in the same sense than you do in maths. That's a real function: f(x, y) = x + y, note that it takes arguments and returns some output (no globals, no states, no updates to variables outside the function).

  3. Ruby is a OOP language, so we usually define a class (or module) to contain our code.

  4. Don't overuse statements, use expressions. This code uses statements: x = []; x << 1; x << 2, this one uses expressions: x = [1, 2].

  5. You are writing a loop where the output is the input of the next iteration. That can be written with Enumerable#inject (this method is somewhat difficult to grasp at first, study the docs carefully).

A more idiomatic Ruby approach would be:

require 'tk'

class Example
  def initialize(options = {}) 
    @rate = options[:rate] || 1.5
    @start_point = options[:start_point] || [750, 750]
    @canvas_size = options[:canvas_size] || [800, 800]
  end

  def run
    canvas = TkCanvas.new(:width => @canvas_size[0], :height => @canvas_size[1])
    canvas.pack('fill' => 'both', 'expand' => true)

    1.upto(10).inject(@start_point) do |point, index|
      # get_next_point is a one-liner and could be written here,
      # but let's show how to use arguments to call functions/methods.
      point2 = get_next_point(point, @rate)
      circle = TkcOval.new(canvas, point, point2)
      circle[:fill] = (index % 2) == 0 ? "red" : "blue"
      point2
    end
    Tk.mainloop
  end

  def get_next_point(point, rate)
    [point[0] / rate, point[1] / rate]
  end
end

if __FILE__ == $0
  example = Example.new(:rate => 1.5, :start_point => [750, 750])
  example.run
end
share|improve this answer
    
Ruby may be OOP, but it's also a scripting language designed for building small purpose-built scripts. There is no need to shoe-horn in a class definition for such a small program, but if this is meant to be the foundation of a larger app then it's definitely a good idea. –  meagar Jan 6 '13 at 3:18
    
@meagar, I agree, but as you say I tried to show good practices for medium/large scripts. –  tokland Jan 6 '13 at 9:52
add comment

This is slightly late, but I feel like the existing answers are somewhat over-wrought.

A few points:

  • 10.times, as others have mentioned
  • You're not using ** correctly: x ** 1 / rate is the same as (x ** 1) / rate, and x ** 1 equals x. So, things are cleaned up right away by replacing ** 1 / rate with / rate
  • You can pass :fill directly to the constructor of TkcOval, meaning you don't need to store your circles at all
  • If you do need to store the circles, you can use circles = (0..9).map { |i| ...} instead of 10.times and return the circle from the block
  • You can compute color on one line using the ternary operator, or, better yet, store the colors in an array (colors = %w(blue red)) which can ban indexed by i % 2
  • Because we're dealing with maths, I prefer to store the points outside an array as (x1, y1) and (x2, y2). I think this makes things clearer than using arrays. Clearer still would be using points with .x and .y members like p1.x, p1.y, but that isn't supported
  • There is no need for your before/after methods, because they're doing next to nothing. They should be written as single lines of code.

Here's the results, about 10 lines of extremely concise and idiomatic code:

require 'tk'

canvas = TkCanvas.new(:width => 800, :height => 800).pack('fill' => 'both', 'expand' => true)

x1, y1, rate = 750, 750, 1.5

colors = %w(blue red)

10.times do |i|
  x2, y2 = x1 / rate, y1 / rate
  TkcOval.new(canvas, [x1, y1], [x2, y2], :fill => colors[i % 2])
  x1, y1 = x2, y2
end

Tk.mainloop

Note that we could get even shorter and ditch x2/y2, but I feel this starts to verge on code-golf rather than simply writing concise code:

require 'tk'

canvas = TkCanvas.new(:width => 800, :height => 800).pack('fill' => 'both', 'expand' => true)

x, y, colors, rate = 750, 750, %w(blue red), 1.5

10.times do |i|
  TkcOval.new(canvas, [x, y], [x /= rate, y /= rate], :fill => colors[i % 2])
end

Tk.mainloop
share|improve this answer
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.