Cleaner controller class syntax

[davej]

Any thoughts about the idea of providing a base CoffeeScript class that can be extended to hide some of the cruft (e.g. specifying injectables twice and binding class methods) associated with using CS classes instead of functions.

Perhaps something like this? https://gist.github.com/elado/8138516

[davej]

Made a couple of small adjustments, this is how I'm currently doing my controller classes with Radian: https://gist.github.com/davej/8198036

As you can see controllers are a lot cleaner now. We could also register the controller automatically without the call to @register(). Personally I prefer an explicit call to @register() because omitting it seems a bit too magical for my liking.

[ahmednuaman]

Very nice, I've made an update with some possibilities, all of which are valid; I've also removed as much of the logic in BaseCtrl and moved it into a static helper as that's more performant that duplicating all those functions every time we new up a controller: https://gist.github.com/ahmednuaman/8200701

[ahmednuaman]

Also the module-helper can be used with any class, eg a service too.

[ahmednuaman]

TBH I prefer this:

@register 'appController', [
  '$scope'
  'pageTitleFactory'
]

[ahmednuaman]

What do you think of 3c6d09e9ecc3a456e91a6ae37e11a98600800a68?

[davej]

Looks good. The only thing I'm a bit unsure of is why you're registering the AppController with angular as appController with a lowercase 'a'? Is it just to show an example where the controller name is changed?

Angular generally has the convention to begin with an uppercase letter for controllers. Is it just a stylistic or are you avoiding a namespace conflict with the class name somehow? It would make a bit of sense if appController was an instance of AppController but personally I just find it confusing when you're registering the controller as a different name.

How would it look using your preferred syntax if you wished to register the controller using the existing name of the class? I think I should be able to register like so:

@register [
  '$scope'
  'pageTitleFactory'
]

... and it would register the controller as the class name. It seems un-DRY to have to specify the controller name twice if there is no good reason for it.

Overall though, I think this is great!

[ahmednuaman]

I prefer camel casing as that's then universal for all modules, whether they're controllers, factories or filters; I believe this keeps it simple.

I think it does make sense to tidy up the register as you suggested, but the biggest thing for me is performance, I worry that generating the module name on the fly is unneeded code and overhead, but maybe I'm being anal?

[davej]

I prefer camel casing as that's then universal for all modules, whether they're controllers, factories or filters; I believe this keeps it simple.

Ok, I guess I tend to think that Angular only has a few conventions so it's best to stick to them, especially if you want to describe the frameworks as 'scalable' (i.e.. used by multiple developers).

I think it does make sense to tidy up the register as you suggested, but the biggest thing for me is performance, I worry that generating the module name on the fly is unneeded code and overhead, but maybe I'm being anal?

It's already there because coffeescript uses named functions for classes (@constructor.name) so there is no overhead, this is why you can do cls.name in your example. I believe that older versions of coffeescript don't do this which is why I have added the parseFnName method as a shim, if you're running a recent version of coffeescript then parseFnName shouldn't be executed.

[davej]

Sorry got that wrong, it's shouldn't be an issue with Coffeescript but parseFnName will be used in IE. It might be worthwhile measuring the performance hit to see if it's significant. I might try booting up a windows VM to test it at some stage.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name

[ahmednuaman]

I think even Angular gets confused with its conventions, eg compare the service class casing to the controller class casing on http://docs.angularjs.org/api/AUTO.$provide (scroll to the service example); hence why I think it'd be better and simpler to have one form of casing across the board.

Regarding the name issue, I'll checkout what coffee script does, I'd like to keep the performance shape.

[ahmednuaman]

Also happy NYE!

[ahmednuaman]

What do you think of this: ac21c4f44d8104f4665acbb1d0afe4c9e1535d63

[davej]

Happy New Year :-)

I think even Angular gets confused with its conventions, eg compare the service class casing to the controller class casing on http://docs.angularjs.org/api/AUTO.$provide (scroll to the service example); hence why I think it'd be better and simpler to have one form of casing across the board.

It's actually consistent:

• ThisIsUninstantiated (i.e. a class)
• thisIsAnInstance

A controller is uninstantiated, and it remains uninstantiated when you register it using app.controller 'FooController', FooController, so it always begins uppercase. In fact, it is never instantiated unless you reference the class in a template using ng-controller="FooController".

