Ruby OOP convention/layout

Question

I am new to programming and I have just written my first program. It takes files from an SD card and copies then into a new named folder and has the option to name the files themselves or simply move them into the named folder.

As I am new I am unsure of the correct convention in terms of layout or structure of a program. Would a 'real' program look like this and how should it be structured?

Dir.chdir '/home/moot/pickaxe/pics' #replace this with destination directory
$pic_names = Dir['/media/moot/**/*.{CR2,cr2,JPG,jpg}'] #replace this with sd card directory
def folder
 puts "Enter the folder name"
 while $folder_name = gets.chomp
 if File.exist?("#{$folder_name}")
 puts "This folder already exists, choose a unique name"
 else
 require 'fileutils'
 FileUtils::mkdir_p "/home/moot/pickaxe/pics/#{$folder_name}"
 break
 end 
 end
end
def files
 puts "Do you want to name the files? Answer y/n"
 while question = gets.chomp
 if question == "y"
 puts "What do you want to call them?"
 $file_name = gets.chomp
 print "Downloading #{$pic_names.length} Files: "
 pic_number = 1
 $pic_names.each do |name|
 print 'ˁ ̊ᴥ ̊ˀ '
 new_name = if pic_number < 10
 "#{$file_name} 0#{pic_number} .CR2"
 else
 "#{$file_name} #{pic_number} .CR2"
 end
 require 'fileutils'
 include FileUtils
 cp(name, new_name) 
 pic_number = pic_number + 1
 end
 break
 elsif question == "n"
 print "Downloading #{$pic_names.length} Files: "
 $pic_names.each do |name|
 print 'ˁ ̊ᴥ ̊ˀ '
 new_dir = "/home/moot/pickaxe/pics/#{$folder_name}"
 require 'fileutils'
 include FileUtils
 cp(name, new_dir)
 end
 break
 else 
 puts "Please answer y/n"
 end
 end
end 
folder
Dir.chdir "/home/moot/pickaxe/pics/#{$folder_name}"
files
puts "Move Complete."

• Caridorc: Please state only code-purpose in the title, like "Sd card files copying"

Answer 1

I started to write a detailed review, but in middle of it I realised that best advise I could give you looking at this code is to pick up some good Ruby book and work through it carefully, paying attention to how examples are formed. You seem to understand basics of how programming works, but your code lacks proper usage of many basic concepts, like arguments, returning, modules etc. Explaining them in detail here seems pointless, it's not a place for Ruby tutorials. You should focus on that basics before you move to object oriented design/conventions.

Still, I'll post what I already wrote as advices for you.

---

I don't really see any need to use OO structure in this code. It definately could still be written better though.

Indentation

Your way of indenting makes code very hard to read. Compare:

 $pic_names.each do |name|
 print 'ˁ ̊ᴥ ̊ˀ '
 new_name = if pic_number < 10
 "#{$file_name} 0#{pic_number} .CR2"
else
 "#{$file_name} #{pic_number} .CR2"
end
require 'fileutils'
include FileUtils
cp(name, new_name) 
pic_number = pic_number + 1
end

and:

$pic_names.each do |name|
 print 'ˁ ̊ᴥ ̊ˀ '
 new_name = 
 if pic_number < 10
 "#{$file_name} 0#{pic_number} .CR2"
 else
 "#{$file_name} #{pic_number} .CR2"
 end
 require 'fileutils'
 include FileUtils
 cp(name, new_name) 
 pic_number = pic_number + 1
end

Ruby style guide contains some usefull examples on how you should indent your code to make it nice to read. Generally every block means 1 level of indentation (2 spaces).

Require

require 'fileutils'
include FileUtils

You only need to require 'fileutils' once, at top of your file. include also doesn't need to be reapated, instead generally it should be at top of a module.

Avoid globals

Instead of putting result of your methods in global variables, return from them. I.e:

