On 30 Sep 2013, at 08:47, Dirk Dittmann <dirk.dittmann....@gmail.com> wrote:
> branche fix-issue1164 @ my clone
> https://gitorious.org/fg/dirks-flightgear/source/85592ec45db2866a15197c051d97cb4014537b4b:
Hi Dirk, this is looking pretty good, just some small nitpicks:
- please make a helper function for the great-circle XTK error function, with
some sane name like 'greatCircleCrossTrackError'. And please include a full URL
to the aviation formulary link, for future readers of the code.
(You already did this in one place I think)
- where you only want course OR distance, SGGeodesy has some convenience
wrappers (I realise in most places in this patch you do want both). This is
just to improve readability right now (avoids useless 'az2' declarations), but
in the future we might be able to do less work if only one value is needed.
- Please squash commits, rebase -i is your friend - so all the evolution of the
LegController should probably be squashed together. Similary the adding and
removing of _mode_node will disappear with some squashing.
- Avoid UTF-8 degree symbols in comments, it might upset some compilers. We
recently had an issue with the older GCC on Jenkins rejecting UTF-8 BOM marker.
- I would prefer arithmetic terms in conditions to *always* be parenthesised,
we've had some bad bugs due to this, so:
if (a < (b + c))
NOT
if (a < b + c)
- Where boolean conditional get complex, I often like to create explicit bools,
so instead of:
if ((some complex test) && (some other complex test) && ((another
thing) || (still more)))
I think it's more readable to do:
bool answerOfTest1 = some complex text
bool answerOfTest2 = some other complexTest
…
if (answerOfTest1 && answerOftest2 && …)
The compiler will get rid of the bool vars, although you might force evaluation
of more terms depending on how smart you the optimiser is, but you force
yourself to give each boolean expression a name, like
'isWithinOverflightDistance' or 'isWithinInterceptAngle'. As a result it
becomes much easier for someone else to evaluate the conditional logic and
decide if they agree with it or not. If the boolean test is used more than
once, make it into a helper method - grep-ing for methods names is*() and
has*() in the codebase points to many of these.
In general it looks pretty good though, let me know if you're happy to make the
above changes or need any help (especially if you're new to git rebase -i, it
can do terrible things)
Kind regards,
James
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk
_______________________________________________
Flightgear-devel mailing list
Flightgear-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/flightgear-devel
