# Making a Small Program Fit Ruby Standards [closed]

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

-

## closed as off-topic by Jamal♦Jan 31 '15 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 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).

-
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]
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

-
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 = 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

-