Re: [simspark-devel] Re: communication code progress

[Joschka B. <jbo...@un...>]

Hi,

Markus Rollmann wrote:
> Hi,
>
> Hesham wrote:
> > Thank you so much for your fast and complete review. My code had a lot
> > of coding style mistakes, if I know this I didn't send it :-)
>
> I think your code was actually quite ok. It's just that every project
> uses it's own style of coding in order to make life easier for the
> developers as the need to read each other's code. Otherwise the codebase
> would result in a mixture of different styles. The only trouble is that
> most projects use their very own style guide. So sending in you're code
> for review is a good way to adapt to it ;-)
>
> >     - 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'
> >
> >
> > But I didn't write it. It seems it's a variable that sexpmonitor uses
> > that. But I didn't change it because I'm not sure about it.
>
> Ok I see. It seems that Irene Markelic committed that code... (according
> to 'cvs annotate sayeffector.h') But now you're our expert for the
> sayeffector ;-)
>
> Do you understand how ifText is used in the sayeffector? Can you come up
> with a better name for it? Or can we safely remove this code?

I don't get the meaning of this method either. It seems to be something very 
specific that Irene implemented for her undergrad thesis. 

@Oli: do you remember anything about that?

> I looked through the changed version of you're code and I think it looks
> very good now. If Joschka and Oliver are Ok with it I think we can
> commit it.

It looks very good. Just a small little thing in the 
SayEffector::GetActionObject() method:

The do...while loop is not necessary, I think. It's just run one time anyways, 
so you could just move the construction of the empty ActionObject into the 
if-statement where you check for the message error:

if (predicate.name != GetPredicate())
{
    GetLog()->Error() << "ERROR: (SayEffector) invalid predicate"
                      << predicate.name << "\n";
    break;
}

Predicate::Iterator iter = predicate.begin();

std::string message;
if (! predicate.AdvanceValue(iter, message))
{
    GetLog()->Error()
        << "ERROR: (SayEffector) said message expected\n";

    // some error happened
    return shared_ptr<ActionObject>();
}

// construct the SayAction object
return shared_ptr<SayAction>(new SayAction(GetPredicate(), message));

> I think there is just one thing missing: In order to use the say
> effector with the soccer simulation you had to modify the
> rcssserver3d.rb (app/simulator/rcssserver3d.rb, function addagent) to
> install the effector. I think you missed to include this change in the
> file you sent.

Right. If possible, please send this file for reference.

Cheers,
Joschka