Am Dienstag, 1. Oktober 2013, 21:37:58 schrieb James Turner: > 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)
I understand and will make the improvements. Thx for the hind ! I squash the commit and improve readability. Is there any code convention documentation ? Which I could read? > > 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
