3 ms·

I think many people react negatively to this article because they gloss over the idea that this is a refactoring. When coding you always make the choice betwee

by tinco 14y ago

I think many people react negatively to this article because they gloss over the idea that this is a refactoring. When coding you always make the choice between abstraction and readability. If you look at the patterns from GoF, the downside of almost every pattern is that it reduces readability for the simple cases. Thus, when you write your controllers and your models in my opinion you should start out doing it 'the rails way' which is having your business logic in your models, and the controlling logic in your controllers. Then when your app is starting to feel clunky, your controllers are gaining weight and your models have too many responsibilities, that is the point where you start refactoring. And that is also where you decide wether the logic belongs into a concern, or a policy, or whatever fancy pattern is hip right now and fits there. (But you really should make an effort to make clear where logic ends up for someone new to your code, so be consistent!) I think rapid development is one of the key features that made Rails big, don't let these concerns that big apps have drag you down in the beginning.

jeremyjh 14y ago
People will spew all these separate classes from the very beginning in the name of "test-ability".
- alisnic 14y ago
  I don't understand what is the problem of having a lot of small classes? They are there when you need them, and they get out of the way you don't, and I think that's the most important thing.
  - fbuilesv 14y ago
    Indirection. Every time you add a new class to do something and call it from somewhere else you've added an extra level of indirection. This can be good when dealing with complex logic but for many cases it just hinders code readability.
    - alisnic 14y ago
      I am not sure I understand your point, can you elaborate?
      - Firehed 14y ago
        When the actual logic resides deep in a call stack, it's more difficult to debug code, much less bring on new developers. Each additional abstraction layer is, unsurprisingly, another layer that you're going to have to trace through to get at the root of the issue. Similarly, when your logic doesn't reside at the root level, it's easy to accidentally bypass. Perhaps this is intentional, but I'd argue that intentionally bypassing certain logic under certain conditions indicates your logic is poorly factored. My most common example to this is an admin panel: admins can, as an example, change any user's name, not just their own. However, this only holds true when in the admin panel. So in your user controller, you have some sort of permission check ensuring that the user being operated on is the one authenticated[1], but intentionally don't place that check in your admin controller. One day in the name of a better UX you add an AJAX component to this, and forget to copy across that check into the ajax endpoint. Suddenly I can change anyone's username, despite not being an admin. To solve this, we've created a couple of libraries: PermissionCheck and AuthContext; the former (typically) depends on the latter. AuthContext answers the questions of who are you (authentication) and where are you (context), and the permission check wraps up the authorization component: in the (web frontend | api | admin panel | cronjob | upgrade script) does the currently authenticated user have permission to do X on object Y? By putting these checks in the models, it's easy to ensure they're always enforced (a user's updateName method may call checkUpdateName()->enforce() ); if you want to side-step (or ramp up) authorization under a certain context, you edit the model's permission check for that action, rather than one or more controllers. Could you put that into a different helper library? Probably, if you were so inclined. However, that leaves you with at least two files to check when you want to examine behavior, and an inability to use any protected or private methods from these permission checks. Having an object that does a lot of stuff is not necessarily the much-feared "God object", although it certainly can become that. I've learned to become more fearful of the "God methods" instead; having 300 10-line methods doesn't concern me nearly as much as 10 300-line methods. Breaking code apart simply because you want a smaller file means you have a crappy text editor. Break code apart when it's logically doing more than it should be doing, or is poorly encapsulated. This is, to a limited degree, an issue I have with mocking in unit tests: while great in theory (test components individually and trust that your interfaces/contracts/whatever ensure that A and B independently working mean that in production they will work together), in practice the two components do have a dependency on each other, and it's exceedingly difficult to reproduce production issues because of that fake isolation. How many ways have you seen an external API fail? Have you reproduced those failure modes in your mock, and have you written tests against each one of those different modes? Maybe it's the industry I'm in (payments), but in a world made of edge cases, effectively squaring the number of scenarios I have to test is not a pleasant experience. /edit - forgot my footnote :) [1] In practice, a user controller will hopefully tend to always operate on the currently-authenticated user, rather than passing around a userId by whatever means. However I've seen the latter done, and this exact kind of problem is what ensued.
