Object Oriented Ember architecture? Single responsibility?


#1

Having browsed through the ember and ember-data code, I find that there appears to be a “fear” of defining new classes to handle new concerns. Mostly the code is organized in a very flat fashion, and using mixins to a great extend. I think the code would be much easier to understand and much easier to contribute to if it was organized in a more OO fashion.

As an example, looking at the store.js DS.Store class, it is almost 1500 lines long, with perhaps around 60+ methods and counting… I don’t see how this can adhere to “Single Responsibility”? Why not split up the create, update and find/query parts into separate classes. Then use mixins for these classes for shared concerns/logic such as transactions and adapter etc. The didXXXX methods also logically belong together.

Same goes for the Ember Router/Route implementation which also seems to bundle too many varied concerns together under these two main classes. I think a new architecture approach using more of an OO design would really clean up the code bade and allow for much more flexibility and better understanding (including newcomers to the project). With a better/cleaner architecture it would be easier to understand the code and easier to contribute. A class should rarely have more than 10 functions in my experience (if been programming for over 25 years).

I do understand however that the project is under a lot of stress, has many contributors and are pushing for a 1.0 release. I just hope that the 1.1, 1.2 or 2.0 release will gradually introduce a much better OO architecture with a clearer separation of concerns. Of course we gotta start somewhere :wink:


#2

Yes, that’s something that bothered me. Ember-data is specially complex to understand. That makes things more complicated to people to contribute and put a lot of pressure in a few developers that knows how everything works.

I see a lot of anti-patterns in ember in that regard. Take the router for example. There are a lot of functions there that should be methods of a class. It’s very hard to override just one method there, because you need to recreate those functionally all over again (or you override it inline in the main file, but that’s hardly the best approach).

Just to exemplify what I’m saying:

If you decide to override the render method and reuse the functionality of normalizeOptions, you can’t. Why normalizeOptions isn’t a method of that class?

It’s even worst in others classes like: https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/vendor/router.js

If you want to override the handleURL, like I wanted, you will need to create your on collectObjects, setupContexts, loaded, setContext, resolveHandlers, eachHandler, partitionHandlers and maybe others.

I mean, what kind of reuse is that?

And there’s a lot of mixed forms of defining a class: sometimes using prototypes, sometimes using Ember itself, sometimes just functions and factories. Although that part may be for performance reasons and that could be understandable.


#3

Totally agree… having looked at Ember for about 2 weeks now, I’m baffled by how it locks you into one way of doing things. It is very limiting in its design, exactly because they use an extremely flat/procedural design and passing state around in functions. This is exactly why I never liked to use javascript much before, but modern javascript (and coffee) is supposed to have cures for that and Ember has a nice object model - so why not use it more :-; I think the current design is mostly due to time-constraints and continual “patching” - just fix it y adding another function, oh, yeah, we need to pass all the state… ok, we will worry about architecture and cleanup later. More important we document what we have now… just too bad. I hope that they will have more time to clean up the internal design/arch. later, but from the Ember Camp talk, it sounds like they are putting big limits on themselves in this regard, since they want to follow semantic versioning and not break any existing APIs or unit tests. Now my worry is this - how fine grained are the existing unit tests? If they only test the top level APIs however, they can still redesign the internals without interfering with the existing APIs :wink:

I would love to hear what other people think… I tend to be the kind of developer who always likes to tweak the frameworks, rarely just using it “out of the box”, why these kinds of things worry me a little. But I’m not trolling, just a concerned developer wishing for better development tools :slight_smile:


#4

I agree that a lot of functions used throughout the framework could be converted to class methods. Maybe even make it a rule: No anonymous functions. Private methods can be marked with a @private JsDoc annotation and/or prefixed with underscores.

Here is a real world example where a guy is trying to add very little functionality to Ember.LinkView:

But he needs to redefine a lot of “private” functions which are basically the exact same in the core code.

It would be nice to know if the core teams agree on this? In that case we all can just start digging :slight_smile:


#5

I think it should be done sooner than later. Because everyone is starting to copy that madness.

See: https://github.com/emberjs/list-view/commit/333cacec81849b364437c9fc773936b3f3739674#L9R59 (there are a lot more cases in this PR than just this line)

And so many other commits and projects I’m seeing.

Every day it passes, it will be more difficult to revert it.

Is there a reason for it? @wycats? @tom?

I’m interested in helping out. Just waiting a word from the core team as well.


#6

It’s not too late to do it for 1.0 if it doesn’t affect APIs that are supposed to be public or protected… but where is that line drawn? How long would it delay release?

Is it really the kind of thing you do after RC2? Probably not.

That said, perhaps now is the time to get very specific about which hooks are considered to be part of the API and which aren’t, so that everyone can hack knowing they do so at their own risk and hope that Ember 2.0 addresses these concerns.


#7

Is the objection to putting internal functions inside the closure scope of each file?


#8

If those functions are going to be used inside class’ methods, yes.


#9

Many of those functions are intentionally private. I am open to PRs that add more public API, but we would need to discuss them carefully first.


#10

Would it make sense to expose “stuff” that should be available to applications as privileged?

Privileged

A privileged method is able to access the private variables and methods, and is itself accessible to the public methods and the outside. It is possible to delete or replace a privileged method, but it is not possible to alter it, or to force it to give up its secrets.

