[simspark-devel] Re: communication code progress

[Markus R. <rol...@un...>]

Hi Hesham,

I looked over the code you've submitted and I think it looks okay and
it compiled without errors (gcc 3.3.4).

However I've got some comments about the coding style that I ask you
to change. The comments below are about how the 'perfect' code in our
CVS should look- you'll certainly find many places that differ from
this style. But we should try to minimize these ;-)

In the doc/ section of the CVS you'll find a document called
codingstyle.txt that explains this in detail.

regards,
Markus

SayAction.h
-----------

void GetMessage(std::string& mess)
- call it 'msg' instead of 'mess' ;-)

- is the #include <salt/vector.h> necessary =

sayeffector.h
-------------

- Are body.h and ball.h #includes necessary?

- Please try to minimize number of #includes in .h files as they
increase the compile time and number of dependencies in large
projects. All class types that you only need in pointer types or
references can be forward declared. Move the #include statements into
the .cpp file.

Example:

--(instead of )--

#include <oxygen/agentaspect/agentaspect.h>
#include <soccer/soccerruleaspect/soccerruleaspect.h>

---

-- (write) --

namespace oxygen
{
	class AgentAspect;
	// other forward declarations...
}
class SoccerRuleAspect;

---

sayeffector.cpp
---------------

- unnecessary #include: random.h

- What is the meaning of the 'ifText' member? Please add a comment to
sayeffector.h explaining it

- All member variables should start with a small 'm' for
'member'. Therefore the ifText member should be renamed 'mIfText'

hearperceptor.cpp
-----------------
- ther are left over commented lines (87-89)

agentstate.h
------------

- Please add some comments to the new member variables that describe
their meaning (mHearMax, mHearInc etc.)

agentstate.cpp
--------------

- please indent blocks under if, for and while statements, i.e. add {
} lines: (line 125, 126 and 134, 135 and 184,185 )

if (mHearMateCap < mHearDecay)
{ // add these lines
     return;
}

- line 134: please put opening and closing brackets on separate lines
and indent the following block

         // end of block1
     } else
         {
             // start of block2

soccerruleaspect.cpp
--------------------
- please indent blocks under if statements, i.e. add { } lines (same
as in agentstate.cpp)

- don't declare a loop iterator outside of the loop. This is bad style
and can lead to subtle errors (e.g. unintended reuse). Please declare
it inside the loop, i.e. declare it in line 733 and 756.

- typedef complex type, i.e. don't write
    std::list<boost::shared_ptr<AgentState> >::const_iterator i;

    instead write something like
    typedef std::list<boost::shared_ptr<AgentState> > TAgentStateList
    in a .h file

    then write only TAgentStateList::const_iterator in the .cpp- thats
    far more readable. This typedef could go into soccerbase.h (you
    could modify the GetAgentStates function to use this type when
    you're at it). Someone made the same mistake there and used a very
    very long type in the function instead of a typedef

    You'll find an example for this method is in
    lib/zeitgeist/leaf.h. There (line 57) TLeafList is declared and
    used for example in GetChildren (line 136)

- please dont't use std:: and boost::, oxygen::, salt:: etc. prefixes
in .cpp files. That's harder to read. At the beginning of
soccerruleaspect.cpp you'll see 'using namespace' statements (line
34,35). This means that you can ommit these prefixes and write
e.g. 'list' instead of 'std::list'. Note: Only use 'using namespace'
in .cpp files.

- futher please format complex and long for statements over 3 lines,

Your for loops should then look something like this:

      for (
	  TAgentStateList::const_iterator it = agent_states.begin();
	  it != agent_states.end();
	  ++it
          )
	 {
	  // ...
          }