Feature/web app organization #216
Conversation
TekuConcept
commented
Mar 22, 2017
TekuConcept
commented
- Routes are now in their own file, reducing index.js file size and perceived complexity.
- Hardware specific calls have been moved to their own file. Route calls to hardware are made through a messenger.
- Hardware initialization and management are now hidden behind an app module. This module exposes the already-created factories to AI scripts.
- Removed experimental snippets, such as task-script implementations, in favor of promoting more organized approaches of execution and testing.
| CHANGE | ||
| ); | ||
| while((!Serial.available())||(Serial.read()==0)); | ||
| controllers[1] = new ThrustController(MOVE_PIN); |
There was a problem hiding this comment.
Might consider adding integer constants for these array indices in the future, but it doesn't prevent approval of this PR.
|
I think a second review on this PR would be good just because it's so large @nfcopier |
|
I'm pretty sure this pull request inherits changes from a couple of the previous PRs. I completed a review of #212. Check that one for my requested changes. |
| powerManager.turnOnEscs(); | ||
| }, | ||
| turnOffEscs: function() { | ||
| DMSG("Turn Off Escs"); |
There was a problem hiding this comment.
We have a logger. Use it. Also, this type of psuedo-macro looking naming does not feel appropriate for JavaScript.
There was a problem hiding this comment.
That was temporary (DMSG) - I forgot to delete it.
(Right now I'm not sure about the current integrity of the Logger object. I didn't change any of the code for it, but some portions of it were broken before I even started the WebBrainUpdate branch.)
| @@ -0,0 +1,36 @@ | |||
| var hdwr = require('./hardware.js'); | |||
There was a problem hiding this comment.
This file feels more than a little redundant.
There was a problem hiding this comment.
I felt the same. There were some occassions in other projects where this wasn't the case for me. Feel free to refactor it out.
| @@ -0,0 +1,138 @@ | |||
| module.exports = function(app, msngr) { | |||
There was a problem hiding this comment.
Please avoid unnecessary abbreviations.
There was a problem hiding this comment.
This is one of those moments when history begins to haunt you. (I would have named "msngr" => "messenger")
| var HardwareFactory = require('./CppInterface/Factory.js'); | ||
| var Vision = require('./VisionInterface'); | ||
|
|
||
| var server = Sockets.createSocket(Ports.DispatcherPort); |
There was a problem hiding this comment.
This is breaking encapsulation. These sockets should have been left inside the Factory.
There was a problem hiding this comment.
I have to disagree. App.js (while improperly named) is a form of "builder" design pattern. (The builder design pattern is one of the 23 discussed by the Gang of Four.)
A builder will take advantage of factories to create more complex objects.
There was a problem hiding this comment.
Now I will admit that this builder module does need to be better organized.
|
I really like most of these changes. Awesome job. 👍 |
|
I finished reading the "Design Patterns" book cover to cover as well as "The Clean Coder" and I am in the middle of the book "Clean Code." I feel I have a better idea now about writing amazing clean code. |
