feature/ATC-181 #236

n8rzz · 2016-12-22T02:59:45Z

completes #181

Creates AircraftCommander class to run commands on an AircraftInstance

resolves both #254 and zlsa#777

…an AircraftInstance * Removes commands section in AircraftInstanceModel * Adds AircraftCommander instance to App that gets passed into * InputController * Removes no-undef rule from .eslintrc

…s todos and fixmes to AircraftCommander

erikquinn

Interesting. You're sure you like this abstraction? Everything makes sense, it just seems strange to me. If you like it this way, it definitely works for me... just curious. 😄

n8rzz · 2016-12-30T04:40:06Z

@erikquinn as I'm sure you've gathered already, I'm a big fan of small classes with a single concern. The current AircraftInstanceModel is a behemoth, this is a step in the right direction toward slimming that down.

I have many thoughts on this abstraction that we can talk about at a later date. TL;DR this is a step in the direction of centralizing command of an aircraft, reducing the scope of the AircraftInstanceModel and making things more testable. Though we're not there yet with this abstraction, its a step in that direction.

note: this won't be merging for a while, I just wanted to get it in front of your eyes sooner rather than later because of the scope of the change here.

erikquinn · 2016-12-31T01:38:21Z

@n8rzz Right, and I'm all for all of that. Why not plug it in soon? At least it would chop the aircraft monstrosity into slightly more bite-size pieces, and a later and better reorganization could always be done. This works though, I'd be fine with the change-- any reason you actively want to hold off on it?

n8rzz · 2016-12-31T03:17:48Z

@erikquinn I'd like to hold off because I had planned on @Dabea doing some method clean up and simplifications. Buuuut, I think that would probably be better accomplished under a new issue.

At the very least I'll hold off merging until we're done with #243 so as to avoid any conflicts.

n8rzz · 2017-01-04T13:02:34Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

