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))


        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,

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 >
Flightgear-devel mailing list

Reply via email to