Sign up ×
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

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

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

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

while $i<10 do
    circs[$i] =, $point_A, $point_B)
    if $i%2==0 then
        circs[$i][:fill] = 'blue'
        circs[$i][:fill] = 'red'

share|improve this question

closed as off-topic by Jamal Jan 31 at 6:20

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the 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

3 Answers 3

up vote 5 down vote accepted

You should define arrays with [] not

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]

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|

    circs[i] =, $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'
        circs[i][:fill] = 'red'


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

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]

  def run
    canvas = => @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 =, point, point2)
      circle[:fill] = (index % 2) == 0 ? "red" : "blue"

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

if __FILE__ == $0
  example = => 1.5, :start_point => [750, 750])
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

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 = => 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, [x1, y1], [x2, y2], :fill => colors[i % 2])
  x1, y1 = x2, y2


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 = => 800, :height => 800).pack('fill' => 'both', 'expand' => true)

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

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

share|improve this answer

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