[Ns-developers] Some comments on tomh/routing (static routing manager)

[Tom Henderson]

Gustavo, thanks for the review.  Some comments inline

Gustavo Carneiro wrote:
>> From what I can see in the example program:
> 
>  if (RoutingEnvironment::StaticRoutingEnabled())
>    {
>      Ptr<StaticRouteManager> routeManager = Create<StaticRouteManager> ();
>      routeManager->BuildStaticRoutingDatabase ();
>      routeManager->InitializeRoutes ();
>    }
> 
> The API to be used is contained in the classes RoutingEnvironment and
> StaticRouteManager.  Yet the module exports practically all header files as
> public interfaces.  It would be better if it exported only a couple of
> header files, static-router.h and routing-environment.h, and in those files
> only include the definitions needed for the public part of the API.  This
> may require a bit of refactoring.  For instance, in the OLSR code I'm
> working on I only export a simple Olsr interface class with only a 
> couple of
> methods[1].  The implementation is contained in a "private" header file.
> The connection between interface and implementation is done through the
> component manager.

Yes, I agree with your observation that too much is in the public API. 
We intended to clean up such issues before posting a summary of this 
module to the list and inviting review.

> 
> Another comment is, I'm not 100% sure 'static routing' would be the best
> name.  When hearing "static routing" I associate with static routing table
> entries, or manually configured routing.  GOD routing is also not a very
> clear name, I'm afraid.  Maybe "smart routing"?  Or "automatic routing"?

The reason that I chose to call it static among the other choices is 
that it does install static routes in all of the routers (as opposed to 
a dynamic routing protocol).  I tend to prefer "static" to "God" or 
"smart" routing.  I guess the case could be made to call it automatic.

> 
> Also I was thinking of having the OLSR module as src/routing/olsr.  I see
> this module is src/routing.  Can we converge?  Do you think it's better to
> have a 'flat' module hierarchy src/routing, src/olsr, or a more organized
> module tree, src/routing/static, src/routing/olsr?  I initially went for a
> module tree, src/routing/olsr, because of the src/devices/p2p precedent.  I
> think I kind of like a tree better than flat namespace, but arguments 
> can be
> made either way...

I do not care strongly but I tend to favor the tree as well.  I would be 
fine if we follow the src/devices precedent, so am fine to move it to 
src/routing/static.

> 
> Finally, just a minor nitpick to point out that the code doesn't seem to
> always follow the ns-3 code style convention of including a space between
> method/function names and parenthesis.  E.g. this:
> 
>  if (RoutingEnvironment::StaticRoutingEnabled())
> 
> should be:
> 
>  if (RoutingEnvironment::StaticRoutingEnabled ())

duly noted (and fixed by now, I think)

Anyway, once the code has been cleaned up further, I will post a summary 
to this list.

Tom