Organizing you cpp class for maintainability

[Colin B Maharaj]

Hi all,
lets say I have some code like this

//--------------------------------------------------------

struct NumbersStruct
{
int value;
char string[32];
};

NumbersStruct Numbers[] =
{
{ 1, " One " },
{ 2, " Two " },
{ 3, " Three " }
};

class MyClass
{
public:
__fastcall MyClass();
__fastcall ~MyClass();
AnsiString MyIntToText(int v);
}

AnsiString MyClass::MyIntToText(int v)
{
AnsiString A = "";
for (int i=0; i<3; i++)
{
if (Numbers[i].value == v) A = Numbers[i].string;
}
return A;
}

//--------------------------------------------------------

So basically it resolves an int (1-3) to a string of sorts.

However lets say we have like 300 entries in the Numbers
structure or more....
Question: In terms of organizing my code.
1. Should I place this structure in a separate file and reference it
using an extern declaration, or
2. Should I keep it in the same file as the class and ......
2.a) Should I place it at the top above the class
or
2.b) Should I place it at the bottom and reference it using an extern
declaration.

[Benjamin Pratt]

IMO, MyIntToText would be better as a global scope function that you could
put in a seperate library unit. Then you would only need to include a header
that references the function declaration. Something such as:

std::string MyIntToText(int v)
{
static std::map<int, std::string> TextNums;
if(TextNums.empty())
{
TextNums.insert(std::make_pair(1, "One"));
TextNums.insert(std::make_pair(2, "Two"));
TextNums.insert(std::make_pair(3, "Three"));
}

return TextNums[v];
}

Another advantage this implementation has is that the number/text map is
delay-loaded; the map is only created the first time it is actually used.
After that, it is kept in memory for quick access.

For pure speed, a select...case may be faster, something like:


std::string MyIntToText(int v)
{
switch(v) {
case 1: return "One";
case 2: return "Two";
case 3: return "Three";
default: return "";
}
}


If expandability and/or multi-language support is needed, then loading the
int/text map from an external data file should be considered.

[Benjamin Pratt]

Also, have you thought about doing it programmatically? You could do it
something like this (untested, but you get the idea).


std::string MyIntToText(int v)
{
std::string text;
if(v >= 100 && v < 1000)
text += MyIntToText(v / 100) + " hundred ";

int tens = (v + 100) % 100;
int ones = (v + 10) % 10;

if(tens >= 90) text += "ninety";
else if(tens >= 80) text += "eighty";
else if(tens >= 70) text += "seventy";
else if(tens >= 60) text += "sixty";
else if(tens >= 50) text += "fifty";
else if(tens >= 40) text += "fourty";
else if(tens >= 30) text += "thirty";
else if(tens >= 20) text += "twenty";

if(tens >= 20)
{
if(ones > 0)
text += std::string("-") + MyIntToText(ones);
}
else if(tens == 19) text += "nineteen";
else if(tens == 18) text += "eighteen";
else if(tens == 17) text += "seventeen";
else if(tens == 16) text += "sixteen";
else if(tens == 15) text += "fifteen";
else if(tens == 14) text += "fourteen";
else if(tens == 13) text += "thirteen";
else if(tens == 12) text += "twelve";
else if(tens == 11) text += "eleven";
else if(tens == 10) text += "ten";
else if(ones == 9) text += "nine";
else if(ones == 8) text += "eight";
else if(ones == 7) text += "seven";
else if(ones == 6) text += "six";
else if(ones == 5) text += "five";
else if(ones == 4) text += "four";
else if(ones == 3) text += "three";
else if(ones == 2) text += "two";
else if(ones == 1) text += "one";
else if(v == 0) text += "zero";

return text;
}

[Colin B Maharaj]

This was not the best example for what I wanted to
do. In actuality I am writing a simple (yeah right)
program that deciphers RADIUS messages so the text
cannot really be generated, like this. It has to be
looked up.

But you suggestion is appreciated.
:)

