YourTradeBase Code Test
This is a very simple rails application that allows users to manage a list of books. The app mainly contains rails auto generated scaffold code, however some code has been added to allow books to be managed via the users show page. This code has not been written very well! We would like you to refactor the code in order to make the code more maintainable and to be written in a style more suited to a well written rails application. There has also been a complaint from some end users that the site is slow to respond occasionally when creating new users. The application has some basic Rspec tests that can be run using the `rspec` command.
Please spend no more than 2 hours on the exercise excluding any time it takes to install the required software such as ruby, gems etc.
We would like you to keep a record of your thought process and actions taken in order to prepare the application and refactor the code, please be as detailed as possible. You should include your explanations as to why the code you have found is badly written and how you think your refactored code will improve the codebase.
We are interested mainly in your thought process through the exercise, don't worry if you don't manage to complete the refactoring in the time allowed, you may add any suggestions for further refactoring you would wish to carry out if you had more time.
Your taske are:
Fork this git repository to your own github account then clone it
Install the correct ruby version for the project using a ruby version manager of your choice
Install the required gems for the project using bundler
Setup the applications database
Identify code to be refactored
Create appropriate commits for your refactored code
Create 1 commit with your notes
Push the commits to your github repository
Provide us with the url of your github fork of the app
Please add your notes below.
I am about to undertake a 2-hour long code test for YourTradeBase. These are my thoughts and actions as I'm doing it. I'll try to keep it entertaining, funny, a stream-of-consciousness kind of thing (which is an illusion, at least somewhat).
The first thing I do is open a new tmux session for the project. I have cloned the repository before reading the instructions, but it seems I should fork the repository to my own account first. This is better, as it will allow me to create issues for each task, just to simulate a truer working environment.
I have now forked the project and cloned it. I already have Ruby installed, but `.ruby-version` is saying the project uses `2.1.2`, so I'm going to install it using `rbenv`. This might take a while, so while that's happening, I'm going to take the opportunity to talk about my preferred development workflow.
I use tmux as my multiplexer, Vim as my text editor and zsh as my shell. I keep the configuration for all of these in a git repository with a very simple install script, so I have mostly the same environment at home and work, and even on my personal VPS. Vim seems to have trouble with navigating long lines, as it lags with long lines (not an issue in code, but not great for prose). I'll try to have lots of line breaks and compress them together with MacVim later (which doesn't suffer so much with this problem).
Okay, the right Ruby version has finished installing. Time to bundle. I select the “Currently on Repeat” playlist in iTunes and wait for bundle to complete.
Bundle has completed and I've setup the database. I quickly glance through the migrations, and it look simple enough: we have users with names and emails, and they have books with titles.
The first thing I notice, reading through the README again is that users show page doesn't sound like the place to manage books. Let's just peek at the view. Good place to start the clock, I think. 10:10 am.
Start the Rails server. I've decided to abandon the idea of creating GitHub issues, it doesn't sound worth it for this simple test. I'd normally create a new branch for a logical unit of work, but since I know for this task, I will only ever have one, I will ignore that.
Run the tests, 3 examples, no failures. Adding a book displays the book title on the user show page? Hmm, these are feature specs that'll have to be changed. I don't think it's a sin to display the book title on the user's profile page, but the book form should definitely not be on there (and only the title, if the book starts getting more fields, they probably won't all show up on the user profile view). I'll leave the assertion, but I'll change the `before` block. I'll do this for all the tests. Huh, one of these assertions is repeated, according to the description of the test, but the code tells a different story. Fix it! I don't mind not getting the test perfect, I can change it if necessary, as long as the intent of the test remains, the nitty gritty of the code of the test doesn't matter (but the assertion descriptions are important). For the deleting a book test, it might be better to expect subject to change page from having the content to not having the content. I'm not sure how to do that for an assertion, but that is my thought. I'll check later.
Now that's done, I'm going to run the `scaffold_controller` generator, to generate the views and controller for books. Normally, I would write them myself (or if I was lying, write custom generators), but since this is a simple test with no existing theme, I don't mind generating them. The existing code is the same.
I add the default RESTful book routes and while I'm there, move the root route declaration to the top of the file. Seems like the appropriate thing to start with. I open up the users controller and remove the book-related actions. I copy and paste the `app/views/users/_book_form.html.erb` into `app/view/books/_form.html.erb` and merge the two forms in the most appropriate way (making my own amendments to both, such as changing the user field into a select, as opposed to a hidden field). I add the required attributes to the strong params. Huh, looks like `#require` doesn't take more than one argument. A quick search reveals some basic of strong parameters that I missed in my first Rails 4 venture (I don't use Rails 4 often). I fix that, resubmit the form and see that the books index page doesn't show the books. Gotta add a test case for that, watch them fail and fix it.
Fast forward (for you guys), and the meat of the work is done. I've added 2 commits (I'd normally have more, but these are such routine tasks). Now to iron out the edges, add JSON views for the books as well, maybe have a `/users/:id/books/new` view to create books and go back to a hidden `user_id` field. I'd definitely like to add service objects to deal with the processes within the domain, not because the application needs it at this stage, but because I can tell it's going to become a popular app, and will need to be well-maintained. Before that, though, let me just add some JSON views to match our convention that we've established with users. Ah, missed it the first time, looks like we already have some, but they just need some adjustments. I'll expose the book title to our API, as well as the user name (owner, author?). I'll expose book_ids for users to our API. This is less convenient than exposing all the book details along with the user, but I think it's much less strain on our servers. I'm not going to go to the effort to benchmark it, because it's such a small decision, so I'm going to go with my gut feeling here.
Now that that's done, I'm going to run all the specs again. Huh, turns out the user specs were failing because of a missing `require` statement. Add it in, run again, it all works. It's describing the process of creating a user. Since these were generated using Rails generators and I trust their test suite to catch something this critical, I'm not going to test all the default stuff.
I'd like to start making the users create action faster. I'm going to branch off now, as this looks like a new logical unit of work. It looks like we send an email to the admin to let them know a new user has registered. If I had no idea why it was slow, I'd start benchmarking with tools like `rack-mini_profiler`, New Relic or what we have available (whatever seems appropriate for the specific section of code I'm benchmarking. I know from experience, though, that moving the mailer to a background queue is a good idea, so I'm going to try that simple fix first. It's not that important that this part be fast, so I'm not going to write a test for it (otherwise, I'd write performance tests for everything, which I've had no experience writing, actually).
Looking at the Gemfile, it doesn't look like we have any existing solutions to this, like Sidekiq. Sidekiq happens to be the one I have experience with, and is very popular, but I'll spend a few minutes making sure I made the right choice. With a bit of research, I find out that Sidekiq is the favoured choice for good reasons, is well-maintained, and is well-suited to our purposes. I also happen to be familiar with it, so I go for it. As I'm about to move the AdminMailer line from the users controller to a worker, I notice it says, “`deliver_now`”, implying there's a `#deliver_later`. I quickly check it out, and yes, there is. We have ActiveJob now. Nice! I rebase and remove the single commit that introduced Sidekiq.
As my time runs out, I ponder what I might want to do given more time. I'd like to make some quality of life changes to the application, such as linking to the book on the user's profile page, where it lists the book titles. I don't know why I didn't do that the first time. Some more tests would be nice, but I'm not haunted by the lack of it in this case, just because there was nothing complex or important. I would have liked my thoughts to be transferred to this monologue without me having to manually write it down. Looking back, it seems like I also forgot to namespace book creation under users (routes, not views, controller actions, etc.). Finally, now would be the opportunity to extract controller actions into service objects. The user create emails the admin, so that seems like a good place to start. Alas, I have no time left.