Ruby
Summarized using AI

Rails Code Review

by Jamis Buck and Marcel Molina

The video titled 'Rails Code Review' features speakers Jamis Buck and Marcel Molina discussing best practices for reviewing and improving Ruby on Rails applications during the MountainWest RubyConf 2007. The main focus of the discussion is on analyzing a live Rails application provided by a participant, emphasizing the importance of adhering to Ruby and Rails idioms.

Key points covered include:

- Controller Practices: The speakers address redundant patterns in the code, suggesting the use of 'unless' instead of 'if not' for clarity and idiomatic Ruby style.

- Method Refactoring: They recommend consolidating functionality within high-level methods, such as 'getcommunities', to maintain simplicity in controllers.

- Handling Data Types: The conversation includes the differences between 'to
i' and 'Integer', stressing the importance of explicit error handling to avoid potential bugs.

- Business Logic in Models: The discussion highlights the importance of keeping controllers slim and avoiding direct coercion of values in the controller, which should manage business logic instead.

- Polymorphism and Code Readability: The speakers advocate for using polymorphism to manage conditions and reduce nested 'if' statements, which often indicate design smells.

- Exception Handling: They suggest that using exceptions can lead to clearer error management compared to maintaining complex boolean conditions in procedural code.

- Testing and Flexibility: It is noted that clear relationships between controllers and models lead to better testing practices, promoting reusable code and clearer expectations of behavior.

- View Layer Abstraction: They recommend refactoring logic out of views into helpers to maintain clarity and follow DRY principles.

In conclusion, Buck and Molina emphasize that a well-structured Rails application involves succinct controllers, expressive models, and clear views, ultimately leading to better maintainability and clarity for both current and future developers. They advocate for adhering to Rails conventions to enhance code reuse and foster community improvement. The session closes with a call for attendees to engage with these principles in their coding practices.