Post by Benjamin Pratt
Also, have you thought about doing it programmatically? You could do it
something like this (untested, but you get the idea).
std::string MyIntToText(int v)
{
std::string text;
if(v >= 100 && v < 1000)
text += MyIntToText(v / 100) + " hundred ";
int tens = (v + 100) % 100;
int ones = (v + 10) % 10;
if(tens >= 90) text += "ninety";
else if(tens >= 80) text += "eighty";
else if(tens >= 70) text += "seventy";
else if(tens >= 60) text += "sixty";
else if(tens >= 50) text += "fifty";
else if(tens >= 40) text += "fourty";
else if(tens >= 30) text += "thirty";
else if(tens >= 20) text += "twenty";
if(tens >= 20)
{
if(ones > 0)
text += std::string("-") + MyIntToText(ones);
}
else if(tens == 19) text += "nineteen";
else if(tens == 18) text += "eighteen";
else if(tens == 17) text += "seventeen";
else if(tens == 16) text += "sixteen";
else if(tens == 15) text += "fifteen";
else if(tens == 14) text += "fourteen";
else if(tens == 13) text += "thirteen";
else if(tens == 12) text += "twelve";
else if(tens == 11) text += "eleven";
else if(tens == 10) text += "ten";
else if(ones == 9) text += "nine";
else if(ones == 8) text += "eight";
else if(ones == 7) text += "seven";
else if(ones == 6) text += "six";
else if(ones == 5) text += "five";
else if(ones == 4) text += "four";
else if(ones == 3) text += "three";
else if(ones == 2) text += "two";
else if(ones == 1) text += "one";
else if(v == 0) text += "zero";
return text;
}

[Jason Cipriani]

Post by Colin B Maharaj
Hi all,

Hi Colin.

Post by Colin B Maharaj
lets say I have some code like this

...

Post by Colin B Maharaj
NumbersStruct Numbers[] =
{
{ 1, " One " },
{ 2, " Two " },
{ 3, " Three " }
};
//--------------------------------------------------------

...

Post by Colin B Maharaj
So basically it resolves an int (1-3) to a string of sorts.
However lets say we have like 300 entries in the Numbers
structure or more....
Question: In terms of organizing my code.
1. Should I place this structure in a separate file and reference it using
an extern declaration, or
2. Should I keep it in the same file as the class and ......
2.a) Should I place it at the top above the class
or
2.b) Should I place it at the bottom and reference it using an extern
declaration.

Part of the answer depends on whether or not you want NumbersStruct and the
Numbers[] data itself to be visible outside the translation unit you use it
in. It sounds to me like you do not, so I will assume that. This limits your
options, then:

In that case, you should not do option 1 (and in fact, your example code
also suffers from this problem). If your Numbers[] array is only used in one
translation unit (i.e. object file), it is better to declare it static in
that object file, so that it has internal linkage and the symbol is not
exported. This is especially true if your code is part of a library, and
even more especially true if your symbol is a relatively mundane name such
as "Numbers" that does not exist in a unique namespace. That is, do this, in
the MyClass source file where MyIntToText is defined:

static NumbersStruct Numbers[] = { ... };

Also, if you do not intend to modify Numbers[], consider declaring it const
as well, the compiler will then make sure that you are not inadvertently
modifying any of it's data:

static const NumbersStruct Numbers[] = { ... };

Now that means that you can't do option 1 - separate translation unit with
extern declaration. However, you can still do a slight modification of
option 1 if you want (let's call it 1b): declare it as static in another
file and #include that file into the source file that uses it. No extern
declaration required, for example:

// --- NumbersTable.h ---

static const NumbersStruct Numbers[] = { ... };

// --- MyClass.cpp ---

#include "NumbersTable.h"

void f () {
int x = Numbers[0].value;
}

This is, of course, basically the same as option 2a, except you #include it
rather than typing it directly into the file.

Option 2b is not possible if Numbers[] is static and const. You must
initialize const non-member variables when you declare them. Static
variables can not be extern. AFAIK it is not possible to "forward declare" a
static variable, const or not, before it is initialized. Therefore you'd
have to put it at the top of the file.

In any case for a large table of numbers my vote goes to option 1b: static,
possibly const, but in a #included file used only in the one source file you
are using the table in. This keeps the data out of your MyClass source file
(because it's *probably* not the most important thing in that source file --
but if it is then it may make sense to keep it there of course), but also
clearly identifies it. This organization is also especially handy if you are
using a separate program to generate the data in the table. If you plan on
using your table in more than one source file then I would consider giving
it it's own .cpp file for the same reasons.

So really your questions boil down to:

1) Am I using it in just one source file and do I want to avoid polluting
the global external linkage namespace? If yes, make it static, if not, make
it "global" and forward declare it with extern.

2) Should I put it in another file and #include it, or put it in the same
source file as MyIntToText? Put it in another file if any of the following
is true:
- It is generated by another program.
- It is very large but is not the most significant thing in the source
file -- so it is distracting.
- If you just feel more comfortable having it in it's own file. Personal
preference and comfort is very important.

Hope that helps,

Jason

[Colin B Maharaj]

Thanks this has been good reading.....

