[WIP] Code quality, browserify, es6, tooling

[n8rzz]

Opening this PR for visibility with #678 . This branch is still very much under development.

Changes contained herein are massive and will cause breakages for anybody currently in development. This would be for a v3.0.0 release due to the massive changes in the codebase.

Major items addressed or being addressed in this MR:

• [x] adds .editorconfig
• [x] adds npm support and node_modules for external dependency management
• [x] adds project information to package.json file
• [x] adds build tasks: gulp build and gulp dist (and many more supporting tasks) to transpile, concat, minify, etc source files into the assets dir.
• [x] adds src dir for development
• [x] adds support for ES6/ES2015 via babel
• [x] adds eslint and eslint-plugin-airbnb ruleset
• [x] moves script import to foot of index.html
• [x] updates script to only import bundle.js
• [x] removes need for load_modules function in modules.js and removes scripted insertion of script tags
• [x] manually instantiates each file/class previously located inside the MODULES array.
• [x] adds callbacks in appropriate places for each module file
• [x] manually adds some functions to window for use elsewhere
• [x] begins addressing lint errors/warnings (eslint found 14,494 total errors/warnings)
• [x] add documentation for build tools
• [x] add ava and nyc for testing and reporting
• [x] abstract common modules across application into self-contained units
• [x] remove need for Fiberdependency in favor of plain es6 inheritance.
• [x] add npm run test task
• [x] translate files to ES6 where applicable
• [x] separate out concerns across smaller files. (This is a biggie)
• [x] util.js needs to be split into smaller files grouped by concern

---

Upcoming tasks (in no particular order):

• Refactor/simplify AircraftInstanceModel, AircraftFlightManagementSystem, Leg and Waypoint classes.
• remove global vars from the window, explicitly pass them where necessary (WIP)
• Explicitly pass window classes as parameters to only the class(es) that need them.
• integrate lodash where applicable (WIP)
• update for/in loops to use either for or _map when iterating through arrays. for reasons why, see: http://stackoverflow.com/questions/5263847/javascript-loops-for-in-vs-for (WIP)
• simplify if/else if/else logic blocks. Some methods/functions are 500+ lines long. There is room for abstraction and simplification there. (WIP)
• Various other abstractions and simplifications have been called out via TODO and FIXME comments and will be addressed soon.
• add tests

I'll add to/update this list as I continue to make progress. Feel free to add comments and feedback as you see fit. I didn't open this MR just to look at it! 😄

[erikquinn]

@n8rzz I was hoping we might be able to push the upgrades live somewhat incrementally.... if it does have to be kind of "everything at once", perhaps it's best to put this work on a separate branch eg zlsa/v3 which would eventually be merged into zlsa/gh-pages?

That way, we are all able to test locally and find and make whatever small fixes and amendments that might be needed before pushing it live to the main branch of the game. This way, it doesn't all have to be 100% perfect in a single supermassive PR, because that could take quite a while to do (I would think). Thoughts?

[n8rzz]

@erikquinn Yeah, sorry about that. I kinda just went for it. But, now that I think of it, it would probably be best to do all at once. There are a lot of changes and I'd hate to affect any other development. An all at once plan will be a little pain once instead of a little pain many times.

I think a zlsa/v3 branch would be the best plan anyway. With a refactor this size (I have or will have touched essentially every line of js), I am expecting at least some bugs. I've been trying to carefully test everything as I go, but I don't know the app as well as the rest of you. So I'm sure I will have missed something. I would feel much better knowing it's been kicked around a little bit before being released out into the wild.

Good news is with the new file structure, finding and fixing bugs should be significantly easier.

---

The thing that always gets me about refactors is this: If I've done a good job, the end user will hardly notice a difference. But the code that runs it all looks completely different. Crazy.

[erikquinn]

But, now that I think of it, it would probably be best to do all at once. There are a lot of changes and I'd hate to affect any other development. An all at once plan will be a little pain once instead of a little pain many times.

^ My thoughts exactly: if we're going to do it, let's just put the rest on pause, do it RIGHT once and for all.

I am expecting at least some bugs. I've been trying to carefully test everything as I go, but I don't know the app as well as the rest of you.

^ Oh, for sure, but the benefits immeasurably outweigh the potential bugs, as it would make it so much easier to work with for those of us working on implementing improvements to the "meat and bones" of the simulator. I'll make some time to look through what you've done and make comments, ask questions, etc so we can all be on the same page. And once it's workable, we can post a link for people to test it on the web instead of having to check out a local copy.