00:00:07.560 All right. I just have a quick question. This is a Ruby conference and not a Rails conference, but I'm just curious how many here have played with Rails? How many do Rails programming professionally? This is almost a Rails conference, anyway. We've been asked to do a live code review of a real Rails application. I'd like to thank Joel for providing this application for us to look at. It's always kind of frightening to have a large number of people look over your code and have someone else go over it and point out what could be done better. We really appreciate that. Should we just hop into it? Yeah? All right, so let's first pull up the Community controller.
00:00:26.599 To tell you the truth, we're not entirely sure what this application even does because we couldn't run it — we didn't have PostgreSQL set up. So we're not looking at it from the perspective of how this application should have been written to do what it needs to do better. We're just examining it from the standpoint that certain snippets of code could probably be done differently to fit better with Ruby and Rails idioms. One thing that struck us is a pattern that shows up a lot where something is assigned. If not, the first nitpick is to use 'unless.' This is a more Ruby-style idiom. Instead of saying 'if not,' you can say 'unless.' Even better, since we're just checking to see if the current tab is null or not, we can eliminate that whole second line.
00:01:04.519 Another area we're looking at is the check for the offset. If it isn't nil, there's a call to 'to_i.' Otherwise, it sets it to zero. Fortunately, 'nil.to_i' is zero, which simplifies that code. This one is a bit hairier to condense into one line, and you don't always want to go for one-liners anyway. Ultimately, all of this should really be pulled into the 'get_communities' method on the Community controller. You would assign all the necessary parameters — like 'params[:offset]' and 'params[:limit]' — and then let the 'communities' method determine what to do in case they are nil or whatever. There's a part in Ken Beck's 'Smalltalk Best Practice Patterns' where he gives a tip on how to approach refactoring your methods and determining where to decompose methods. His tip is that all things happening in a given method should be at the same level of abstraction.
00:02:19.720 In this case, we have the notion of getting communities, which is the higher-level operation, and then we have all these little implementation details about how to get those communities, like setting the offset and the limit. In the controller, all you want are the higher-level symbols of what's happening; thus, it should really just be about getting communities. You want to make the data available for that so you're not mixing those two aspects and context-switching between lower and higher-level concepts.
00:03:08.560 Does anybody know the difference between 'to_i' and 'Integer'? In the context of what we were saying, when we were discussing handling nil offsets, using 'to_i' would return zero when the passed string is empty, while using 'Integer' would raise an exception if that string can't genuinely represent a number. In cases where it matters that you're strictly expecting either nil or an integer, using 'Integer' is the preferable choice because an exception is raised rather than just defaulting to zero. This difference can lead to subtle bugs if you're not careful.
00:04:21.919 In general, when handling business logic, it probably depends on what the rules are in your application. I would argue one shouldn't be coercing values in the controller directly; that logic should be implemented in the model. The story here is that this code structure could be refactored in a more logical way, but given its current state, there are certainly better alternatives.
00:05:14.160 In fact, there's a lot of value in keeping controllers slim. When you have a number of nested 'if' statements, it may indicate a failure to utilize polymorphism, which can signal a design smell. You can express behaviors better by encapsulating concerns in object behavior rather than piling more procedural code with all the 'if' statements.
00:05:59.520 Returning to the code, when dealing with booleans and error handling, it's worth considering whether exceptions could handle this logic better. When there are multiple conditions involved interacting with booleans, it obscures higher-level understanding of what is going on. This higher-level operation should ideally determine whether an issue arises and raise an appropriate exception instead of maintaining silhouette states within procedural code.
00:07:02.039 The best practices for organizing Rails applications emphasize designing robust models because that leads to thin controllers. I find that a controller action should contain a minimal number of lines — ideally five at most — and serve as a clear declaration of intent rather than being cluttered with implementation details. For example, an action should say 'banish user if not allowed,' which provides clarity rather than digging through various method calls to deduce what that logically means.
00:08:33.360 It's essential to understand that when dealing with models, you can also encapsulate multiple model interactions in a 'super model' that doesn't have to inherit from Active Record if that simplifies the logic. By abstracting complex relationships into a proper encapsulated model, your controller can stay simple, allowing for clear API calls and concise testing methods.
00:09:51.840 Additionally, if the structure of your controllers is coherent — with clear design principles established — testing becomes streamlined. By shifting logic into models, one can leverage unit tests which are generally more flexible than large functional tests that can be brittle. Your testing leads to clearer expectations of behavior, which allows for higher quality code.
00:11:03.959 It's vital during development to hold a cohesive relationship between controllers and models, allowing for easily extendible and reusable functionalities. This leads to the realization that well-structured code not only benefits usability but also maintains integrity as further developments occur.
00:12:02.200 Concerning view layer interaction, particularly when employing ERB, you should strive to maintain the clarity of intent. Instead of performing intricate logic within your views, you'd better serve the application by isolating responsibility within helpers. Creating blocks that encapsulate logical conditions is a straightforward way to elevate clarity while also optimizing for DRY practices.
00:12:50.400 The function to generate conditional helpers retains the benefits of abstraction while offering a clean solution to avoid cluttering your templates with repeated input. By including basic wrappers for ubiquitous structures, the readability of your views is greatly enhanced, and any conditional display logic becomes manageable.
00:14:16.280 To summarize, effective application structure revolves around succinct controllers, expressive models, and evaluative views. This fusion is fundamental to optimizing Rails app development for long-term clarity and maintainability. By continually applying these principles throughout a project's lifecycle, you set a solid foundation for both current and future developers.
00:15:41.480 Your groundwork in ensuring adherence to Rails conventions bolsters the advantages present in a robust framework. Substantial opportunities for code reuse stem from this coherent approach and serve the community as we endeavor to share knowledge and improvement practices.
00:17:09.600 As we conclude this discussion, I'd like to open the floor for any final thoughts or questions. Remember, as you implement these evolving practices and philosophies, working fluidly across your application's boundaries while adhering to these principles will always lead toward better coding experiences.
Explore all talks recorded at MountainWest RubyConf 2007
+13