On the other hand, when you register a service ($provide.service('fooService', [FooService])) it becomes instantiated (i.e. it is invoked with the new keyword). So, because it is now instantiated it begins with lowercase.

Hope that explains it for you, the whole factory/provider/service thing in Angular is confusing but I think the class/instance naming conventions are consistent. :-)

[davej]

Excellent, yes I think that new API is the best of both worlds, thanks!

[ahmednuaman]

Ah see I never knew that the controller API didn't instantiate the class til it was used. That makes sense. So then the issue is to put all the names as camelCase and just the controllers as ClassFoo.

[davej]

Yeah, personally I think that's better and consistent with the Angular philosophy but I don't think the base controller should enforce any convention. Currently you are running lowercaseFirstLetter() on the class name, I feel strongly that this is wrong and it will confuse people when their controller isn't working because the base class has magically changed it's name.

Please don't change the function to uppercaseFirstLetter. Just let people do the own thing and instead encourage naming conventions in your docs if you wish.

[davej]

Two other issues to be aware of:

• The @register string syntax signature is ambiguous. Currently if I do @register '$scope' then it will name the class as $scope and it wont inject $scope. Perhaps you should remove it? It's a nice convenience but I think an unambiguous API is more important.
• Registering without a class name (e.g. @register ['$scope', 'pageTitleFactory']) wont work in IE, I haven't tested it but I believe this is true even in the latest version of IE, this is why I included the parseFnName method. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name

[ahmednuaman]

Makes sense, I think this is the best way to register a class as it's the fastest, strict and we don't need to be messing with casing the name:

@register 'AppController', [
  '$scope'
  'pageTitleFactory'
]

[ahmednuaman]

It's also a lot less code!

[ahmednuaman]

See 5a116ff

[ahmednuaman]

If you're happy then I'll take this further and all in a base for services, factories, etc.. too.

[davej]

To be honest, I feel registering without a class name should be possible. Nine times out of ten, I reckon the class name will be the same as the controller name.

@register [
  '$scope'
  'pageTitleFactory'
]

[davej]

or perhaps you could use a different API for auto registering if you wish to make it more obvious what is happening? This would also allow you to pass in the injectables as params instead of an array.

@autoRegister '$scope', 'pageTitleFactory'

[ahmednuaman]

Yeah but speed is my big concern, also remember that this code will be minified, eg:

var __hasProp = {}.hasOwnProperty,
    __extends = function (e, t) {
        function r() {
            this.constructor = e
        }
        for (var n in t) __hasProp.call(t, n) && (e[n] = t[n]);
        return r.prototype = t.prototype, e.prototype = new r, e.__super__ = t.prototype, e
    };
define(["controller/radian-controller"], function (e) {
    var t, n;
    return t = function (e) {
        function t() {
            return n = t.__super__.constructor.apply(this, arguments), n
        }
        return __extends(t, e), t.register("StubController", ["$scope"]), t.prototype.init = function () {}, t
    }(e)
});

That's StubController minified. Now if we didn't specify the name there then there would be problems as the name parsed would be t right?

[davej]

Yup, good point. Cool, let's go with the explicit syntax you have defined in 5a116ff so.

FYI: There's a good history of Coffeescript and the .name + minification issues here: https://github.com/jashkenas/coffee-script/issues/2052

[ahmednuaman]

Cool, here's the latest: 54c2c25

Karma is giving me grief over coffee coverage support, which is always fun.

[davej]

I've just figured out that you can use anonymous classes, this gets me over my obsession with not repeating class names :-)

class extends BaseCtrl
  @register 'AppController'
  @inject '$scope'

  init: ->
    @$scope.foo = "yo"

By the way, I'm still using the original syntax, it's a little bit longer but I prefer it because it's more explicit and 100% obvious what it is doing. Any chance you'd consider supporting this syntax too? I can make a pull request if you like.

[ahmednuaman]

Yep anon classes are nice but I still prefer keeping register and inject as one method purely because it means the base class is smaller and all the logic is deferred to the static helper.

[ahmednuaman]

Merged #19

[davej]

It looks like you removed automatic binding of public functions to the $scope, any particular reason for this? I find it very useful.

[ahmednuaman]

I think it should be at the discretion of each different developer. Personally I prefer to explicitly specify which functions the $scope has access too and what they're bound to.