refactored code into more modular structure and added build

[joelhooks]

The monolith was hard to parse mentally so I broke it out into angular modules. Additionally wanted a gulp build, and thought this would be a strong candidate for adding to bower so I added that as well.

The modular approach is still a work in progress. I also plan to add unit tests and refine the overall structure.

Would love to hear thoughts about this direction, and if it is desired. If it isn't, I'll stop sending PRs ;)

[danhunsaker]

Not against any improvements. :-)

Don't have time at the moment to read through your changes, but I'll give them a proper review when I get the chance.

[joelhooks]

This PR is really to open a discussion with some code and show you what I've done so far. No rush, I'm going to keep tinkering.

The build result in /release should be functionally the same as the existing library.

Joel

On Mar 16, 2014, at 16:44, Daniel Hunsaker notifications@github.com wrote:

Not against any improvements. :-)

Don't have time at the moment to read through your changes, but I'll give them a proper review when I get the chance.

— Reply to this email directly or view it on GitHub.

[joelhooks]

Whoops, this code doesn't actually work yet. So let's talk about the changes generally :)

I've fixed the issue, just traveling today so can't push.

On Mar 16, 2014, at 14:44, Daniel Hunsaker notifications@github.com wrote:

Not against any improvements. :-)

Don't have time at the moment to read through your changes, but I'll give them a proper review when I get the chance.

— Reply to this email directly or view it on GitHub.

[joelhooks]

I've committed the fixes, and it looks like they are picked up in the PR.

[danhunsaker]

Yeah, the way GitHub handles PRs, anything you commit to that branch from here out will become part of the PR.

I should be able to review this now, but I get interrupted a lot, so it might still take a while.

[danhunsaker]

Alright, I've reviewed (but not tested) the changes. The way you chose to break things up wasn't quite what I expected, though that's hardly a problem in itself. I notice that there's a lot of "let's convert this to a function, despite the fact it's only used in one place", which is a coding habit I'm not terribly fond of. Each function call adds overhead, which slows things down somewhat, and it becomes noticeable when nearly everything is done inside a function call. There also seems to be a lot of "let's make a service" in places where a service doesn't really make sense, at least to me. Services are useful if you're building functionality that should be accessible to the entire app, but there isn't really anything in the directive that qualifies.

That said, I definitely see the benefit in modularizing the code. The lib/controls and lib/util bits should definitely be in separate files, and the main directive can definitely be broken apart some as well. So this is a good direction to move. I'd like to change a couple of specifics, though, specifically in how the main directive is split up, and in how much of the functionality is moved into functions uneccesarily. Shared functionality (anything used in more than one place) should absolutely be moved to functions. Moving code into functions just to improve readability, though, is inefficient. That purpose can be served just as well with simple variables.

I'm not the most experienced Angular or JS developer out there, so I won't say the approach taken here is wrong, but there are some improvements that could be made in the design which would only help the project as a whole.

[joelhooks]

A service is useful for making API that you want to encapsulate, test, and use with dependency injection.

There is absolutely no way those function calls are going to cause any real measurable performance issues in a form. Sure, the calls will make it several _ìs "_slower", but the cognitive gains that are achieved by making it parsable by a human far outweigh any optimization achieved by unrolling the functions. If there was actual, measurable, real-world performance issues that cropped up because of liberal use of functions, it would be extremely easy to do targeted optimization.

It isn't just a matter of shared functionality, it is a matter of creating code that is understandable, readable, maintainable, and testable. That's what the functions are for. The majority of them should be in utility services that are injected and tested in isolation.

[danhunsaker]

Alright, I was mistaken about the services. Completely spaced testability considerations. In fact, with that correction to my thinking, I'd like to see the code involved in creating the indivdual input elements broken into services, so that the if trees are simplified to selecting service calls, and we can run various tests on individual elements independent from the main directive.

As to overhead, it's likely to be a nonissue here, so I'll leave it for the moment, but it's still something to keep in mind when breaking things up. It's far too easy to use functions in ways that do result in noticable overhead (I'm thinking mobile devices, here), such as deep recursion and other causes of large stack depth.

I still feel, however, that there are a couple of places where you've used a function with no real benefit to doing so. That is, when that function is only going to be called once in the lifetime of the parent function, and so the operation could just as easily be performed inline and the result placed in a variable instead. Said another way, instead of function blah () { return result_of(expression); }, it would make as much sense, and be cleaner, to just say blah = result_of(expression);. This still improves readability, and is in fact more readable in my experience, so I don't see any reason to use the more complex approach in these cases.

None of my comments here are the final word on the topic, by the way. I'm just as interested in discussion on this as you are. I'll be pretty stubborn about some things (such as keeping use of functions low, for various reasons), mostly because of the 17 years or so of development experience I do have, despite only about 2 of those focusing on JavaScript, and about 1 with Angular. But I'm still open to ways to improve approaches to doing other things - such as using services properly. So please, continue the discussion!

[joelhooks]

Thanks for discussing it with me!

Totally agree I went "function crazy". It was mostly for myself and understanding. I plan to do some more, but with the end goal being structured sanity, not functions littered everywhere!

We can close this PR if you'd like. I am going to make a branch and work against that. If I put in some more time, we should be able to close in on something you will like.

Joel

On Mar 19, 2014, at 0:03, Daniel Hunsaker notifications@github.com wrote:

Alright, I was mistaken about the services. Completely spaced testability considerations. In fact, with that correction to my thinking, I'd like to see the code involved in creating the indivdual input elements broken into services, so that the if trees are simplified to selecting service calls, and we can run various tests on individual elements independent from the main directive.

As to overhead, it's likely to be a nonissue here, so I'll leave it for the moment, but it's still something to keep in mind when breaking things up. It's far too easy to use functions in ways that do result in noticable overhead (I'm thinking mobile devices, here), such as deep recursion and other causes of large stack depth.

I still feel, however, that there are a couple of places where you've used a function with no real benefit to doing so. That is, when that function is only going to be called once in the lifetime of the parent function, and so the operation could just as easily be performed inline and the result placed in a variable instead. Said another way, instead of function blah () { return result_of(expression); }, it would make as much sense, and be cleaner, to just say blah = result_of(expression);. This still improves readability, and is in fact more readable in my experience, so I don't see any reason to use the more complex approach in these cases.

None of my comments here are the final word on the topic, by the way. I'm just as interested in discussion on this as you are. I'll be pretty stubborn about some things (such as keeping use of functions low, for various reasons), mostly because of the 17 years or so of development experience I do have, despite only about 2 of those focusing on JavaScript, and about 1 with Angular. But I'm still open to ways to improve approaches to doing other things - such as using services properly. So please, continue the discussion!

— Reply to this email directly or view it on GitHub.

[danhunsaker]

I think I'll leave it open until the new PR is opened, just so it's here for others to be able to chime in, or in case one of us thinks of something else. Thanks for contributing!

[danhunsaker]

Made some updates to support fieldsets, which will probably affect how things break out into separate pieces. It may not be the best implementation for this, though - I'm not married to the exact approach. Just thought you'd like the heads-up as you work on this. I really won't have the time myself any time soon (which is to say, "thanks for the help!").

[danhunsaker]

Been a while since I checked in on this one. Had a chance to make any progress?

[danhunsaker]

Ping!