I was recently turned down for a Rails developer position (I’m not
really a Rails developer, but it’s something I’ve been playing with in
my spare time, so I gave it a go).
Part of the technical screen was to take an old, junky Rails app, with
invalid views, old controllers etc. and smarten it up so be ‘better’
(the time allocated was less than an hour or so).
Unfortunately I just heard they rejected my submission, saying they were
‘unhappy with the standard of the code’.
I agree it’s not beautiful, but given the time limit, I thought I had
done an ok job. Clearly I was wrong. However they didn’t provide any
further feedback, and so I’m currently at a loss as to what I had done
wrong.
Their few comments mentioned that I had written unnecessary code and had
not stuck to Rails principles (which I focussed on above CSS, etc.).
I’m not sure what the app was really for, but I kept the intention much
the same, and tried to improve the implementation.
Because I’d like to learn from my mistakes - please take a look at the
attached Rails app. Please pick it apart. Please be as critical as you
can - anywhere where I’ve used Rails in a non-standard way, anywhere
I’ve written unnecessary code, anywhere I’ve been anything other than
the most exemplary Rails developer, please, please, let me know.
Currently I’m ignorant!
N.B. For size, I’ve only included the app folder.
Thanks.
Im not sure what state the app was in when you received it, but here’s a
few pointers.
Controller Code –
There’s a lot of code to rescuing from ActiveRecord::RecordNotFound
errors. There is an easier way to do this on a global scale in
application_controller. Check out a method called
rescue_action_in_public here →
In the index action Page.all was used. This is almost NEVER what you
want. ActiveRecord is amazing for finding and manipulating database
records, but really really bad with large amounts of data. I
accidentally used it once on an xml feed and rendered almost 400K
records… needless to say, that was a bad day.
Model Code –
The models are pretty thin, so not much to say. The email regex is the
only that stuck out. It’s not nearly enough for production. Rails has
a regex for email validation built in (i think). Check that out.
Views and Helpers –
I noticed both helper files were empty. When I first started with rails
I was super confused about how to use the helpers. The views for this
app seem pretty simple, but here’s a good rule of thumb. When you write
an if statement in your view, ask yourself, “could I write this in a
helper method?”. Or when you write a link that will change based on the
user’s state (login/logout, user role etc…), put it in a helper method.
They are life savers.
I think that in a perfect world, ANY view logic would be in a helper
method, but that’s a war that will wage for a long time.
The only other thing in the views that I noticed was the flash partial.
Rails’ flash idiom is global and really should be in the layouts folder,
or better yet, in application_helper. 
In conclusion, what I see is not that bad, but there’s definitely a few
things that show some unfamiliarity with Rails. Hope that helps.
Jon
Views and Helpers –
I noticed both helper files were empty. When I first started with rails
I was super confused about how to use the helpers. The views for this
app seem pretty simple, but here’s a good rule of thumb. When you write
an if statement in your view, ask yourself, “could I write this in a
helper method?”. Or when you write a link that will change based on the
user’s state (login/logout, user role etc…), put it in a helper method.
They are life savers.
I think that in a perfect world, ANY view logic would be in a helper
method, but that’s a war that will wage for a long time.
The only other thing in the views that I noticed was the flash partial.
Rails’ flash idiom is global and really should be in the layouts folder,
or better yet, in application_helper. 
In conclusion, what I see is not that bad, but there’s definitely a few
things that show some unfamiliarity with Rails. Hope that helps.
Jon
It’s not the only thing I agree with but Jon made a good point regarding
helper files. It’s too often that people do not take advantage of them.
Views should always have as little work to do as possible in an ideal
world. An example of using helper files is on my test site
(unfortunately on my local server). However, in it contains several
methods to assist the forms on the site. I constructed a CMS for the
site which has forms for manipulating the records in each table of
course. Placing actions in the form helper made it simple to integrate
code for my TinyMCE Editor, as well as giving my forms global access to
things like this editor, collection selects, document up-loaders and
image up-loaders.
See below for my example helper method for my TinyMCE Editor:
def tinymce_text_area(attribute)
@template.text_area(@object_name, attribute, :cols => nil, :rows
=> nil, :class => “mceEditor”)
Helpers are especially useful when you find yourself generating a lot of
applications that follow the same framework and you can simply emulate
the data from the helpers.
end