Code Review of Ozmozr by Jamis Buck and Marcel Molina
The review is actually less painful than I would have thought. They are looking at parts of the code and giving specific feedback on code blocks. Here's a quick summary:
'and' and 'or' are different than '||' and '&&'. The second returns boolean values. The first returns strings. Avoid using 'and' and 'or'.
When dealing with errors. Try to use exceptions instead of booleans and procedures.
Putting models in the application.rb file is depricated and probably a bad idea anyway. - As an excuse, we never liked that, but it was required by the login engine which we have gotten rid of.
Empower your models. Include as much data validation and manipulation in the model as possible.
How to deal with business requirements that span models? Create a model that spans the logic.
Keep your controllers "skinny". If your view doesn't require any code do this:
def about
render
end
That lets you sort of self document your code.
Use exceptions. Nested ifs are ugly and if elsif is bad. In the code proceed with the assumption that you were successful and catch problems in exceptions.
Extract stuff from views into helpers. ie The views have long lines that look like this: "
they should look like this:
<%= tab 'popular' %>
def tab(name, options={})
lang = _(name)
s = "li>"
s << "class='current'" if @current_tab == options[:name] || name.downcase
s << ">"
s << link_to(lang, options[:url] || send("#{name.downcase}_path"))
s << "
end
Instead of this: I talked to Jamis a bit about Rails. A couple of questions I had: How do you test your rss, xml etc if you use responds_to? How do you share code and manage it between applications? They are going to send us their feedback which we will take to heart.
<% if @editable %>
<% end %>
Do this:
<% editable do %>
stuff
<% end %>
In the helper add this:
def editable(&block)
concat(block.call, block.binding) if @editable
end
which also lets you do stuff like this:
def editable(&block)
concat("
concat(block.call, block.binding) if @editable
end
set :format => 'xml' in your get in the test.
Use plugins. They use propset when in development, but do plugin install when they deploy so that the plugin version is locked.
-
Chris Moss
Justin Ball is a software consultant and entrepreneur with a passion for Ruby. He evolved from a C++ and .Net monkey into a python programmer and finally found Ruby. In the rare moments when he isn't writing code, talking about code or measuring his code productivity in profanity per hour, you can find him on his bike in the mountains or on the roads surrounding Cache Valley. 
