Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Can somebody looks my code https://github.com/NicholasShatokhin/OpenNNL ? What can you say about it? Can you say something about API?

Best regards, Nick

share|improve this question
To help other people as well as you we prefer if you post the code as part of the question. If you want advice on the API post the API and your expected usage patterns. Then maybe we can comment on them. – Loki Astari Nov 14 '12 at 0:05

closed as off topic by Jeff Vanzella, Corbin, palacsint, Trevor Pilley, Brian Reichle Nov 15 '12 at 2:06

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined in the FAQ. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about closed questions here.

2 Answers

up vote 3 down vote accepted

Looked at your code, here is something that can be said:

  • Naming.

    Using of namespaces is strongly recommended, especially in the public project. And it should not be OpenNNL unless you are not going much deeper with developing you implementation and creating the comunity around it.

    The class should be named accordingly the entity it represents, eg. NeuralNetwork.

  • Comments.

    You should use english comments in public projects. And i personally recommend doing so in private ones, too.

    Add somewhere a comment with reference to description of model you are using or the file name with its description.

    You use very little comments in implementation part. This decision is totally up to you, but using comments on what the code is doing is always better then not using them in my opinion.

  • Interface

    Public methods before private methods are preferrable, because they are the ones the programmer is looking for in most cases. Also, consider applying PIMPL idiom.

    XXXRefs methods are breaking class invariant. Also see my notes on arrays below.

    Plain C arrays in interface are not something your users will be happy to have. Consider using std::vector, overloading operator[] and building more abstractions, eg NeuronLayer. Also see notes on arrays below.

    load/saveNetwork simply should not be there. The class is responsible for modelling network, not disk operations. Consider overloading operators >>

    I recommend to hide compute methods from user. Instead, make getOutputs to compute the result. Of course, use dirty flag to indicate if it should be comuted at all.

    BP and IDBD should be decoded in comments, i think.

  • The code

    Extensive use of plain C arrays. There should be a strong reason for this. Standard classes std::vector and std::valarray is less error prone and easer to use. If used properly thy have no or very little overhead. As i see, you considered to provide CUDA implementation, which may restrict in usage of theese classes, but consider providing vectors in interface anyway.

    What about copy and assignment? Be explicit in this field, implement operations or make them inaccesible.

    Note about inline functions. It's strange to have no implementation in header. And, in my experience such functions will be inlined by compiler without inline modifier. Functions compiler won't inline, it will not inline with modifier. In my opinion, this keyword is obsolette.

    calculateWorker is not very expressive name. Consider change it and possibly devide this function on several functions doing descibable tasks.

    Consider refactoring changeWeightsXXX functions. They are too long, not commented and have commented debug code in them.

    Functions doEpochXXX have debug code in them.

share|improve this answer
+1 That's a really good feedback. Too bad code is kind of tl;rl for this site. – avip Nov 14 '12 at 9:04

In addition to GeniusIsme answer:

  • Do not put using namespace in a header, this extremely pollutes namespace.

  • Short names of enumerations are unreadable (make them longer and more descriptive) and have higher chances for a name conflict. I'd recommend using enum class from C++11.

  • MnistFile has protected members but no virtual destructor. Using protected members means that you expect the class to be used as a base class, but lack of the virtual destructor means that you cannot delete an object of a child class via the pointer to the base class.

share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.