def folder
 puts "Enter the folder name"
 while folder_name = gets.chomp
 if File.exist?("#{$folder_name}")
 puts "This folder already exists, choose a unique name"
 else
 require 'fileutils'
 FileUtils::mkdir_p "/home/moot/pickaxe/pics/#{$folder_name}"
 return folder_name
 end 
 end
end

And than:

folder_name = folder
Dir.chdir "/home/moot/pickaxe/pics/#{folder_name}"

This is much more flexible.

Answer 2

@Borsunho made some points on functional improvements, so I'll address some style points.

File.exist?("#{$folder_name}") - Folder name is already a string, so you don't need to wrap it in a string and then interpolate it. Just use File.exist?(folder_name).

---

if question == "y"
 ...
 $pic_names.each do |name|
 print 'ˁ ̊ᴥ ̊ˀ '
 ...
else
 ...
end

This doesn't even look like it works - where's the end to the each block? The problem is that your indentation is messy. Every time you change scope you should indent a level further, and when you return to a previous scope you should unindent.

Or to put it another way, when you start an each block, you should start indenting one level further until you end that each block:

line_1
array.each do |item|
 item.line_2
 item.line_3
 item.line_4
end
line_5

Lines 2, 3 and 4 are indented further than 1 and 5 because they belong to a deeper scope.

Doing nothing but re-indenting your code gives us this:

def files
 puts "Do you want to name the files? Answer y/n"
 while question = gets.chomp
 if question == "y"
 puts "What do you want to call them?"
 $file_name = gets.chomp
 print "Downloading #{$pic_names.length} Files: "
 pic_number = 1
 $pic_names.each do |name|
 print 'ˁ ̊ᴥ ̊ˀ '
 new_name = if pic_number < 10
 "#{$file_name} 0#{pic_number} .CR2"
 else
 "#{$file_name} #{pic_number} .CR2"
 end
 require 'fileutils'
 include FileUtils
 cp(name, new_name) 
 pic_number = pic_number + 1
 end
 break
 elsif question == "n"
 print "Downloading #{$pic_names.length} Files: "
 $pic_names.each do |name|
 print 'ˁ ̊ᴥ ̊ˀ '
 new_dir = "/home/moot/pickaxe/pics/#{$folder_name}"
 require 'fileutils'
 include FileUtils
 cp(name, new_dir)
 end
 break
 else 
 puts "Please answer y/n"
 end
 end
end 

You can see how our indents move backwards and forwards by 1 every time we hit a scope changing keyword, like do, end, and if. If you were to draw a ling hugging the left side of our code, it would be smooth as it moved back and forth between levels, with no large jumps.

Now we can easily see that the code is split in to three sections, based on the answer to the question, and it will be easier to understand and improve the code.

---

You should use puts to output to the console, not print, unless you actually want the output to stay on the same line (useful for, say, a progress bar or a question waiting for an answer). Using puts automatically appends a new line if there isn't one, so each line of output gets its own line in the console and you can read it more easily.

---

Avoid using elsif. In this case it's not too bad, but imagine in the future you want people to also be able to answer "q" to quit the process and "a" to automatically rename them and "c" to move them but keep a separate copy, etc. You'd need a separate condition in the elsif for each. Instead you should use a case statement:

puts "My Question (y/n/q/a/c):"
response = gets.chomp
case response
when "y"
 # yes code here
when "n"
 # no code here
when "q"
 # q code here
when "a"
 # a code here
when "c"
 # c code here
else
 puts "Please respond with y/n/q/c/a"
end

---

case statements also accept regexes in their conditions, so you could change those lines to read like

when /^y/
when /^n/

and now people can answer "yes", "yeah", "no", "nope", "y", "n" and still get the desired result. If you don't use a case statement you can still add this functionality in if conditions:

if (response =~ /^y/)
 # yes code
elsif ...

---

FileUtils::mkdir_p "/home/moot/pickaxe/pics/#{$folder_name}"

When you use a method that takes arguments, surround the arguments in parentheses unless it's a Kernel method (like puts, gets and print). That line should be:

FileUtils::mkdir_p("/home/moot/pickaxe/pics/#{$folder_name}")