FreeEMS Issues - OLV
View Issue Details
0000642OLVGeneral Featurespublic2012-08-05 22:432012-08-29 20:17
Fred 
Fred 
highblockN/A
closedfixed 
AllAllAll
0.0.3-SNAPSHOT 
0.0.30.0.3 
0000642: Full code review
Go over all files casually and make a list of shit that's blatantly wrong
Prerequisite of 0000636
No tags attached.
child of 0000636closed Fred Pre-0.0.3 release code clean-up using mvn site. 

Notes
(0001914)
Fred   
2012-08-22 22:16   
MacOSAboutHandler has copy/paste tell tails by way of spaces, not tabs, not caught style checker due to embedded in temp comment.

AboutFrame Line 105 is fine 136 is not: http://stackoverflow.com/questions/5234147/why-stringbuilder-when-there-is-string [^]

SingleGraphPanel constructor sets fields to null (they already are).

Occasional // comments with //this form

GraphPositionPanel roundDecimalsOnlyToTwoSignificantFigures versus MathUtils roundToSignificantFigures Only one copy/version/process, done as text, not as number, unless I am utterly confused.

EntireGraphingPanel has a bunch of minor white space issues in private final Action defs. Suggest turning on "show whitespace" in eclipse editor so you can see it and so that it's ugly for you too ;-)

EntireGraphingPanel line 200 has three calls to get current time, which could all be different, intended? if not, change to one call into a temp final var, if so, comment to explain.

Consider this vs this from InitialLineColoring
    private static final float ONE_THIRD = 1F/3F;
    private static final float ONE_THIRD = 0.3333F;

Also, why not 1F?
    private static final float ALMOST_ONE = 0.999F;

From OpenLogViewer: commented out code, ctrl / gets you a block comment automatically, you did it manually.

From OpenLogViewer exitFullScreen method, idents should be tabs, good, post code to comments should be spaces, bad, is tabs.

Can't see anything else obvious. Didn't run site myself.
(0001915)
BenFenner   
2012-08-23 14:09   
(edited on: 2012-08-23 14:48)
Git hash: 8fe16a9ae4dcbf2f13d209f5d48c240140014f18

Includes the following work:

AboutFrame line 136 uses StringBuilder now. (Also found a use for it in SingleProperty.)

SingleGraphPanel constructor no longer sets fields to null that are already null.

All comments should now be // in this form and not //in this form.

EntireGraphingPanel is cleaned of a bunch of minor whitespace issues. Show whitespace is now on for me in Eclipse.

EntireGraphingPanel line 200 no longer has three calls to System.getCurrentTime() with added comment too.

InitialLineColoring uses 1F/3F and 2F/3F now instead of 0.3333F and 0.6666F.

InitialLineColoring has explanation now of why the use of 0.999F.

OpenLogViewer commented out code is automatic now instead of manual.

OpenLogViewer exitFullScreen method uses spacing to indent comments after code instead of tabs. enterFullScreen gets the same treatment too.





Things not addressed yet:

MacOSAboutHandler has copy/paste tell tails by way of spaces, not tabs, not caught style checker due to embedded in temp comment.


GraphPositionPanel roundDecimalsOnlyToTwoSignificantFigures versus MathUtils roundToSignificantFigures Only one copy/version/process, done as text, not as number, unless I am utterly confused.

(0001916)
BenFenner   
2012-08-23 14:47   
Git hash: 308a5074d19cc1839f4433f2aab17c17db484f42

Replaced spaces with tabs where appropriate in MacOSAboutHandler.





Things not addressed yet:

GraphPositionPanel roundDecimalsOnlyToTwoSignificantFigures versus MathUtils roundToSignificantFigures Only one copy/version/process, done as text, not as number, unless I am utterly confused.
(0001944)
BenFenner   
2012-08-28 17:35   
The last remaining issue (the significant figure code stuff) has been taken care of.

Git hash: 4bbeed7f2efe5aba9d4a0c5e88604d49247549c9


I'm going to run site:site and clean things up and then hand it back to you Fred.
(0001950)
BenFenner   
2012-08-28 18:43   
After site code cleanup we're good to go. Checkstyle errors rose from 260 to 261. So sue me.
Resolve/close at your satisfaction.

Git hash : 9f2b24dee79a810758f85d05c9233d2df67eef68
(0001968)
Fred   
2012-08-29 20:17   
Haven't checked, but I'm calling it good anyway.