The thing that always gets me about refactors is this: If I've done a good job, the end user will hardly notice a difference. But the code that runs it all looks completely different. Crazy.

^ That's the idea! :wink:

Thanks a lot for taking the time to offer up all these improvements; they will make a really big difference. I think I speak for everybody when I say your help is quite welcome! :+1:

[erikquinn]

Added branch zlsa/v3, and made it the target branch for this PR.

[n8rzz]

@erikquinn I'm making good progress here.

Here is the loading profile for the current site:

and here is the loading profile for my develop branch:

With the changes I've made so far, load time has decreased by over a second. Ship it!

---

I look forward to your feedback! I have tried to leave a lot of breadcrumbs along the way as I travel through the app. There are a lot of spots that can be improved, some that I'm doing now and some that will take a little more time. I've noticed a very high frequency of hasOwnProperty checks, is there any reason for these? I'm not sure these are all entirely necessary and may be better accomplished with initializations so at least you know the property exists.

The process of a refactor often starts with a sledge hammer approach; smash big bits of code and let the pieces fall where they may. I'm taking more of a surgical approach, working on smaller sections at a time and making sure my changes won't have side effects. I am trying to lay the groundwork, though, for future smaller refactors like moving the functions in the util folder to more appropriate folders and also adding tests for those functions. I've started doing that in some areas, but those are smaller changes that I think can wait until the big stuff is done.

To be honest, I've spent most of my time fixing the spacings and adding curly brackets to ifs and fors. Curly brackets always, it just makes things easier to ready and reason about. It also doesn't rely on spacing to be interpreted correctly. This isn't yaml after all, lol!

Ok, back to work...

[n8rzz]

I have removed the fonts and images task from gulp dist. They are still available as a tool, but will no longer run with that specific command.

[n8rzz]

Just a quick update:

Still making good progress. I've moved over to a feature branch for the last few days just to keep things isolated. I'm almost finished tying everything together in encapsulated classes that get attached to the window. There is still a fair amount of work to do to move everything away from global window functions. Once I'm done on this current branch, there will be a very good base for making those changes. It's just taking time.

I'll merge this latest work in by the end of the weekend. yay! another 30 or so commits for this PR.

[n8rzz]

@erikquinn Fiber is officially gone! The main refactor is done! Load times, have improved:

However, work continues...

There is still a lot of work to do (see updated checklist above). I have already begun abstracting and simplifying (see: 996e567).

• Most of the main modules are still attached to the window, only now they are all encapsulated in instances that live in App.js. The next step is to explicitly pass them as parameters to only the files that need them.
• As I simplify methods, I have started adding comment blocks to each method. This is, so far, inconsistent throughout the app. But its a start.
• util.js needs to be split into smaller files grouped by concern and tests need to be added.
• Various other abstractions and simplifications have been called out via TODO and FIXME comments and will be addressed soon.

When you review this, it would be easier to just pull the repo and look at the code. I have added a readme to the tools/ folder that walks through getting this set up locally. I've done my best to make changes in small chunks and then test things after making those changes. But, alas, I'm sure there are some bugs that have snuck in. I am aware of two that are on my radar and will be addressed as part of my continuing refactor efforts. With the amount of changes made here, git bisect is your best friend when zeroing in on a bug. Seriously, know it, use it, love it.

I think this is coming along nicely, the code base is much more readable now and I'm moving towards a much better separation of concerns. These last commits are a great example of abstraction and simplification.

I do want to call out, if it isn't already painfully obvious, that I have touched every line of code in this project with the exception of the contents of the parser.js file. That means that some file histories (git histories) are going to be lost. Many files have been split up, renamed, or simply removed. So be aware that the gits will say I wrote a lot of this. I didn't. I've just moved it around and made it look pretty.

[erikquinn]

Wow... At first glance, much of the new code blows my mind! :heart_eyes: But I love how the whole thing feels, it just will take some re-learning on my part. There are (of course) some gameplay/functionality issues I've come across, but nothing worrying; I'm just thrilled with all the progress! :tada:

Will have to play around with it more later, gather a high-priority hit list of stuff that broke, and then we can go bug smashing :beetle: :hammer:

In the meantime though, unless there are important things you wanted to amend, I think it might be a decent idea to take a minute to clean up any obvious breakages, and then merge this into the v3 branch and post a testing link, in order to give the contributors some time to help by generating a comprehensive list of issues that I may not have noticed. Then, I (or anybody) can address the bug fixes simultaneously with your continuance along your list of "upcoming tasks", thus accelerating the timeline to getting 3.0 to the big stage.