Post by Jason Cipriani

Post by Colin B Maharaj
Hi all,

Hi Colin.

Post by Colin B Maharaj
lets say I have some code like this

[Remy Lebeau (TeamB)]

Post by Colin B Maharaj
lets say I have some code like this

<snip>

Post by Colin B Maharaj
So basically it resolves an int (1-3) to a string of sorts.

There is an easier way to do that without looping:

AnsiString MyClass::MyIntToText(int v)
{
if( (v >= 1) && (v <= 3) )
return Numbers[v-1].string;
return "";
}

Post by Colin B Maharaj
Should I place this structure in a separate file and reference
it using an extern declaration

You could. Or you could just keep it inside the .cpp file that actually
uses it. The compiler and linker do not really care where it is located, as
long as it can be found when needed. If you keep it in the same .cpp file,
I would suggest defining it at the bottom of the file so it is not too
cluttered.

Post by Colin B Maharaj
Should I place it at the bottom and reference it using an
extern declaration.

You don't need an 'extern' declaration for things that are located in the
same file.


Gambit

[Jason Cipriani]

Post by Remy Lebeau (TeamB)

Post by Colin B Maharaj
Should I place it at the bottom and reference it using an
extern declaration.

You don't need an 'extern' declaration for things that are located in the
same file.

For initialized non-member data you do; if a variable declaration isn't
extern then it must be initialized where it is declared, I think. So that
puts you in a weird situation if you are only using the data in one file and
you also want to put it at the bottom of the files.

Jason

[Chris Uzdavinis (TeamB)]

Post by Colin B Maharaj
Hi all,
lets say I have some code like this
//--------------------------------------------------------
struct NumbersStruct
{
int value;
char string[32];
};
NumbersStruct Numbers[] =
{
{ 1, " One " },
{ 2, " Two " },
{ 3, " Three " }
};

Um, ok. Already, however, I'd suggest you have an issue, maybe two.

1) 32 strings is the max. What happens when you need to go to 33, or
128? Recompile and redeploy the application? Ewww. That's an
aritficial limitiation and a pain to work with. Better, you should
load the array from a file, and build it to the proper size as
necessary.

2) You are building in formatting into the data, which means that the
data knows how it will be used. That's typically indicative of a
design flaw. It may work when it's only used in one way today, but
once you have a new use for it, you have possibly "wrongly
formatted" strings, and must strip out the spaces if you want to,
say, make normal sentences with numeric words in them.

3) if you put {0, "Zero"} as the first element, then you can use the
number as the index into the array, and even get rid of the numeric
value in the array.

Post by Colin B Maharaj
AnsiString MyClass::MyIntToText(int v)
{
AnsiString A = "";
for (int i=0; i<3; i++)
{
if (Numbers[i].value == v) A = Numbers[i].string;
}
return A;
}

Why loop? Are you planning on omitting certain numbers? Having an
array automatically gives a simple indexing function already.

Post by Colin B Maharaj
1. Should I place this structure in a separate file and reference it
using an extern declaration, or

I'd hide the implementation behind a functional interface. Then the
"how" is not exposed to clients. You just pass in a number, and get
back a string. "Magic."

Post by Colin B Maharaj
2. Should I keep it in the same file as the class and ......

No. This could be is a self-contained class or function that is
*used* by your class.

[Colin B Maharaj]

Hi Chris,
Thanks for the code analysis, however
the idea of the sample was on dealing
with an array of fix data that need
referring to later in my code.

The actual project was one to figure out
the RADIUS Accounting protocol, where I have
like large arrays of text that represents
the Vendor specific information and the
text representing that the rfc itself.
e.g. (read RFC2866)


