imperial dive add / planner

[Dirk Hohndel]

Sorry for not responding sooner. I spent a good chunk of the afternoon 
in the ER after my wife took a rather bad fall.

I am happy with the new code. You did things that I wish you hadn't 
done, but they were all reasonably easy to address (well, a couple 
still need addressing):

- white space; I know it seems silly to many of you but consistency 
really pays off in your eyes getting better at "just getting" the code.
And those of us who have stared at Linux kernel code for 20+ years
are REALLY used to the rules that we are using here… so it does
make a difference:
There's a space after keywords: "for (…)" and "if (…)". There's space 
around curly braces. Especially "if (…) {" and things like "} else {"

Oddly it feels to me that I may have said this before (once, maybe 
twice), but since switching to kdevelop I can see some of the 
challenges as it by default does a real hack job out of white space.
QtCreator is actually better at dealing with our whitespace rules.

- units: we really worked hard to figure out sane units. Don't switch
to meters and minutes. We use mm and seconds for a reason.

I fixed the meters (in order to be able to do feet as well), but still
need to change over to seconds.

- your use of macros is sometimes really brilliant and sometimes
strikes me as odd. It's hard to put a finger on it - but overall I'd
consider it mostly positive.

- the classes seemed to be well designed and I had very little 
problems following the flow and figuring out which does what
and how they work together.

- the odd "variables are private and we have public accessor
functions" thing that is so popular in OO programming continues
to drive me batshit crazy. Not your fault, I blame the people who
taught you OO programming.

- your handling of corner cases sometimes seems a little 
half-hazardous. The code to deal with positioning of handles 
really wasn't all that well designed, I think.

- the handling of gas changes is way off - that's an area that we
need to redo. The user needs to be able to not just pick one of
three predefined gases - she needs to be able to enter any gas
she wants

- I can't figure out how I'm supposed to delete a node. When I
click on one it doesn't appear to stay selected. I can't right click
to get a context menu (which would be one way to easily be able
to both delete a node and change to a different gas on a node)

Now as always with these things, that sounds like a lot of
criticism. If I were Linus I guess I would simply say "your children
are ugly, your code sucks, you need to work harder" (trying to
channel his charming directness here). I'd put it differently:

This is really cool code that adds an impressive feature to 
Subsurface - a feature that I absolutely LOVE. I hope that with
increasing experience as a diver and more actual real life use
of Subsurface (I hear you still haven't actually downloaded dives
from the dive computer I gave you) I think the constraints and 
comments will make more sense - but overall I'm thrilled with
your contributions and find myself slowly approaching the point
where I feel like I can fix bugs and address issues.

Well done

/D

On Sep 22, 2013, at 12:54 PM, Tomaz Canabrava wrote:

> Care to comment on the code? Ya know, this is my first big change on tue subsurface code, i want some feedback, good or bad. :)
> 
> Em 22/09/2013 19:41, "Dirk Hohndel" <dirk at hohndel.org> escreveu:
> 
> I think I have this one solved as well.
> 
> Can people test dive add and dive planner a bit more - both in imperial
> and metric mode. I'm sure I must have broken something, somewhere.
> 
> Admittedly it wasn't as hard as I thought it might be... still... some
> rather intrusive changes. On the plus side I now understand that code
> much better :-)
> 
> /D
> 
> _______________________________________________
> subsurface mailing list
> subsurface at hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
> _______________________________________________
> subsurface mailing list
> subsurface at hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20130922/e63437c4/attachment-0001.html>