I'm getting weirdly excited by all this! haha

[n8rzz]

Awesome, I'm glad you like what I've done. Funny thing is, even though it is laid out completely differently, a lot of the old code remains mostly untouched. Well, other than syntax changes and some simplifications here and there. There is a healthy amount of es6 sprinkled in there, now, too. And lodash. Lodash makes things soooo much easier. https://lodash.com/docs/4.16.2

I've updated my list above, there are still two items that I would like to finish before we tag this as v3.0.0 and start QAing. Both of those tasks are in progress right now and should be done by the weekend. After that, I'd feel comfortable with starting to squash bugs.

How would you like to handle tracking of those bugs; A specific tag on the existing issues board or create issues on my fork and address them there? Its up to you, I'm not picky just so long as there is a place to collect them, track them and make comments.

ES6 has a way of making javascript fun again. Its totally normal. Once you start dreaming in javascript, though, then you might want to start getting concerned. Waking up in the morning with visions of _map(this.waypoints, (waypoint) => waypoint.name) and such, then you may have a problem.

[n8rzz]

One other thing, if you have any questions about what I'm doing or why (or both) please don't be afraid to ask! I love talking javascript and I especially love talking about specific implementations. I don't get defensive about my code and code reviews are a daily thing at my job. So I'm not shy about critiques and/or justifying the why.

So please ask if you're curious or say something if you see something you don't like!!

[n8rzz]

For those interested, my fork is live at: https://n8rzz.github.io/atc/

I think, just to keep things together (and @erikquinn feel free to chime in) we should log any issues with this potential v3 codebase at: https://github.com/n8rzz/atc/issues

That way we can keep everything together, but separate. I will continue with the last two items above and merge that into gh-pages once its ready.

[n8rzz]

Unfortunately, I didn't get as much accomplished as I would have liked. I was successful in moving everything in util.js to proper concerned function files. There are still a few functions left in that file that may eventually move to a not-yet-created stringHelpers.js file. As of right now, though, there isn't a huge need for such a file. I did not get a chance to move the main controllers off of the window. That will have to wait for another day.

I think we're at a good stopping point here to start QAing the crap out of this beast. I have already logged a few issues that I'v found. Mostly minor stuff, although the ILS track one is somewhat concerning. I'll have to check the current game to see if I can reproduce it there.

For anybody that decides to help out on bugfixes, I'd like to follow a specific branching and merging strategy:

• any bugfix should be done on a specific branch off of gh-pages release/3.0.0 with a title bugfix/ATC-{ISSUE_NUMBER}.
• gh-pages is a protected branch in my fork. Once a fix is complete, please submit an PR that targets the develop branch.

I'm hopeful we'll be able to move through any bugfixes relatively quickly, but I've been through QA many times before and I know that that is not always the case.

At any rate, after we're done with QA I think my next order of business will be a refactor/simplification of the AircraftInstanceModel and the AircraftFlightManagementSystem along with [probably] Legs and Waypoints. There is a lot of room for refactor in all of those files, every time I'm in them I need to restrain myself from going on a refactor binge.

Everything is up-to-date on my fork's gh-pages branch. So feel free to take it for a spin (https://n8rzz.github.io/atc/) and log any issues you find (https://github.com/n8rzz/atc/issues). If possible, please provide screenshots and steps to reproduce. Also include your browser if you are not on Chrome.

[n8rzz]

@erikquinn yes. And IntelliJ and Vim.

My current editor of choice is Atom. I'm really liking it, too, it's giving vim a run for it's money. I don't think I'll ever go back to SublimeText (2 or 3). Too many weird bugs and not a lot of support. Atom is great because it's easy to customize, it's built on electron and the community is very active. Plus, being a client side developer, there is something poetic about writing code for browsers in a browser. And it's pretty fast too. My only complaint is with the current eslint plugins. I have two and both are, in my opinion, sub-par.

IntelliJ is great, but it's so slow. I was on a Scala project a while ago and IntelliJ was very useful there. I could run the db and debug the scalas right in the IDE. I do use it very occasionally for the merge tool and history diffing.

Now, though, probably 85% Atom 10% Vim 5% IntelliJ.

[n8rzz]

I forgot to add VSCode to that list as well. I recently just started using that instead of Monodevelop when I work with C# in Unity.

[eliuuk]

VSCode all the way!

[n8rzz]

closing this in favor of #702