-Crockord


#11

I don’t think those methods are private, at least not all of them. As @stusalsbury said, they should be protected/ privileged.


#12

Well, technically I wasn’t claiming that anything should or shouldn’t be private. I think what I meant was that the privileged strategy can be used to clearly delineate what should be considered fair game – to call, not to override. Public functions that are designed to allow overrides could make it a point to use only privileged functions.

Edit: note, I’m not talking about cases where there is an expectation of application enhancements by calling ::_super(args), I’m talking about when you want to allow applications to rewrite public functions – i.e. those functions that the API is designating as something equivalentish to protected. So the protected-ish methods are in fact public, but would rely solely on privileged methods so that they can be cleanly overridden.


#13

I wasn’t talking about converting to class methods or making methods private, protected, privileged - whatever. To me, a class such as DS.Store, reminds me of a presentation given by “uncle bob” (Rob Martin), where he had a class (User or Order?) scroll insanely fast for about 5mins? with scary/creepy music, demonstrating the worst kind of bad practice of “OO” design, just trying to fit (where else to put) every function into the same class (KISS principle!?). This turns into a nightmare pretty soon and no one dares to even attempt to clean it up… so I only gets worse. I think classes should be used more to subdivide concerns such as finding, vs internal state logic (didUpdate?) vs create new record, vs update, vs load, vs … so many more fine grained classes would make it much less of a nightmare I’m sure. Less complex, not more complex. Please try it out for size :wink: Just dividing into submodules (mixins) that end up in the same class scope is not a real architecture fix for sure.


#14

Classes size aside, I think the underline for protected and double underline for private are a good way of telling your compromise for the long term for methods. It has been used by some major projects over the years, successfully IMO.

qooxoo uses that convention. I used it for a long time and that was clear in the docs if you should or shouldn’t be using a specific method. Developers accepted it pretty well. For years developing for it, I have seen just a few cases where people was complaining/asking why their code stopped working after updating.

I’ll stress again why I think the current convention is not a good choice: Imagine the same scenario I describe earlier, you are developing and realize you need to subclass a class to make it work the way you want. Looking at the class code, you see you just need to add a if in one of its methods. But adding that simple if will make you copy and paste code from 1, 2, 3, 4, 5 … N functions and convert them into methods of your class. What was a simple 30 seconds change, turned out to take you 30 minutes. After all, you took more time adding functionality, and was unable to reuse a lot of code already there, end up duplicating it, making your app bigger in size and complexity. And changes by the core team still can, and probably will, break your code in the future[1].

You don’t get the advantages of OO and still get the disadvantages of letting all methods “publicly” available.

[1] In case two methods use the same private function, you may override just one method and end up with one method calling the private function and one using your copy and paste code. When changing the private function, you will be relying that your method and the function are doing the same thing, but they will not. They are different. That’s the kind of bug harder to find than simply getting a TypeError: Object #<Object> has no method '__methodName'.


#15

Hi all, I came here after being linked on my pull request to fix this problem on the router. I think it definitely makes sense to make this stuff overrideable. It’s really annoying when it’s impossible to override a method that needs overriding.

It makes sense to have a _privateMethod type convention, but intentionally hiding stuff in closures causes more problems than it solves in my experience - it seems a bit paranoid and inconsistent to be honest. There are plenty of other methods that are already overridable that can cause problems when upgrading and shouldn’t be relied on, but then some methods are arbitrarily impossible to change. And I don’t think the solution is to make them “public API”. This stuff is internal. But to make it hidden in a way that is not at all possible to override just leads to even uglier hacks that are more brittle, copy-pasting code, and developer frustration. I challenge any supporter of this approach to find someone who has actually been helped by it, because I can easily point to at least 4 people who have been hindered by it.

There is more detail in this recent post I found on HackerNews and totally agree with. People will do crazy shit and you won’t stop them by limiting them like this, all you will limit is legitimate extension.


#16

To be brutally honest, I left the “Ember camp” in part because I found it easier/possible to override things in Angular… just sayin… … Ember has a lot going for it and I didn’t really want to leave, but feeling locked in was the number one reason I looked for alternatives (the other two being the cognitive dissonance that struck me when trying to decide what data provider to use, and freedom from naming conventions).


#17

I’m sorry you found this to be the case. In general it’s actually pretty easy to override things in Ember. However, we have done a poor job of documenting that in some areas. Can you share which parts you had trouble with?


#18

You’ll be happy to know that we’re started a renewed push to simplify the Ember Data internals. The goal is to have things significantly improved within the next month.


#19

Thanks for asking. It’s been a while and I don’t remember everything that went through my head, but I recall feeling cramped by the router. I’ve gone from using the angular-ui project’s ui-router/state service to writing an offshoot of that router that supports lazy routing and on-the-fly changes to routing to support a CMS-type environment… so I’m sure routing was on my mind. I don’t recall ever thinking about replacing the router in Ember – would that have been possible? Does Ember support something like Angular’s “decorator” facility to overwrite portions of components that you need to operate differently or just swapping out entire components when you want a fresh start? That’s something I really appreciate in Angular, but I never thought about trying it when I was using ember (although I did contemplate writing my own data layer since I couldn’t find one that was stable and felt right to me at the time, and since I knew there was flexibility in that area to plug in my own).