ginn

Code review comment for lp:~bijanbina/ginn/GIR

GIR
Merge into trunk

Revision history for this message

Stephen M. Webb (bregma) wrote on 2011-08-23:

I have a number of issues with this proposal.

(1) You need to explicitly justify moving from C to C++. In particular, renaming all existing C sources to C++. What does the project gain from this? What are the possible repercussions (ie. what new problems are introduced and what new potential for error is obtained)? The renaming introduces a lot of noise that makes it difficult to evaluate the rest of the change.

(2) This merge marks most sources as executable. That is undesirable.

(3) What is the .directory file? It does not appear to be necessary to build the project.

(4) Please do not use ///-style or //!-style comments, use Javadoc-style (/**) comments for consistency with the style of other uTouch projects.

(5) The various classes implement their own containers. Is there a good reason not to use the (tested, dependable) standard library instead? One of the main advantages to porting to C++ is the ability to use the C++ library.

(6) None of this code is exception safe. When errors are encountered, resources are leaked or undefined behaviour is encountered.

(7) There is no cleanup: objects are created using new but never deleted.

(8) Class constructors are assigning initial values to members in the constructor body instead of using initializer lists. Most of the classes are C structs that rely on external code to maintain their invariants: what is the point of converting to C++ if the code is just non-OO style C? See http://en.wikipedia.org/wiki/Class_invariant.

(9) Why add a capital G or D to classes and function names? This does not contribute to readability in a positive way. If this is to represent words like "device" or "gesture", use those words. We are not experiencing a shortage of ASCII.

(10) Some variable names could reflect their purpose better: for example, what is "local" used for?

(11) "header.h" is not an acceptable header file name. Also, please do not create a single header file to include all other header files any source file might need. Headers should pull in only the minimal subset of headers they need to compile on their own. Never put a "using namespace" directive in a header file.

(12) GINN::Dadded catches an exception of type error_t, but there is no code inside the try-block that can throw such a type. The only exception possible from the try-block is std::bad_alloc.

(13) Please leave at least one blank line between function definitions for readability.

I have a number of issues with this proposal.

(1) You need to explicitly justify moving from C to C++.  In particular, renaming all existing C sources to C++.  What does the project gain from this?  What are the possible repercussions (ie. what new problems are introduced and what new potential for error is obtained)?  The renaming introduces a lot of noise that makes it difficult to evaluate the rest of the change.

(2) This merge marks most sources as executable.  That is undesirable.

(3) What is the .directory file?  It does not appear to be necessary to build the project.

(4) Please do not use ///-style or //!-style comments, use Javadoc-style (/**) comments for consistency with the style of other uTouch projects.

(5) The various classes implement their own containers.  Is there a good reason not to use the (tested, dependable) standard library instead?  One of the main advantages to porting to C++ is the ability to use the C++ library.

(6) None of this code is exception safe.  When errors are encountered, resources are leaked or undefined behaviour is encountered.

(7) There is no cleanup:  objects are created using new but never deleted.

(8) Class constructors are assigning initial values to members in the constructor body instead of using initializer lists.  Most of the classes are C structs that rely on external code to maintain their invariants:  what is the point of converting to C++ if the code is just non-OO style C?  See http://en.wikipedia.org/wiki/Class_invariant.

(9) Why add a capital G or D to classes and function names?  This does not contribute to readability in a positive way.  If this is to represent words like "device" or "gesture", use those words.  We are not experiencing a shortage of ASCII.

(10) Some variable names could reflect their purpose better:  for example, what is "local" used for?

(11) "header.h" is not an acceptable header file name.  Also, please do not create a single header file to include all other header files any source file might need.  Headers should pull in only the minimal subset of headers they need to compile on their own.  Never put a "using namespace" directive in a header file.

(12) GINN::Dadded catches an exception of type error_t, but there is no code inside the try-block that can throw such a type.  The only exception possible from the try-block is std::bad_alloc.

(13) Please leave at least one blank line between function definitions for readability.

review: Needs Fixing

« Back to merge proposal