-------------------------------------------------------------------------------------------
....raw radius packet in hex......
-------------------------------------------------------------------------------------------
0439030BA260924F4DB0E2162E00F4260406CB5F2C0A30303030303046371F06333030321A3800000009193268
3332332D73657475702D74696D653D31343A34333A31302E3237302055544320467269204D6172203232203230
30321A1A000000092114683332332D67772D69643D526F757465722E1A38000000091832683332332D636F6E66
2D69643D46364632373730432033434439313144362038323743443745362037343431454343431A1F00000009
1A19683332332D63616C6C2D6F726967696E3D616E737765721A20000000091B1A683332332D63616C6C2D7479
70653D54656C6570686F6E791A4100000009013B683332332D696E636F6D696E672D636F6E662D69643D463646
32373730432033434439313144362038323743443745362037343431454343431A1E0000000901187375627363
72696265723D526567756C61724C696E651A8000000009017A666561747572652D7673613D666E3A5457432C66
743A30332F32322F323030322031343A34333A31302E3237302C63676E3A333030322C63646E3A2C6672733A30
2C6669643A3232382C666369643A46364632373730433343443931314436383237434437453637343431454343
432C6C656749443A45302A06000000002B06000000002F06000000003006000000002E06000000001A3A000000
091C34683332332D636F6E6E6563742D74696D653D31343A34333A31332E3138312055544320467269204D6172
20323220323030321A3D000000091D37683332332D646973636F6E6E6563742D74696D653D31343A34333A3133
2E3138312055544320467269204D617220323220323030321A20000000091E1A683332332D646973636F6E6E65
63742D63617573653D31301A2300000009011D683332332D6976722D6F75743D5461726966663A556E6B6E6F77
6E1A1800000009011272656C656173652D736F757263653D311A1C000000091F16683332332D766F6963652D71
75616C6974793D300106333030322806000000023D0600000000050600000000570946585320332F3106060000
00010406AC11002429060000000F

-------------------------------------------------------------------------------------------
.....deciphered radius packet as text.....
-------------------------------------------------------------------------------------------
Radius Type : Accounting Request
Sequence Id : 57
Length Calc : 779
Length Given : 779
Authenticator : A260924F4DB0E2162E00F4260406CB5F
Normal RADIUS-Area
Acct-Session-Id [44] 10 000000F7
Calling-Station-Id [31] 6 3002
Vendor-Specific-Area [26]
H323 Setup Time [25] 50 h323-setup-time=14:43:10.270
UTC Fri Mar 22 2002
H323 GW Id [33] 20 h323-gw-id=Router.
H323 Conf Id [24] 50 h323-conf-id=F6F2770C 3CD911D6
827CD7E6 7441ECCC
H323 Call Origin [26] 25 h323-call-origin=answer
H323 Call Type [27] 26 h323-call-type=Telephony
Cisco AV Pair [1] 59 h323-incoming-conf-id=F6F2770C
3CD911D6 827CD7E6 7441ECCC
Cisco AV Pair [1] 24 subscriber=RegularLine
Cisco AV Pair [1] 122
feature-vsa=fn:TWC,ft:03/22/2002
14:43:10.270,cgn:3002,cdn:,frs:0,fid:228,fcid:F6F2770C3CD911D6827CD7E67441ECCC,legID:E0
Normal RADIUS-Area
Acct-Input-Octets [42] 6 0.0.0.0
Acct-Output-Octets [43] 6 0.0.0.0
Acct-Input-Packets [47] 6 0.0.0.0
Acct-Output-Packets [48] 6 0.0.0.0
Acct-Session-Time [46] 6 0.0.0.0
Vendor-Specific-Area [26]
H323 Connect Time [28] 52 h323-connect-time=14:43:13.181
UTC Fri Mar 22 2002
H323 Disconnect Time [29] 55
h323-disconnect-time=14:43:13.181 UTC Fri Mar 22 2002
H323 Disconnect Cause [30] 26 h323-disconnect-cause=10
Cisco AV Pair [1] 29 h323-ivr-out=Tariff:Unknown
Cisco AV Pair [1] 18 release-source=1
H323 Voice Quality [31] 22 h323-voice-quality=0
Normal RADIUS-Area
User-Name [1] 6 3002
Acct-Status-Type [40] 6 0.0.0.2
NAS-Port-Type [61] 6 0.0.0.0
NAS-Port [5] 6 0.0.0.0
NAS Port ID [87] 9 FXS 3/1
Service-Type [6] 6 0.0.0.1
NAS-IP-Address [4] 6 172.17.0.36
Acct-Delay-Time [41] 6 0.0.0.15

-------------------------------------------------------------------------------------------
In the deciphered text, the items at the left such as 'H323 Disconnect
Cause' is actually a byte value 40 so I have this massive structured
array with integers and text.

Hope you guys finally get it..
:)
However thanks for your responses..

Cheers!!

Post by Chris Uzdavinis (TeamB)

Post by Colin B Maharaj
Hi all,
lets say I have some code like this
//--------------------------------------------------------
struct NumbersStruct
{
int value;
char string[32];
};
NumbersStruct Numbers[] =
{
{ 1, " One " },
{ 2, " Two " },
{ 3, " Three " }
};

