[Rock-dev] Discussion: Timestamped Commands in base/types

[Steffen Planthaber]

Am 05.11.2013 11:14, schrieb Sylvain Joyeux:

> I do, however, have an issue with the way you did it. Why not use an
> implicit copy constructor ? You are basically assuming that your base
> class does not have a set() method ...

I don't know if i got you right, but there is no explicit copy 
constructor, the only copy constructor used is an "upcast constructor" 
one from the base class to the derived one (which initializes the 
missing time variable in the base class with Time::now(), different 
discussion).

Only in case of the timestamped commands, this was useful and enabled 
the commands to be written to a timestamped output port:

command_in.read(command)
commandSample_out.write(command)

The copy constructor is only defined for the command  itself, not for 
the template, so i didn't see an issue here. The implicit copy 
constructor is used in all other "normal" cases.

But I agree, the programmer should "give a thought" on the time to use, 
so I'm fine with removing the "upcast constructor".

>>> What I am strongly against is the initialization of the timestamp with
>>> Time::now(). Timestamps need to be carefully thought before they get
>>> set, and Time::now() is usually *not* the right answer. This is
>>> promoting bad habits.
>>
>> Well, the case i started with was the one of commands. When you want a
>> timestamped command set the time there, it will definitely be
>> Time::now(), when the timestamped data was created.
> Yes, and it is wrong in the general case. I did not mean this as a
> personal attack, it is simply a comment that the final type should IMO
> have no such method.

This is ok, I don't argue that

>
>>> Moreover, since the time field is public, having a setter which just
>>> sets the field is ... not really needed.
>>
>> Well the existence promotes the importance of setting the timestamp when
>> a better source is available than Time::now().
> ???? In which world ?

In my world ;-).

Having public class variables is gernerally bad, you cannot change or 
refactor the internal implementation (e.g. varibale names) without 
breaking everyones implementations, having getters and setters lets you 
change class implementations without breaking other programmers code.
I mostly program this way. This is also why I personally read the 
documentation of methods and functions more than the documentation of 
class variables. But as i understood having the time variable publis had 
other reasons, at least i hope so.

But not having getters and setters is fine with me in this case.

> My point is: when one sets a timestamp field, he has to *think*. To me,
>    bla.time = base::Time::now();
> is good because it is explicit
>    commandType bla;
> is wrong because the programmer will not even give a thought. And
>    bla.updateTime()
> is wrong because it is implicitly setting the value to base::Time::now()

Good point.

Best, Steffen

-- 
  Steffen Planthaber
  Weltraumrobotik

  ###############################################
  #### Neue Anschrift und neue Kontaktdaten! ####
  ###############################################

  DFKI Bremen
  Robotics Innovation Center
  Robert-Hooke-Str.5
  28359 Bremen, Germany

  Phone: +49 (0)421 178 45 - 4125
  Fax:   +49 (0)421 218 - 64150
  E-Mail: steffen.planthaber at dfki.de

  Weitere Informationen: http://www.dfki.de/robotik
  -----------------------------------------------------------------------
  Deutsches Forschungszentrum fuer Kuenstliche Intelligenz GmbH
  Firmensitz: Trippstadter Straße 122, D-67663 Kaiserslautern
  Geschaeftsfuehrung: Prof. Dr. Dr. h.c. mult. Wolfgang Wahlster
  (Vorsitzender) Dr. Walter Olthoff
  Vorsitzender des Aufsichtsrats: Prof. Dr. h.c. Hans A. Aukes
  Amtsgericht Kaiserslautern, HRB 2313
  Sitz der Gesellschaft: Kaiserslautern (HRB 2313)
  USt-Id.Nr.:    DE 148646973
  Steuernummer:  19/673/0060/3
  -----------------------------------------------------------------------