bugCERTI - Bugs: bug #24515, getOrderingName/Handle and...

 
 

bug #24515: getOrderingName/Handle and getTransportationName/Handle not implemented

Submitted by:  Petr Gotthard <gotthardp>
Submitted on:  Fri 10 Oct 2008 11:49:03 AM UTC  
 
Category: CERTISeverity: 2 - Minor
Item Group: HLA compliance errorStatus: Ready For Test
Privacy: PublicAssigned to: Petr Gotthard <gotthardp>
Open/Closed: ClosedRelease: CVS
Reproducibility: Every TimeFixed Release: 3.3.2

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Fri 10 Oct 2008 03:37:33 PM UTC, comment #4:

I'm OK with the consistency reason.
From time to time we should get rid of char*
in the internal CERTI API replace them with
std::string.

We may do it for a whole class, then
the class will be consistent.

Regarding the small std::map, I agree
its small so it's not currently worth the change.

We may modify it if we propose extension in the future.
In particular for TransportType, we may imagine more QoS strategies
this would be CERTI specific.

Eric NOULARD <erk>
Project Administrator
Fri 10 Oct 2008 03:11:31 PM UTC, comment #3:

Eric,

The arrays should be private. This is an error and I'm sorry for that.

However, I have a response to your other two comments ;-) I believe I did have good reasons. I knew the rules you mentioned above, but every rule has its exception and I believe this is the case.

I used const char* to make the new functions look like all the others. There is a block of similar functions and if one looks differently, the reader might be confused.
If we want these functions to use std::string, we should change all functions in this block at once to keep consistency.

Do you really want to have these little arrays to be std::map? This is a static array that never changes. If we make it std::map, we make it unnecessary complex. From both performance and understandability point of view (we loose the static initializer).

What do you think?

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.
Fri 10 Oct 2008 02:54:55 PM UTC, comment #2:

Petr,

When you add code for new feature
I think you should try to use

  • std::string instead of char*

unless you have good reason not to do so

  • STL datatype instead of plain C-style array

... unless you have good reason not to do so

in this case the:
TransportType getTransportationHandle(const char *theName);
const char *getTransportationName(TransportType theType);

would be
TransportType getTransportationHandle(const std::string theName);
const std::string getTransportationName(TransportType theType);

std::string is OK here because it is an CERTI internal API.

struct TransportTypeList {
const char *name;
TransportType type;
};

is would typically be a [static] map<TransportType,std::string>
which may be initialized in ObjectManagement constructor.

May be in this case it is overkill because map
would have 2 entries but this may be easier to read and
maintain.

A last remark is regarding encapsulation.
Try to use the more stringent visibility
private, then protected and last public.

Think twice before adding a public member to a class.
Making something public make it harder to change afterwards.

Your
struct TransportTypeList {
const char *name;
TransportType type;
};
static const TransportTypeList transportTypeList[];

should really be protected if not private to
ObjectManagement.

current CERTI source code is not always (humm...)
a good example for the preceding practice but I think
that when new or refactored code is done we should try
to apply those rules.

Do not take those remark as "pure" criticism but
as an effort to provider better code base.

Eric NOULARD <erk>
Project Administrator
Fri 10 Oct 2008 02:08:10 PM UTC, comment #1:

Both functions are implemented, however the behaviour is incorrect until the bug #24516 is fixed.

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.
Fri 10 Oct 2008 11:49:03 AM UTC, original submission:

These 4 functions are currently not implemented.

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by erk (Posted a comment)
  • -unavailable- added by gotthardp (Submitted the item)
  •  

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 29 Apr 2009 09:14:15 AM UTCerkOpen/ClosedOpen=>Closed
      Fixed ReleaseNextRelease=>3.3.2
    Fri 10 Oct 2008 02:08:10 PM UTCgotthardpStatusIn Progress=>Ready For Test

    Back to the top


    Powered by Savane 3.1-cleanup1