alisnic 14y ago
If you do TDD, I don't see how this slows you down when you are isolated from the start. (Rails takes ~1 second to boot in the beginning). And you are right, this was not intended to be presented as refactoring pattern, although it qualifies as one, this is how I manage code in my projects from day 1. And I do agree with you that is not always reasonable to do that, but I just feel more benefits rather than penalties doing it from the start.

by tinco 14y ago

jeremyjh 14y ago

People will spew all these separate classes from the very beginning in the name of "test-ability".

alisnic 14y ago
I don't understand what is the problem of having a lot of small classes? They are there when you need them, and they get out of the way you don't, and I think that's the most important thing.
- fbuilesv 14y ago
  Indirection. Every time you add a new class to do something and call it from somewhere else you've added an extra level of indirection. This can be good when dealing with complex logic but for many cases it just hinders code readability.
  - alisnic 14y ago
    I am not sure I understand your point, can you elaborate?
    - Firehed 14y ago
      When the actual logic resides deep in a call stack, it's more difficult to debug code, much less bring on new developers. Each additional abstraction layer is, unsurprisingly, another layer that you're going to have to trace through to get at the root of the issue. Similarly, when your logic doesn't reside at the root level, it's easy to accidentally bypass. Perhaps this is intentional, but I'd argue that intentionally bypassing certain logic under certain conditions indicates your logic is poorly factored. My most common example to this is an admin panel: admins can, as an example, change any user's name, not just their own. However, this only holds true when in the admin panel. So in your user controller, you have some sort of permission check ensuring that the user being operated on is the one authenticated[1], but intentionally don't place that check in your admin controller. One day in the name of a better UX you add an AJAX component to this, and forget to copy across that check into the ajax endpoint. Suddenly I can change anyone's username, despite not being an admin. To solve this, we've created a couple of libraries: PermissionCheck and AuthContext; the former (typically) depends on the latter. AuthContext answers the questions of who are you (authentication) and where are you (context), and the permission check wraps up the authorization component: in the (web frontend | api | admin panel | cronjob | upgrade script) does the currently authenticated user have permission to do X on object Y? By putting these checks in the models, it's easy to ensure they're always enforced (a user's updateName method may call checkUpdateName()->enforce() ); if you want to side-step (or ramp up) authorization under a certain context, you edit the model's permission check for that action, rather than one or more controllers. Could you put that into a different helper library? Probably, if you were so inclined. However, that leaves you with at least two files to check when you want to examine behavior, and an inability to use any protected or private methods from these permission checks. Having an object that does a lot of stuff is not necessarily the much-feared "God object", although it certainly can become that. I've learned to become more fearful of the "God methods" instead; having 300 10-line methods doesn't concern me nearly as much as 10 300-line methods. Breaking code apart simply because you want a smaller file means you have a crappy text editor. Break code apart when it's logically doing more than it should be doing, or is poorly encapsulated. This is, to a limited degree, an issue I have with mocking in unit tests: while great in theory (test components individually and trust that your interfaces/contracts/whatever ensure that A and B independently working mean that in production they will work together), in practice the two components do have a dependency on each other, and it's exceedingly difficult to reproduce production issues because of that fake isolation. How many ways have you seen an external API fail? Have you reproduced those failure modes in your mock, and have you written tests against each one of those different modes? Maybe it's the industry I'm in (payments), but in a world made of edge cases, effectively squaring the number of scenarios I have to test is not a pleasant experience. /edit - forgot my footnote :) [1] In practice, a user controller will hopefully tend to always operate on the currently-authenticated user, rather than passing around a userId by whatever means. However I've seen the latter done, and this exact kind of problem is what ensued.

alisnic 14y ago

If you do TDD, I don't see how this slows you down when you are isolated from the start. (Rails takes ~1 second to boot in the beginning). And you are right, this was not intended to be presented as refactoring pattern, although it qualifies as one, this is how I manage code in my projects from day 1. And I do agree with you that is not always reasonable to do that, but I just feel more benefits rather than penalties doing it from the start.