Um, ok. Already, however, I'd suggest you have an issue, maybe two.
1) 32 strings is the max. What happens when you need to go to 33, or
128? Recompile and redeploy the application? Ewww. That's an
aritficial limitiation and a pain to work with. Better, you should
load the array from a file, and build it to the proper size as
necessary.
2) You are building in formatting into the data, which means that the
data knows how it will be used. That's typically indicative of a
design flaw. It may work when it's only used in one way today, but
once you have a new use for it, you have possibly "wrongly
formatted" strings, and must strip out the spaces if you want to,
say, make normal sentences with numeric words in them.
3) if you put {0, "Zero"} as the first element, then you can use the
number as the index into the array, and even get rid of the numeric
value in the array.

Post by Colin B Maharaj
AnsiString MyClass::MyIntToText(int v)
{
AnsiString A = "";
for (int i=0; i<3; i++)
{
if (Numbers[i].value == v) A = Numbers[i].string;
}
return A;
}

Why loop? Are you planning on omitting certain numbers? Having an
array automatically gives a simple indexing function already.

Post by Colin B Maharaj
1. Should I place this structure in a separate file and reference it
using an extern declaration, or

I'd hide the implementation behind a functional interface. Then the
"how" is not exposed to clients. You just pass in a number, and get
back a string. "Magic."

Post by Colin B Maharaj
2. Should I keep it in the same file as the class and ......

No. This could be is a self-contained class or function that is
*used* by your class.

[Jason Cipriani]

Post by Colin B Maharaj
Hope you guys finally get it..
:)

Cut us some slack, we're only robots!

[Colin B Maharaj]

OK, I think I understand now...
:)
Robot - OFF!
:)

Post by Jason Cipriani

Post by Colin B Maharaj
Hope you guys finally get it..
:)

Cut us some slack, we're only robots!

[Jason Cipriani]

Post by Colin B Maharaj
OK, I think I understand now...
:)
Robot - OFF!

BEEEEEEEEooooooooooooooo......

[Jason Cipriani]

Post by Chris Uzdavinis (TeamB)

Post by Colin B Maharaj
struct NumbersStruct
{
int value;
char string[32];
};
NumbersStruct Numbers[] =
{
{ 1, " One " },
{ 2, " Two " },
{ 3, " Three " }
};

1) 32 strings is the max. What happens when you need to go to 33, or
128? Recompile and redeploy the application? Ewww. That's an
aritficial limitiation and a pain to work with. Better, you should
load the array from a file, and build it to the proper size as
necessary.

32 is the maximum length of a specific string. The maximum number of entries
in the table is 3. However, the OP specified that his example was purely an
illustration, and his application is actually using the table to map RADIUS
message codes to strings. I do not know much about the application but if
the mapping is standardized and known ahead of time, nothing is lost by
hard-coding the table into the EXE, and what is gained is not having to ship
an extra file around.

Post by Chris Uzdavinis (TeamB)
2) You are building in formatting into the data, which means that the
data knows how it will be used. That's typically indicative of a
design flaw. It may work when it's only used in one way today, but
once you have a new use for it, you have possibly "wrongly
formatted" strings, and must strip out the spaces if you want to,
say, make normal sentences with numeric words in them.

The numbers were only an example.

Post by Chris Uzdavinis (TeamB)
3) if you put {0, "Zero"} as the first element, then you can use the
number as the index into the array, and even get rid of the numeric
value in the array.

I suspect the lookup table he is actually using does not have that
convenient property.

Post by Chris Uzdavinis (TeamB)

Post by Colin B Maharaj
AnsiString MyClass::MyIntToText(int v)
{
AnsiString A = "";
for (int i=0; i<3; i++)
{
if (Numbers[i].value == v) A = Numbers[i].string;
}
return A;
}

Why loop? Are you planning on omitting certain numbers? Having an
array automatically gives a simple indexing function already.

The numbers are likely not accessible by index. However, Colin, you could
consider using an std::map<int,InfoStruct> or something similar, if your
lookup table is large. Hard-code an initial table of values and load them
into the map when your application initializes. This gives you a number of
benefits, including faster lookup times than a brute-force search, and the
ability to dynamically add new entries to the table if you need to in the
future (such as easily loading additional mappings from a file or some other
source, as Chris Uzdavinis mentioned above).

Post by Chris Uzdavinis (TeamB)

Post by Colin B Maharaj
1. Should I place this structure in a separate file and reference it
using an extern declaration, or

I'd hide the implementation behind a functional interface. Then the
"how" is not exposed to clients. You just pass in a number, and get
back a string. "Magic."

In the original example, the way I read it is clients would access the data
via MyIntToText, and the big array, while externally visible (that was part
of the question) wasn't being used by anything other than that function. The
question pertained more to how to organize the source code.


Jason