Code critique

[John Harrison]

Re: Code critique

> This should be avoided, but is not terrible, particularly for small[color=blue]
> programs. Generally, import only what you need where you need it, so you
> could use
>
> using std::string;
> using std::vector;
>[/color]

Unfortunately MSVC 6 does not implement this usage correctly.
[color=blue]
> and you could even put these inside the functions that use those items.
>[/color]

Has E Robert Tisdale got to you? It's not advice I would give.
[color=blue][color=green]
> >
> > const string KSeqFile = "collatz.tx t";
> > const unsigned KCheckInterval = 10000000;[/color]
>
> This value is much too large to portably put in an unsigned int. The
> portable maximum for unsigned int is 65535. Better make it unsigned long.
>[color=green]
> >
> > //-----------------------------------------------
> >
> > void awaitEnter()
> > {
> > char c[2];
> > cout << "Program finished. Hit <Enter> to close.";
> > cin.getline(c, 2);
> > }
> >
> > bool prompt(const string& message)
> > {
> > char c;
> > cout << message << " (y/n <Enter>) ";
> > cin >> c;
> > #ifdef WIN32
> > cin.ignore(); // Swallow Windows newline's second character.
> > #endif[/color]
>
> Eh? I don't think this is right. This should be the same, Windows or
> not. Besides that, you can't control how many characters are actually
> entered, so you should probably extract the entire line, and check the
> first character.[/color]

This is a well known MSVC 6 bug, it was fixed in a service pack (which the
OP should certainly get hold of) but you can also get this fix and more from
here

http://www.dinkumware.com/vc_fixes.html

john

[John Harrison]

Re: Code critique

> This should be avoided, but is not terrible, particularly for small[color=blue]
> programs. Generally, import only what you need where you need it, so you
> could use
>
> using std::string;
> using std::vector;
>[/color]

Unfortunately MSVC 6 does not implement this usage correctly.
[color=blue]
> and you could even put these inside the functions that use those items.
>[/color]

Has E Robert Tisdale got to you? It's not advice I would give.
[color=blue][color=green]
> >
> > const string KSeqFile = "collatz.tx t";
> > const unsigned KCheckInterval = 10000000;[/color]
>
> This value is much too large to portably put in an unsigned int. The
> portable maximum for unsigned int is 65535. Better make it unsigned long.
>[color=green]
> >
> > //-----------------------------------------------
> >
> > void awaitEnter()
> > {
> > char c[2];
> > cout << "Program finished. Hit <Enter> to close.";
> > cin.getline(c, 2);
> > }
> >
> > bool prompt(const string& message)
> > {
> > char c;
> > cout << message << " (y/n <Enter>) ";
> > cin >> c;
> > #ifdef WIN32
> > cin.ignore(); // Swallow Windows newline's second character.
> > #endif[/color]
>
> Eh? I don't think this is right. This should be the same, Windows or
> not. Besides that, you can't control how many characters are actually
> entered, so you should probably extract the entire line, and check the
> first character.[/color]

This is a well known MSVC 6 bug, it was fixed in a service pack (which the
OP should certainly get hold of) but you can also get this fix and more from
here

http://www.dinkumware.com/vc_fixes.html

john

[Ivan Vecerina]

Re: Code critique

"Martin Eisenberg" <martin.eisenbe rgNOS@PAMudo.ed u> wrote in message
news:1081296111 .489512@ostenbe rg.wh.uni-dortmund.de...
....[color=blue]
> #ifdef _MSC_VER
> // "identifier was truncated to '255' characters in the debug information"
> #pragma warning(disable : 4786)
> #endif[/color]
Note that you can disable such warnings in the command line,
or in the project's settings. This keeps implementation-
specific stuff out of your source code.
[color=blue]
> void awaitEnter()
> {
> char c[2];[/color]
....[color=blue]
> cin.getline(c, 2);[/color]
The following is a more explicit way to do this:
cin.ignore( std::numeric_li mits<int>::max( ), '\n' );
(first value is a max # of characters: you could
probably use any large integer as well)
[color=blue]
> bool prompt(const string& message)
> {
> char c;
> cout << message << " (y/n <Enter>) ";
> cin >> c;
> #ifdef WIN32
> cin.ignore(); // Swallow Windows newline's second character.[/color]
Needed? The stream library normally strips-off the
windows-specific '\r' (any platform-specific line termination
is converted to '\n' when it text mode).
Maybe you want to use 'getline'. Or ignore all characters
to the end of line (inclusive) -- see the call I posted above.
In any case, no Windows-specific handling should be needed here.

....[color=blue]
> typedef void (*WriteBest)(co nst Best&, unsigned nodeCount);
> typedef bool (*CheckUserBrea k)();
> typedef const Node* PNode;[/color]
The first two typedefs are very reasonable, but I would
recommend dropping PNode: It is usually best to be
explicit about pointers and their const-qualifications
-- use Node* or Node const* instead.
[color=blue]
> typedef std::vector<Nod e> Level;
> typedef std::vector<Lev el*> Levels;[/color]
A container of naked pointers ?
This smells like memory leaks are going to happen
( unless you are using some special type of
memory allocation policy ).
Recommendations :
- use std::vector<Lev el> unless performance problem
is established. Consider std::list<Level >
or std::deque<Leve l> also (may address performance
issues you observe).
- use std::vector<boo st::shared_ptr< Level>> : will
manage the memory deallocation for you automatically.
shared_ptr can be found on www.boost.org , and is
expected to be part of the next revision of the
C++ standard.
- Use a container such as std::list or std::deque
to store any objects you allocate -- i.e. replace:
Level* p = new Level();
with
levelPool.push_ back( Level() );
Level* p = &levelPool.back ();
after adding a data member such as:
std::deque<Leve l> levelPool;
At destruction time, the 'levelPool' container
will safely destroy and deallocate all Level instances.
Other schemes are possible, but these 3 are easy to use
and will improve the safety of your code.
(even though this might not matter for this application).
[color=blue]
> CollatzTree::Co llatzTree(Colla tzTree::WriteBe st writeBest,
> CollatzTree::Ch eckUserBreak checkUserBreak, unsigned checkInterval)
> : writeBest_(writ eBest), checkUserBreak_ (checkUserBreak ),
> checkInterval_( checkInterval), nodeCount_(4), isFinished_(fal se)
> {
> PNode pn = addInitial(1, 0);[/color]
....[color=blue]
> }
>
> CollatzTree::~C ollatzTree()
> {
> Levels::iterato r pl = levels_.begin() ;
> for(; pl != levels_.end(); ++pl) delete *pl;
> }[/color]
Infortunately, this is not enough to avoid all memory leaks.
For example, if your constructor (which adds items) fails
with an exception (e.g. during a memory allocation), objects
will be leaked.
[color=blue]
> /* The method addLevel() finds the numbers that a single
> Collatz iteration takes to those in the previous top level.
> It controls the new level's capacity to avoid memory waste.
> */
>
> void CollatzTree::ad dLevel()
> {
> if(isFinished_) return;
> const Level& oldTop = *levels_.back() ;
> levels_.push_ba ck(new Level());
> levels_.back()->reserve(getNew LevelSize());[/color]

Here too, if the push_back() call fails, you will be
left with a memory leak.

Also a serious problem: levels_.push_ba ck() might
reallocate the contents of the levels_ vector,
and invalidate the reference you stored in oldTop
(unless you make sure to first call 'reserve').
Suggestion: initialize oldTop after the push_back:
const Level& oldTop = levels_[ levels_.size()-2 ];



Well, this is as far as my attention span goes...

I hope this helps,
Ivan
--
http://ivan.vecerina.com/contact/?subject=NG_POST <- e-mail contact form
Brainbench MVP for C++ <> http://www.brainbench.com

[Ivan Vecerina]

Re: Code critique

"Martin Eisenberg" <martin.eisenbe rgNOS@PAMudo.ed u> wrote in message
news:1081296111 .489512@ostenbe rg.wh.uni-dortmund.de...
....[color=blue]
> #ifdef _MSC_VER
> // "identifier was truncated to '255' characters in the debug information"
> #pragma warning(disable : 4786)
> #endif[/color]
Note that you can disable such warnings in the command line,
or in the project's settings. This keeps implementation-
specific stuff out of your source code.
[color=blue]
> void awaitEnter()
> {
> char c[2];[/color]
....[color=blue]
> cin.getline(c, 2);[/color]
The following is a more explicit way to do this:
cin.ignore( std::numeric_li mits<int>::max( ), '\n' );
(first value is a max # of characters: you could
probably use any large integer as well)
[color=blue]
> bool prompt(const string& message)
> {
> char c;
> cout << message << " (y/n <Enter>) ";
> cin >> c;
> #ifdef WIN32
> cin.ignore(); // Swallow Windows newline's second character.[/color]
Needed? The stream library normally strips-off the
windows-specific '\r' (any platform-specific line termination
is converted to '\n' when it text mode).
Maybe you want to use 'getline'. Or ignore all characters
to the end of line (inclusive) -- see the call I posted above.
In any case, no Windows-specific handling should be needed here.

....[color=blue]
> typedef void (*WriteBest)(co nst Best&, unsigned nodeCount);
> typedef bool (*CheckUserBrea k)();
> typedef const Node* PNode;[/color]
The first two typedefs are very reasonable, but I would
recommend dropping PNode: It is usually best to be
explicit about pointers and their const-qualifications
-- use Node* or Node const* instead.
[color=blue]
> typedef std::vector<Nod e> Level;
> typedef std::vector<Lev el*> Levels;[/color]
A container of naked pointers ?
This smells like memory leaks are going to happen
( unless you are using some special type of
memory allocation policy ).
Recommendations :
- use std::vector<Lev el> unless performance problem
is established. Consider std::list<Level >
or std::deque<Leve l> also (may address performance
issues you observe).
- use std::vector<boo st::shared_ptr< Level>> : will
manage the memory deallocation for you automatically.
shared_ptr can be found on www.boost.org , and is
expected to be part of the next revision of the
C++ standard.
- Use a container such as std::list or std::deque
to store any objects you allocate -- i.e. replace:
Level* p = new Level();
with
levelPool.push_ back( Level() );
Level* p = &levelPool.back ();
after adding a data member such as:
std::deque<Leve l> levelPool;
At destruction time, the 'levelPool' container
will safely destroy and deallocate all Level instances.
Other schemes are possible, but these 3 are easy to use
and will improve the safety of your code.
(even though this might not matter for this application).
[color=blue]
> CollatzTree::Co llatzTree(Colla tzTree::WriteBe st writeBest,
> CollatzTree::Ch eckUserBreak checkUserBreak, unsigned checkInterval)
> : writeBest_(writ eBest), checkUserBreak_ (checkUserBreak ),
> checkInterval_( checkInterval), nodeCount_(4), isFinished_(fal se)
> {
> PNode pn = addInitial(1, 0);[/color]
....[color=blue]
> }
>
> CollatzTree::~C ollatzTree()
> {
> Levels::iterato r pl = levels_.begin() ;
> for(; pl != levels_.end(); ++pl) delete *pl;
> }[/color]
Infortunately, this is not enough to avoid all memory leaks.
For example, if your constructor (which adds items) fails
with an exception (e.g. during a memory allocation), objects
will be leaked.
[color=blue]
> /* The method addLevel() finds the numbers that a single
> Collatz iteration takes to those in the previous top level.
> It controls the new level's capacity to avoid memory waste.
> */
>
> void CollatzTree::ad dLevel()
> {
> if(isFinished_) return;
> const Level& oldTop = *levels_.back() ;
> levels_.push_ba ck(new Level());
> levels_.back()->reserve(getNew LevelSize());[/color]

Here too, if the push_back() call fails, you will be
left with a memory leak.

Also a serious problem: levels_.push_ba ck() might
reallocate the contents of the levels_ vector,
and invalidate the reference you stored in oldTop
(unless you make sure to first call 'reserve').
Suggestion: initialize oldTop after the push_back:
const Level& oldTop = levels_[ levels_.size()-2 ];



Well, this is as far as my attention span goes...

I hope this helps,
Ivan
--
http://ivan.vecerina.com/contact/?subject=NG_POST <- e-mail contact form
Brainbench MVP for C++ <> http://www.brainbench.com

[E. Robert Tisdale]

Re: Code critique

John Harrison wrote:
[color=blue][color=green]
>>This should be avoided, but is not terrible,
>>particularl y for small programs. Generally,
>>import only what you need where you need it, so you could use
>>
>> using std::string;
>> using std::vector;
>>
>>and you could even put these inside the functions that use those items.[/color]
>
> It's not advice I would give.[/color]

And just exactly what advice would you give?

[E. Robert Tisdale]

Re: Code critique

John Harrison wrote:
[color=blue][color=green]
>>This should be avoided, but is not terrible,
>>particularl y for small programs. Generally,
>>import only what you need where you need it, so you could use
>>
>> using std::string;
>> using std::vector;
>>
>>and you could even put these inside the functions that use those items.[/color]
>
> It's not advice I would give.[/color]

And just exactly what advice would you give?

[John Harrison]

Re: Code critique


"E. Robert Tisdale" <E.Robert.Tisda le@jpl.nasa.gov > wrote in message
news:40739564.2 010704@jpl.nasa .gov...[color=blue]
> John Harrison wrote:
>[color=green][color=darkred]
> >>This should be avoided, but is not terrible,
> >>particularl y for small programs. Generally,
> >>import only what you need where you need it, so you could use
> >>
> >> using std::string;
> >> using std::vector;
> >>
> >>and you could even put these inside the functions that use those items.[/color]
> >
> > It's not advice I would give.[/color]
>
> And just exactly what advice would you give?
>[/color]

Put all using ... at the top of the file, after all the includes.

john

[John Harrison]

Re: Code critique


"E. Robert Tisdale" <E.Robert.Tisda le@jpl.nasa.gov > wrote in message
news:40739564.2 010704@jpl.nasa .gov...[color=blue]
> John Harrison wrote:
>[color=green][color=darkred]
> >>This should be avoided, but is not terrible,
> >>particularl y for small programs. Generally,
> >>import only what you need where you need it, so you could use
> >>
> >> using std::string;
> >> using std::vector;
> >>
> >>and you could even put these inside the functions that use those items.[/color]
> >
> > It's not advice I would give.[/color]
>
> And just exactly what advice would you give?
>[/color]

Put all using ... at the top of the file, after all the includes.

john

[David Harmon]

Re: Code critique

On Wed, 7 Apr 2004 06:34:42 +0100 in comp.lang.c++, "John Harrison"
<john_andronicu s@hotmail.com> wrote,[color=blue][color=green][color=darkred]
>> > cin >> c;
>> > #ifdef WIN32
>> > cin.ignore(); // Swallow Windows newline's second character.
>> > #endif[/color]
>>
>> Eh? I don't think this is right. This should be the same, Windows or
>> not. Besides that, you can't control how many characters are actually
>> entered, so you should probably extract the entire line, and check the
>> first character.[/color]
>
>This is a well known MSVC 6 bug, it was fixed in a service pack (which the
>OP should certainly get hold of) but you can also get this fix and more from
>here
>
>http://www.dinkumware.com/vc_fixes.html[/color]

I don't think so (I could be wrong.) The problem I remember there was
with getline() and did not affect operator>>.

[David Harmon]

Re: Code critique

On Wed, 7 Apr 2004 06:34:42 +0100 in comp.lang.c++, "John Harrison"
<john_andronicu s@hotmail.com> wrote,[color=blue][color=green][color=darkred]
>> > cin >> c;
>> > #ifdef WIN32
>> > cin.ignore(); // Swallow Windows newline's second character.
>> > #endif[/color]
>>
>> Eh? I don't think this is right. This should be the same, Windows or
>> not. Besides that, you can't control how many characters are actually
>> entered, so you should probably extract the entire line, and check the
>> first character.[/color]
>
>This is a well known MSVC 6 bug, it was fixed in a service pack (which the
>OP should certainly get hold of) but you can also get this fix and more from
>here
>
>http://www.dinkumware.com/vc_fixes.html[/color]

I don't think so (I could be wrong.) The problem I remember there was
with getline() and did not affect operator>>.

[John Harrison]

Re: Code critique


"David Harmon" <source@netcom. com> wrote in message
news:409d97ec.9 0147801@news.we st.earthlink.ne t...[color=blue]
> On Wed, 7 Apr 2004 06:34:42 +0100 in comp.lang.c++, "John Harrison"
> <john_andronicu s@hotmail.com> wrote,[color=green][color=darkred]
> >> > cin >> c;
> >> > #ifdef WIN32
> >> > cin.ignore(); // Swallow Windows newline's second character.
> >> > #endif
> >>
> >> Eh? I don't think this is right. This should be the same, Windows or
> >> not. Besides that, you can't control how many characters are actually
> >> entered, so you should probably extract the entire line, and check the
> >> first character.[/color]
> >
> >This is a well known MSVC 6 bug, it was fixed in a service pack (which[/color][/color]
the[color=blue][color=green]
> >OP should certainly get hold of) but you can also get this fix and more[/color][/color]
from[color=blue][color=green]
> >here
> >
> >http://www.dinkumware.com/vc_fixes.html[/color]
>
> I don't think so (I could be wrong.) The problem I remember there was
> with getline() and did not affect operator>>.
>[/color]

Yes I think you are right.

It would be interesting to know what the OP thought was the problem with
WIN32 systems in particular. As you say the code shouldn't have to be
different for Windows.

john

[John Harrison]

Re: Code critique


"David Harmon" <source@netcom. com> wrote in message
news:409d97ec.9 0147801@news.we st.earthlink.ne t...[color=blue]
> On Wed, 7 Apr 2004 06:34:42 +0100 in comp.lang.c++, "John Harrison"
> <john_andronicu s@hotmail.com> wrote,[color=green][color=darkred]
> >> > cin >> c;
> >> > #ifdef WIN32
> >> > cin.ignore(); // Swallow Windows newline's second character.
> >> > #endif
> >>
> >> Eh? I don't think this is right. This should be the same, Windows or
> >> not. Besides that, you can't control how many characters are actually
> >> entered, so you should probably extract the entire line, and check the
> >> first character.[/color]
> >
> >This is a well known MSVC 6 bug, it was fixed in a service pack (which[/color][/color]
the[color=blue][color=green]
> >OP should certainly get hold of) but you can also get this fix and more[/color][/color]
from[color=blue][color=green]
> >here
> >
> >http://www.dinkumware.com/vc_fixes.html[/color]
>
> I don't think so (I could be wrong.) The problem I remember there was
> with getline() and did not affect operator>>.
>[/color]

Yes I think you are right.

It would be interesting to know what the OP thought was the problem with
WIN32 systems in particular. As you say the code shouldn't have to be
different for Windows.

john

[Kevin Goodsell]

Re: Code critique

John Harrison wrote:[color=blue]
> "E. Robert Tisdale" <E.Robert.Tisda le@jpl.nasa.gov > wrote in message
> news:40739564.2 010704@jpl.nasa .gov...
>[color=green]
>>John Harrison wrote:
>>
>>[color=darkred]
>>>>This should be avoided, but is not terrible,
>>>>particularl y for small programs. Generally,
>>>>import only what you need where you need it, so you could use
>>>>
>>>>using std::string;
>>>>using std::vector;
>>>>
>>>>and you could even put these inside the functions that use those items.
>>>
>>>It's not advice I would give.[/color]
>>
>>And just exactly what advice would you give?
>>[/color]
>
>
> Put all using ... at the top of the file, after all the includes.
>[/color]

Sure, that works well enough most of the time. But you /could/ restrict
it to individual functions, if you wanted to.

-Kevin
--
My email address is valid, but changes periodically.
To contact me please use the address from a recent posting.

[Kevin Goodsell]

Re: Code critique

John Harrison wrote:[color=blue]
> "E. Robert Tisdale" <E.Robert.Tisda le@jpl.nasa.gov > wrote in message
> news:40739564.2 010704@jpl.nasa .gov...
>[color=green]
>>John Harrison wrote:
>>
>>[color=darkred]
>>>>This should be avoided, but is not terrible,
>>>>particularl y for small programs. Generally,
>>>>import only what you need where you need it, so you could use
>>>>
>>>>using std::string;
>>>>using std::vector;
>>>>
>>>>and you could even put these inside the functions that use those items.
>>>
>>>It's not advice I would give.[/color]
>>
>>And just exactly what advice would you give?
>>[/color]
>
>
> Put all using ... at the top of the file, after all the includes.
>[/color]

Sure, that works well enough most of the time. But you /could/ restrict
it to individual functions, if you wanted to.

-Kevin
--
My email address is valid, but changes periodically.
To contact me please use the address from a recent posting.

[John Harrison]

Re: Code critique


"Kevin Goodsell" <usenet2.spamfr ee.fusion@never box.com> wrote in message
news:%DNcc.1916 7$lt2.5875@news read1.news.pas. earthlink.net.. .[color=blue]
> John Harrison wrote:[color=green]
> > "E. Robert Tisdale" <E.Robert.Tisda le@jpl.nasa.gov > wrote in message
> > news:40739564.2 010704@jpl.nasa .gov...
> >[color=darkred]
> >>John Harrison wrote:
> >>
> >>
> >>>>This should be avoided, but is not terrible,
> >>>>particularl y for small programs. Generally,
> >>>>import only what you need where you need it, so you could use
> >>>>
> >>>>using std::string;
> >>>>using std::vector;
> >>>>
> >>>>and you could even put these inside the functions that use those[/color][/color][/color]
items.[color=blue][color=green][color=darkred]
> >>>
> >>>It's not advice I would give.
> >>
> >>And just exactly what advice would you give?
> >>[/color]
> >
> >
> > Put all using ... at the top of the file, after all the includes.
> >[/color]
>
> Sure, that works well enough most of the time. But you /could/ restrict
> it to individual functions, if you wanted to.
>[/color]

I'm not denying it, it's just something I would almost never do.

Since generally speaking using ... refers to names that have been included
from a header file, it makes sense to me to put that declaration/directive
(which is it?) close to the header file it refers to.

john