jvandemo / angularcodereview-com

Collection of free checklists you can use to perform a code review of your Angular application.
http://angularcodereview.com/
28 stars 6 forks source link

Change "Is filtering logic handled in controller if needed?" to "Is filtering logic handled in a service if needed?" #3

Open aj-dev opened 8 years ago

aj-dev commented 8 years ago

I suggest changing Is filtering logic handled in controller if needed? to "Is filtering logic handled in a service if needed?"

The reason behind this is that controllers should be as lean as possible which makes them easy to test. Services should provide filtered data, ready to be consumed by controllers. It also makes filter functions reusable if they are defined in services.

More info can be found in John Papa's guide Defer Controller Logic to Services and Keep Controllers Focused, Minko Gechev's guide Controllers.

jvandemo commented 8 years ago

@aj-dev — Thank you, I agree with your reasoning.

The point the performance card is intended to make is:

Do not just blindly perform filtering in the view template using pipes. Instead perform the filtering in the controller so you can better control how many times data needs to be filtered.

You rightfully indicate that it is smart to also delegate filter logic to a service, which wouldn't necessarily help with performance here though.

So I'm wondering if your remark isn't more related to https://github.com/jvandemo/angularcodereview-com/blob/master/src/angularjs/index.jade#L458?

What do you think? Thanks again!

aj-dev commented 8 years ago

@jvandemo, thanks for you reply.

I partially agree with you.

Controlling how and when data needs to be filtered can be done in a controller but it can also be done and, imho, should be done in a service if we take other best practices into account. Delegating filtering to the service will not degrade performance but will increase testability of the service and keep controller lean.

Proxying controller methods to service might seem like an overhead but if filtering logic is complex (more than 1 line of code), it should probably go into a service.

jvandemo commented 8 years ago

@aj-dev — I completely agree with your reasoning.

However, the controller will still be needed to access the filter logic in the service, so it's kind of tricky to get the wording right.

How would you phrase it? Thanks for your input!

aj-dev commented 8 years ago

@jvandemo, it's tricky to phrase it in a concise way, but I think we could simply say "Is filtering logic handled in controller or service if needed?" and add another sentence to the "What?": "Also consider moving complex filtering logic into a service to keep controller lean."