+     * @method aircraftTurnPhysics
+     * This turns the aircraft if it is not on the ground and has not arived at its destenation
+     */
+    aircraftTurnPhysics() {


if we are updating aircraft properties, be sure to stay consistent with your naming. This method should likely be called updateAircraftTurnPhysics

n8rzz · 2017-01-04T13:02:59Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

+    /**
+     * @for AircraftInstanceModel
+     * @method aircraftTurnPhysics
+     * This turns the aircraft if it is not on the ground and has not arived at its destenation


be sure descriptions are at the top of your doc blocks

n8rzz · 2017-01-04T13:06:07Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

+
+        if (abs(offset) < turn_amount) {
+            this.heading = this.target.heading;
+        } else if ((offset < 0 && this.target.turn === null) || this.target.turn === 'left') {


is null a valid value for this.target.turn? It could be, there are nulls all over the place when there really shouldn't be (but that is a different issue).

In javascript, null is actually an object. when doing comparisons like this.target.turn === null you are actually checking to see if that property equals the null object. That may be different that what you are expecting. If this.target.turn is initialized as '' that comparison won't work as expected here because an empty string is not equal to null.

This is likely correct, but I wanted to call that out just in case.

I will have to look further in to it. That was the original logic, I will need to trace it back a little to see if it needs a null or ' '.

I'm fairly certain it is set to either 'left', 'right' or null whenever a turn is assigned. So the comparison should be okay, provided that the logic used within the fms during runHeading hasn't been changed in this regard. If in doubt, maybe a lodash _isNil() would do the trick?

…amd decreaseAircraftAltitude

…sistent nameing

…TC-181

Dabea · 2017-01-15T07:54:17Z

I fixed the merge conflicts and tested and everything looks good, and should not break anything once it is merged with develop.

n8rzz

Lots of little changes for you with docblocks.

One thing you'll notice, I'm a big fan of early returns. Try hard to avoid else if or else statements whenever possible. Often times there is a way to structure the logic so you don't ever need either of the elses. It also makes the code easier to read and way easier to test.

I like the length of your comments, not too long and right to the point. I think, though, that in some cases you are missing the meat of the methods you are trying to describe. This is something I struggle with constantly, what makes a good comment?

I setup a review app, though I haven't tested this yet. I will try to do that tonight. If I run into any issues, I'll add those here.

Other than the changes I called out, this is coming along nicely. You've made some good abstractions and some nice simplifications. I like where this is heading!

n8rzz · 2017-01-16T12:31:52Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

-            const runway = window.airportController.airport_get().getRunway(this.rwy_arr);
+        if (this.fms.currentWaypoint.navmode !== WAYPOINT_NAV_MODE.RWY) {
+            this.fms.setCurrent({ runway: null });
+            return false;


be sure to include an empty line above a return statement

n8rzz · 2017-01-16T12:33:37Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

@@ -2254,6 +1196,51 @@ export default class Aircraft {
        }
    }

+    /**
+     * This will display a waring and record an illegal approach event
+     * @for AircraftInstanceModel


be sure to include an empty line between comments and docblock parameters

n8rzz · 2017-01-16T12:33:48Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

+
+    /**
+     * This will display update the fix for the aircraft
+     * @for AircraftInstanceModel


missing empty line

n8rzz · 2017-01-16T12:34:18Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

+    }
+
+    /**
+     * This will display update the fix for the aircraft


the wording here is confusing, what is this doing exactly?

Yeah I really had a had time with giving this one a correct name and as well for the comment especially because it is for the FIX command where you change the target heading,

As writing this comment to you I think I found the answer to the strugle

n8rzz · 2017-01-16T12:35:01Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

+     */
+    updateFixTarget() {
+        const fix = this.fms.currentWaypoint.location;
+        if (!fix) {


be sure to include an empty line above if blocks

n8rzz · 2017-01-16T12:48:49Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

        let difference = null;

        if (this.target.speed < this.speed - 0.01) {
            difference = -this.model.rate.decelerate * window.gameController.game_delta() / 2;

            if (this.isOnGround()) {
+                // What is 3.5 is this restiance/breaking power?
                difference *= 3.5;


@erikquinn what is 3.5 here?

Haven't the foggiest. I'll see if I can figure it out...

n8rzz · 2017-01-16T12:49:03Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

-
+    /**
+     * This calculates the ground speed
+     * @for AircraftInstanceModel


missing empty line

n8rzz · 2017-01-16T12:49:58Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js

-        const isInsideAirspace = this.isInsideAirspace(window.airportController.airport_get());
+    /**
+     * This calculates the simplify ground speed
+     * @for AircraftInstanceModel


missing empty line

n8rzz · 2017-01-16T12:50:14Z

src/assets/scripts/client/aircraft/AircraftInstanceModel.js


-        const isInsideAirspace = this.isInsideAirspace(window.airportController.airport_get());
+    /**
+     * This calculates the simplify ground speed


this comment is confusing. what exactly are we doing with this method?

I changed it to
"This uses the current speed information to update the ground speed and position"

That is more clear, Any recommendations for improvement?

I like that, that is a little more descriptive

n8rzz · 2017-01-16T12:50:48Z

src/assets/scripts/client/aircraft/FlightManagementSystem/AircraftFlightManagementSystem.js

+
+
+
+


please remove these empty lines

…e needed

n8rzz · 2017-01-20T13:25:22Z

src/assets/scripts/client/aircraft/AircraftCommander.js

+        aircraft.taxi_start = this._gameController.game_time();
+        const runway = this._airportController.airport_get().getRunway(aircraft.rwy_dep);
+
+        runway.addQueue(this);


@Dabea This one is my fault :-(

This line prevents the aircraft info box from appearing when an aircraft has finished taxiing and is first in line to takeoff.

Having abstracted these commands away from the AircraftInstanceModel, why is this line now incorrect? What should be done to fix it?

…TC-181 Conflicts: - src/assets/scripts/client/aircraft/AircraftInstanceModel.js

feature/ATC-181 - Creates AircraftCommander class to run commands on …

e1e15a7

…an AircraftInstance * Removes commands section in AircraftInstanceModel * Adds AircraftCommander instance to App that gets passed into * InputController * Removes no-undef rule from .eslintrc

n8rzz added feature moonshot labels Dec 22, 2016

n8rzz added this to the v3.3.0 milestone Dec 22, 2016

n8rzz assigned n8rzz and erikquinn Dec 22, 2016

n8rzz requested a review from erikquinn December 22, 2016 02:59

n8rzz added 2 commits December 21, 2016 22:29

feature/ATC-181 - Adds window methods as instantiation arguments, add…

7b38a0a

…s todos and fixmes to AircraftCommander

feature/ATC-181 - changes ref from aircraft to this

6d253bf

n8rzz added refactor and removed feature labels Dec 23, 2016

Replaced string sid with enums value

ed4b04d

erikquinn reviewed Dec 30, 2016

View reviewed changes

erikquinn added the WIP label Dec 31, 2016

Dabea added 5 commits January 1, 2017 20:27

simplifed logic in AircraftInstanceModel.js for cancelLanding

5da479f

removed TODO message for simplifing logic

a5d582f

simplified updatePhysics with the addition of updateSpeedPhysics

8122c0c

simplifed updatePhysics by adding aircraftTurnPhysics()

692e5d6

simplified updatePhysics() by adding updateAltitudePhysics()

27d93fd

n8rzz commented Jan 4, 2017

View reviewed changes

Dabea added 5 commits January 7, 2017 23:21

simplified updateAltituePhysics by creating increaseAorcraftA;totdie …

367185d

…amd decreaseAircraftAltitude

fixed spaceing and simplied more logic

ac7d2fe

added TODO for increaseAircraftAltitue and decreaseAircraftAlititude

3916a4e

fixed doc block descriptions

be945bb

updated aicraftTurnPhysics to updateeAircraftTurnPhysics for more con…

b5a090e

…sistent nameing

n8rzz mentioned this pull request Jan 15, 2017

Continued simplification and refactor of AircraftCommander class #260

Closed

n8rzz temporarily deployed to atc-dev-pr-236 January 15, 2017 04:56 Inactive

Dabea added 2 commits January 15, 2017 00:49

Merge branch 'develop' of https://github.com/n8rzz/atc into feature/A…

f8c3ef8

…TC-181

'Completed Merge from devlo'

f1c6218

n8rzz temporarily deployed to atc-dev-pr-236 January 15, 2017 07:52 Inactive

Dabea removed the WIP label Jan 15, 2017

n8rzz commented Jan 16, 2017

View reviewed changes

Dabea added 2 commits January 18, 2017 03:27

resolves both #254 and zlsa#777 by removeing -+.02 variance

2c26e0d

updated comments to make more sense and added and removed spaces wher…

d7b348d

…e needed

n8rzz temporarily deployed to atc-dev-pr-236 January 18, 2017 09:57 Inactive

fixed spelling issue on comment

cb598fa

n8rzz temporarily deployed to atc-dev-pr-236 January 18, 2017 10:15 Inactive

improved comment

0d58fcd

n8rzz temporarily deployed to atc-dev-pr-236 January 18, 2017 10:19 Inactive

removed console.log messages

95c9d3d

n8rzz temporarily deployed to atc-dev-pr-236 January 18, 2017 10:54 Inactive

erikquinn mentioned this pull request Jan 19, 2017

Continued simplification and refactor of AircraftCommander class openscope/openscope#145

Closed

n8rzz commented Jan 20, 2017

View reviewed changes

fixed bug where aircraft info box dose not apear when given taxi command

2a2355c

n8rzz temporarily deployed to atc-dev-pr-236 January 22, 2017 07:09 Inactive

n8rzz added 2 commits January 22, 2017 22:48

Merge branch 'develop' of https://github.com/n8rzz/atc into feature/A…

4147078

…TC-181 Conflicts: - src/assets/scripts/client/aircraft/AircraftInstanceModel.js

feature/ATC-181 - adds changelog entry

f0d7f78

n8rzz temporarily deployed to atc-dev-pr-236 January 23, 2017 04:50 Inactive

feature/ATC-181 - updates runwayModel method name that was changed

43b2269

n8rzz deployed to atc-dev-pr-236 January 23, 2017 05:20 View deployment

n8rzz changed the title ~~[WIP] feature/ATC-181~~ feature/ATC-181 Jan 23, 2017

n8rzz merged commit 64a565a into develop Jan 23, 2017

n8rzz deleted the feature/ATC-181 branch January 23, 2017 05:31

n8rzz mentioned this pull request Jan 23, 2017

feature/ATC-205 #245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/ATC-181 #236

feature/ATC-181 #236

n8rzz commented Dec 22, 2016 •

edited by Dabea

Loading

erikquinn left a comment

n8rzz commented Dec 30, 2016

erikquinn commented Dec 31, 2016

n8rzz commented Dec 31, 2016

n8rzz Jan 4, 2017

n8rzz Jan 4, 2017

n8rzz Jan 4, 2017 •

edited

Loading

Dabea Jan 8, 2017

erikquinn Jan 8, 2017

Dabea commented Jan 15, 2017

n8rzz left a comment

n8rzz Jan 16, 2017

n8rzz Jan 16, 2017

n8rzz Jan 16, 2017

n8rzz Jan 16, 2017

Dabea Jan 18, 2017

n8rzz Jan 16, 2017

n8rzz Jan 16, 2017

erikquinn Jan 16, 2017

n8rzz Jan 16, 2017

n8rzz Jan 16, 2017

Dabea Jan 18, 2017

n8rzz Jan 16, 2017

Dabea Jan 18, 2017

n8rzz Jan 18, 2017

n8rzz Jan 16, 2017

Dabea Jan 18, 2017

n8rzz Jan 20, 2017 •

edited

Loading

feature/ATC-181 #236

feature/ATC-181 #236

Conversation

n8rzz commented Dec 22, 2016 • edited by Dabea Loading

erikquinn left a comment

Choose a reason for hiding this comment

n8rzz commented Dec 30, 2016

erikquinn commented Dec 31, 2016

n8rzz commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8rzz Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dabea commented Jan 15, 2017

n8rzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8rzz Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

n8rzz commented Dec 22, 2016 •

edited by Dabea

Loading

n8rzz Jan 4, 2017 •

edited

Loading

n8rzz Jan 20, 2017 •

edited

Loading