Drawing colored circles using Ruby and Tk

Question

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

Answer 1

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 many 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).

Answer 2

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

Answer 3

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

Furthermore, note some style issues:

• two spaces for indentation, no more no less
• don't prefix your variables with $, you're making global variables needlessly.