AUVs mobility models

[Mathieu Lacage, 2010-07-23 09:59:43]

I did not look at the test or example code but my main comments are:
  - lack of documentation of what the models are modeling and what they do
  - kill unused code: don't leave it uncommented
  - don't implement dodispose methods which only chain up: kill them.

http://codereview.appspot.com/1875047/diff/1/6
File src/helper/auv-mobility-helper.h (right):

http://codereview.appspot.com/1875047/diff/1/6#newcode31
src/helper/auv-mobility-helper.h:31: class AuvMobilityHelper {
I am not sure but I would expect our style checking script to insert a newline
between the name and {. Did you run the script ?

http://codereview.appspot.com/1875047/diff/1/6#newcode34
src/helper/auv-mobility-helper.h:34: AuvMobilityHelper (std::string type,
it seems to me that it would be simpler to remove both constructors here and
replace them with a SetType (std::string type); and use ths SetAttribute method
below.

You should also define clearly what the default constructor does and more
specifically, what default type, and position allocate type it uses.

http://codereview.appspot.com/1875047/diff/1/6#newcode47
src/helper/auv-mobility-helper.h:47: void SetPositionAllocator
(Ptr<PositionAllocator> allocator);
where do you use this position allocator ?

http://codereview.appspot.com/1875047/diff/1/8
File src/mobility/auv-mobility-model.cc (right):

http://codereview.appspot.com/1875047/diff/1/8#newcode31
src/mobility/auv-mobility-model.cc:31: // .AddConstructor<AuvMobilityModel> ()
if code is un-used, remove it.

http://codereview.appspot.com/1875047/diff/1/8#newcode58
src/mobility/auv-mobility-model.cc:58: // chain-up
if you do not do anything in dodispose other than chain up, there is no point in
implementing it.

http://codereview.appspot.com/1875047/diff/1/11
File src/mobility/glider-mobility-model.cc (right):

http://codereview.appspot.com/1875047/diff/1/11#newcode40
src/mobility/glider-mobility-model.cc:40: //    .AddAttribute ("Bounds", "Bounds
of the area to cruise.",
kill dead code

http://codereview.appspot.com/1875047/diff/1/11#newcode87
src/mobility/glider-mobility-model.cc:87: GliderMobilityModel::DoDispose (void)
if you do nothing here other than chain up, no need for this code.

http://codereview.appspot.com/1875047/diff/1/12
File src/mobility/glider-mobility-model.h (right):

http://codereview.appspot.com/1875047/diff/1/12#newcode33
src/mobility/glider-mobility-model.h:33: * \brief keep track of the current
position of an Autonomous Underwater Vehicle (AUV)
do you have a reference to a paper which describes this mobility model ? or a
link to something which explains what this model does.

http://codereview.appspot.com/1875047/diff/1/12#newcode41
src/mobility/glider-mobility-model.h:41: //  virtual ~GliderMobilityModel () =
0;
kill unused code

http://codereview.appspot.com/1875047/diff/1/14
File src/mobility/remus-mobility-model.h (right):

http://codereview.appspot.com/1875047/diff/1/14#newcode32
src/mobility/remus-mobility-model.h:32: * \brief keep track of the current
position of an Autonomous Underwater Vehicle (AUV)
document what the model does

[socket, 2010-07-23 19:25:04]

I've updated the code according to Mathieu's comments. I've added more
documentation and references for the mobility models, deleted unused code and
the DoDispose methods.

On 2010/07/23 09:59:43, Mathieu Lacage wrote:
> I did not look at the test or example code but my main comments are:
>   - lack of documentation of what the models are modeling and what they do
>   - kill unused code: don't leave it uncommented
>   - don't implement dodispose methods which only chain up: kill them.
> 
> http://codereview.appspot.com/1875047/diff/1/6
> File src/helper/auv-mobility-helper.h (right):
> 
> http://codereview.appspot.com/1875047/diff/1/6#newcode31
> src/helper/auv-mobility-helper.h:31: class AuvMobilityHelper {
> I am not sure but I would expect our style checking script to insert a newline
> between the name and {. Did you run the script ?
> 
> http://codereview.appspot.com/1875047/diff/1/6#newcode34
> src/helper/auv-mobility-helper.h:34: AuvMobilityHelper (std::string type,
> it seems to me that it would be simpler to remove both constructors here and
> replace them with a SetType (std::string type); and use ths SetAttribute
method
> below.
> 
> You should also define clearly what the default constructor does and more
> specifically, what default type, and position allocate type it uses.
> 
> http://codereview.appspot.com/1875047/diff/1/6#newcode47
> src/helper/auv-mobility-helper.h:47: void SetPositionAllocator
> (Ptr<PositionAllocator> allocator);
> where do you use this position allocator ?
> 
> http://codereview.appspot.com/1875047/diff/1/8
> File src/mobility/auv-mobility-model.cc (right):
> 
> http://codereview.appspot.com/1875047/diff/1/8#newcode31
> src/mobility/auv-mobility-model.cc:31: // .AddConstructor<AuvMobilityModel> ()
> if code is un-used, remove it.
> 
> http://codereview.appspot.com/1875047/diff/1/8#newcode58
> src/mobility/auv-mobility-model.cc:58: // chain-up
> if you do not do anything in dodispose other than chain up, there is no point
in
> implementing it.
> 
> http://codereview.appspot.com/1875047/diff/1/11
> File src/mobility/glider-mobility-model.cc (right):
> 
> http://codereview.appspot.com/1875047/diff/1/11#newcode40
> src/mobility/glider-mobility-model.cc:40: //    .AddAttribute ("Bounds",
"Bounds
> of the area to cruise.",
> kill dead code
> 
> http://codereview.appspot.com/1875047/diff/1/11#newcode87
> src/mobility/glider-mobility-model.cc:87: GliderMobilityModel::DoDispose
(void)
> if you do nothing here other than chain up, no need for this code.
> 
> http://codereview.appspot.com/1875047/diff/1/12
> File src/mobility/glider-mobility-model.h (right):
> 
> http://codereview.appspot.com/1875047/diff/1/12#newcode33
> src/mobility/glider-mobility-model.h:33: * \brief keep track of the current
> position of an Autonomous Underwater Vehicle (AUV)
> do you have a reference to a paper which describes this mobility model ? or a
> link to something which explains what this model does.
> 
> http://codereview.appspot.com/1875047/diff/1/12#newcode41
> src/mobility/glider-mobility-model.h:41: //  virtual ~GliderMobilityModel () =
> 0;
> kill unused code
> 
> http://codereview.appspot.com/1875047/diff/1/14
> File src/mobility/remus-mobility-model.h (right):
> 
> http://codereview.appspot.com/1875047/diff/1/14#newcode32
> src/mobility/remus-mobility-model.h:32: * \brief keep track of the current
> position of an Autonomous Underwater Vehicle (AUV)
> document what the model does

[socket, 2010-07-27 19:55:14]

Added more documentation, modified the waypoint mobility model to use a constant
velocity model, added energy call for AUVs mobility models, some other minors
changes

On 2010/07/23 19:25:04, socket wrote:
> I've updated the code according to Mathieu's comments. I've added more
> documentation and references for the mobility models, deleted unused code and
> the DoDispose methods.
> 
> On 2010/07/23 09:59:43, Mathieu Lacage wrote:
> > I did not look at the test or example code but my main comments are:
> >   - lack of documentation of what the models are modeling and what they do
> >   - kill unused code: don't leave it uncommented
> >   - don't implement dodispose methods which only chain up: kill them.
> > 
> > http://codereview.appspot.com/1875047/diff/1/6
> > File src/helper/auv-mobility-helper.h (right):
> > 
> > http://codereview.appspot.com/1875047/diff/1/6#newcode31
> > src/helper/auv-mobility-helper.h:31: class AuvMobilityHelper {
> > I am not sure but I would expect our style checking script to insert a
newline
> > between the name and {. Did you run the script ?
> > 
> > http://codereview.appspot.com/1875047/diff/1/6#newcode34
> > src/helper/auv-mobility-helper.h:34: AuvMobilityHelper (std::string type,
> > it seems to me that it would be simpler to remove both constructors here and
> > replace them with a SetType (std::string type); and use ths SetAttribute
> method
> > below.
> > 
> > You should also define clearly what the default constructor does and more
> > specifically, what default type, and position allocate type it uses.
> > 
> > http://codereview.appspot.com/1875047/diff/1/6#newcode47
> > src/helper/auv-mobility-helper.h:47: void SetPositionAllocator
> > (Ptr<PositionAllocator> allocator);
> > where do you use this position allocator ?
> > 
> > http://codereview.appspot.com/1875047/diff/1/8
> > File src/mobility/auv-mobility-model.cc (right):
> > 
> > http://codereview.appspot.com/1875047/diff/1/8#newcode31
> > src/mobility/auv-mobility-model.cc:31: // .AddConstructor<AuvMobilityModel>
()
> > if code is un-used, remove it.
> > 
> > http://codereview.appspot.com/1875047/diff/1/8#newcode58
> > src/mobility/auv-mobility-model.cc:58: // chain-up
> > if you do not do anything in dodispose other than chain up, there is no
point
> in
> > implementing it.
> > 
> > http://codereview.appspot.com/1875047/diff/1/11
> > File src/mobility/glider-mobility-model.cc (right):
> > 
> > http://codereview.appspot.com/1875047/diff/1/11#newcode40
> > src/mobility/glider-mobility-model.cc:40: //    .AddAttribute ("Bounds",
> "Bounds
> > of the area to cruise.",
> > kill dead code
> > 
> > http://codereview.appspot.com/1875047/diff/1/11#newcode87
> > src/mobility/glider-mobility-model.cc:87: GliderMobilityModel::DoDispose
> (void)
> > if you do nothing here other than chain up, no need for this code.
> > 
> > http://codereview.appspot.com/1875047/diff/1/12
> > File src/mobility/glider-mobility-model.h (right):
> > 
> > http://codereview.appspot.com/1875047/diff/1/12#newcode33
> > src/mobility/glider-mobility-model.h:33: * \brief keep track of the current
> > position of an Autonomous Underwater Vehicle (AUV)
> > do you have a reference to a paper which describes this mobility model ? or
a
> > link to something which explains what this model does.
> > 
> > http://codereview.appspot.com/1875047/diff/1/12#newcode41
> > src/mobility/glider-mobility-model.h:41: //  virtual ~GliderMobilityModel ()
=
> > 0;
> > kill unused code
> > 
> > http://codereview.appspot.com/1875047/diff/1/14
> > File src/mobility/remus-mobility-model.h (right):
> > 
> > http://codereview.appspot.com/1875047/diff/1/14#newcode32
> > src/mobility/remus-mobility-model.h:32: * \brief keep track of the current
> > position of an Autonomous Underwater Vehicle (AUV)
> > document what the model does

[socket, 2010-08-05 12:08:57]

Review moved to http://codereview.appspot.com/1743057


On 2010/07/27 19:55:14, socket wrote:
> Added more documentation, modified the waypoint mobility model to use a
constant
> velocity model, added energy call for AUVs mobility models, some other minors
> changes