Code critique

[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

[Kevin Goodsell]

Re: Code critique

John Harrison wrote:
[color=blue]
> "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=green]
>>
>>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.
>[/color]

That seems reasonable. The only problem is if you have similar names in
multiple namespaces, but I've never heard of this being much of a
problem in reality.

-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]
> "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=green]
>>
>>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.
>[/color]

That seems reasonable. The only problem is if you have similar names in
multiple namespaces, but I've never heard of this being much of a
problem in reality.

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

[Martin Eisenberg]

Re: Code critique

Thanks, Kevin!

Kevin Goodsell wrote:
[color=blue]
> Martin Eisenberg wrote:[/color]
[color=blue][color=green]
>> using namespace std;[/color]
>
> This should be avoided, but is not terrible, particularly for
> small programs.[/color]

That's what I figured. I did it because of all the console usage; in
collatztree.cpp I qualified explicitly. But I guess it's Good to put
more specific declarations, as you suggest.
[color=blue][color=green]
>> 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]

Whoa :)
[color=blue][color=green]
>> bool prompt(const string& message)[/color][/color]
[color=blue]
> 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]

Good point. I'll do it like that.


Martin

--
Quidquid latine dictum sit, altum viditur.

[Martin Eisenberg]

Re: Code critique

Thanks, Kevin!

Kevin Goodsell wrote:
[color=blue]
> Martin Eisenberg wrote:[/color]
[color=blue][color=green]
>> using namespace std;[/color]
>
> This should be avoided, but is not terrible, particularly for
> small programs.[/color]

That's what I figured. I did it because of all the console usage; in
collatztree.cpp I qualified explicitly. But I guess it's Good to put
more specific declarations, as you suggest.
[color=blue][color=green]
>> 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]

Whoa :)
[color=blue][color=green]
>> bool prompt(const string& message)[/color][/color]
[color=blue]
> 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]

Good point. I'll do it like that.


Martin

--
Quidquid latine dictum sit, altum viditur.

[Martin Eisenberg]

Re: Code critique

I wrote further upthread:
[color=blue]
> cin >> c;
> #ifdef WIN32
> // Swallow Windows newline's second character.
> cin.ignore();
> #endif[/color]

John Harrison wrote:
[color=blue][color=green][color=darkred]
>>> This is a well known MSVC 6 bug, it was fixed in a service
>>> pack (which the OP should certainly get hold of)[/color][/color][/color]

I have SP5, so that would be SP6?
[color=blue][color=green][color=darkred]
>>> but you can also get this fix and more from here
>>>
>>> http://www.dinkumware.com/vc_fixes.html[/color][/color][/color]

I got those, but now realize that I failed to rebuild the library.
<groan> But my Uni department gives me Visual Studio 2003 for free,
so it's a moot point now.
[color=blue]
> 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.[/color]

Easy. Premises:
- newline has two characters on Windows and one on Unices.
- Every second call to the function excerpted at
the top does not wait for me to touch anything.
Conclusion using Modus Praematurus:
- I need to special-case for Windows.


Martin

--
- Buridan Revisited -
A developer works in a space spanned by two MS
products. When a problem occurs, will she ever
resolve to blame one and meet the schedule?

[Martin Eisenberg]

Re: Code critique

I wrote further upthread:
[color=blue]
> cin >> c;
> #ifdef WIN32
> // Swallow Windows newline's second character.
> cin.ignore();
> #endif[/color]

John Harrison wrote:
[color=blue][color=green][color=darkred]
>>> This is a well known MSVC 6 bug, it was fixed in a service
>>> pack (which the OP should certainly get hold of)[/color][/color][/color]

I have SP5, so that would be SP6?
[color=blue][color=green][color=darkred]
>>> but you can also get this fix and more from here
>>>
>>> http://www.dinkumware.com/vc_fixes.html[/color][/color][/color]

I got those, but now realize that I failed to rebuild the library.
<groan> But my Uni department gives me Visual Studio 2003 for free,
so it's a moot point now.
[color=blue]
> 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.[/color]

Easy. Premises:
- newline has two characters on Windows and one on Unices.
- Every second call to the function excerpted at
the top does not wait for me to touch anything.
Conclusion using Modus Praematurus:
- I need to special-case for Windows.


Martin

--
- Buridan Revisited -
A developer works in a space spanned by two MS
products. When a problem occurs, will she ever
resolve to blame one and meet the schedule?

[Martin Eisenberg]

Re: Code critique

Ivan, thanks for the response.

Ivan Vecerina wrote:
[color=blue]
> cin.ignore( std::numeric_li mits<int>::max( ), '\n' );[/color]

I've seen this before, but it didn't stick. To get really picky for a
moment, would size_t or streamsize or something be more fitting than
int?
[color=blue][color=green]
>> typedef void (*WriteBest)(co nst Best&, unsigned nodeCount);
>> typedef bool (*CheckUserBrea k)();
>> typedef const Node* PNode;[/color][/color]
[color=blue]
> 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]

What are some pitfalls that this abstraction would promote?
[color=blue][color=green]
>> typedef std::vector<Nod e> Level;
>> typedef std::vector<Lev el*> Levels;[/color][/color]
[color=blue]
> A container of naked pointers ?[/color]

First I had a vector of auto_ptr. When I read why that doesn't work I
changed it to pointers.
[color=blue]
> 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).[/color]

Hmm, using a list here right away actually seems very sensible to me.
I don't need random access, it's less typing, and it avoids the
potential reference invalidation you pointed out below.
[color=blue]
> - 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.[/color]

Did you intend levelPool to be of a class of my own creation which
takes ownership of inserted pointers? If not, what's the difference
to the other quoted option?
[color=blue][color=green]
>> CollatzTree::Co llatzTree( // ...
>> {
>> PNode pn = addInitial(1, 0);[/color]
> ...[color=green]
>> }
>>
>> CollatzTree::~C ollatzTree()
>> {
>> Levels::iterato r pl = levels_.begin() ;
>> for(; pl != levels_.end(); ++pl) delete *pl;
>> }[/color][/color]
[color=blue]
> 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]

Would it suffice to wrap the current ctor body in a try block and
then in the handler, put the above dtor code and rethrow? Or maybe
build a local structure and assign that to the member if all is well
in the end?
[color=blue][color=green]
>> 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.[/color]

Level* pl = new Level();
try {
levels_.push_ba ck(pl);
} catch(...) {
delete pl;
throw;
}

Is that reasonable?
[color=blue]
> Also a serious problem: levels_.push_ba ck() might
> reallocate the contents of the levels_ vector,
> and invalidate the reference you stored in oldTop[/color]

Indeed. Good you caught that one.
[color=blue]
> Well, this is as far as my attention span goes...[/color]

You've already been very helpful, and you're in the
pole position so far ;) Thanks again.


Martin

--
going in a direction conventionally the opposite of down

[Martin Eisenberg]

Re: Code critique

Ivan, thanks for the response.

Ivan Vecerina wrote:
[color=blue]
> cin.ignore( std::numeric_li mits<int>::max( ), '\n' );[/color]

I've seen this before, but it didn't stick. To get really picky for a
moment, would size_t or streamsize or something be more fitting than
int?
[color=blue][color=green]
>> typedef void (*WriteBest)(co nst Best&, unsigned nodeCount);
>> typedef bool (*CheckUserBrea k)();
>> typedef const Node* PNode;[/color][/color]
[color=blue]
> 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]

What are some pitfalls that this abstraction would promote?
[color=blue][color=green]
>> typedef std::vector<Nod e> Level;
>> typedef std::vector<Lev el*> Levels;[/color][/color]
[color=blue]
> A container of naked pointers ?[/color]

First I had a vector of auto_ptr. When I read why that doesn't work I
changed it to pointers.
[color=blue]
> 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).[/color]

Hmm, using a list here right away actually seems very sensible to me.
I don't need random access, it's less typing, and it avoids the
potential reference invalidation you pointed out below.
[color=blue]
> - 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.[/color]

Did you intend levelPool to be of a class of my own creation which
takes ownership of inserted pointers? If not, what's the difference
to the other quoted option?
[color=blue][color=green]
>> CollatzTree::Co llatzTree( // ...
>> {
>> PNode pn = addInitial(1, 0);[/color]
> ...[color=green]
>> }
>>
>> CollatzTree::~C ollatzTree()
>> {
>> Levels::iterato r pl = levels_.begin() ;
>> for(; pl != levels_.end(); ++pl) delete *pl;
>> }[/color][/color]
[color=blue]
> 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]

Would it suffice to wrap the current ctor body in a try block and
then in the handler, put the above dtor code and rethrow? Or maybe
build a local structure and assign that to the member if all is well
in the end?
[color=blue][color=green]
>> 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.[/color]

Level* pl = new Level();
try {
levels_.push_ba ck(pl);
} catch(...) {
delete pl;
throw;
}

Is that reasonable?
[color=blue]
> Also a serious problem: levels_.push_ba ck() might
> reallocate the contents of the levels_ vector,
> and invalidate the reference you stored in oldTop[/color]

Indeed. Good you caught that one.
[color=blue]
> Well, this is as far as my attention span goes...[/color]

You've already been very helpful, and you're in the
pole position so far ;) Thanks again.


Martin

--
going in a direction conventionally the opposite of down

[Buster]

Re: Code critique

Martin Eisenberg wrote:
[color=blue]
> Ivan, thanks for the response.
>
> Ivan Vecerina wrote:
>[color=green]
>>cin.ignore( std::numeric_li mits<int>::max( ), '\n' );[/color]
>
> I've seen this before, but it didn't stick. To get really picky for a
> moment, would size_t or streamsize or something be more fitting than
> int?[/color]

'int' is specified in the 1998 standard, BUT see

http://anubis.dkuug.dk/jtc1/sc22/wg21/docs/lwg-defects.html#172

(Thanks to Kevin Goodsell for pointing this out to me:

http://groups.google.com/groups?selm=9Xs8c.55717%24aT1.42015%40newsread1.news.pas.earthlink.net

..)

--
Regards,
Buster.

[Buster]

Re: Code critique

Martin Eisenberg wrote:
[color=blue]
> Ivan, thanks for the response.
>
> Ivan Vecerina wrote:
>[color=green]
>>cin.ignore( std::numeric_li mits<int>::max( ), '\n' );[/color]
>
> I've seen this before, but it didn't stick. To get really picky for a
> moment, would size_t or streamsize or something be more fitting than
> int?[/color]

'int' is specified in the 1998 standard, BUT see

http://anubis.dkuug.dk/jtc1/sc22/wg21/docs/lwg-defects.html#172

(Thanks to Kevin Goodsell for pointing this out to me:

http://groups.google.com/groups?selm=9Xs8c.55717%24aT1.42015%40newsread1.news.pas.earthlink.net

..)

--
Regards,
Buster.

[Kevin Goodsell]

Re: Code critique

Martin Eisenberg wrote:
[color=blue][color=green]
>>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.[/color]
>
>
> Easy. Premises:
> - newline has two characters on Windows and one on Unices.[/color]

This is not true, from a C++ perspective. End-of-line in a text stream
is always indicated with a single '\n' character. The implementation is
responsible for converting to and from the underlying representation
used by the system, so your code can (and should) ignore it.
[color=blue]
> - Every second call to the function excerpted at
> the top does not wait for me to touch anything.[/color]

This sounds like either a bug in your implementation or possibly a
character left in the stream from some other part of the program.
[color=blue]
> Conclusion using Modus Praematurus:
> - I need to special-case for Windows.[/color]

You shouldn't. I tried this simple test program using your code:

#include <ios>
#include <iostream>
#include <string>

using namespace std;

bool prompt(const string& message)
{
char c;
cout << message << " (y/n <Enter>) ";
cin >> c;
// cin.ignore();
cout << '\n';
return c == 'y';
}

int main()
{
cout << boolalpha;

while (true)
{
cout << "prompt() returned " << prompt("") << endl;
}
return 0;
}

In Visual C++ 6 (service pack 5, I think). It worked as expected.

-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

Martin Eisenberg wrote:
[color=blue][color=green]
>>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.[/color]
>
>
> Easy. Premises:
> - newline has two characters on Windows and one on Unices.[/color]

This is not true, from a C++ perspective. End-of-line in a text stream
is always indicated with a single '\n' character. The implementation is
responsible for converting to and from the underlying representation
used by the system, so your code can (and should) ignore it.
[color=blue]
> - Every second call to the function excerpted at
> the top does not wait for me to touch anything.[/color]

This sounds like either a bug in your implementation or possibly a
character left in the stream from some other part of the program.
[color=blue]
> Conclusion using Modus Praematurus:
> - I need to special-case for Windows.[/color]

You shouldn't. I tried this simple test program using your code:

#include <ios>
#include <iostream>
#include <string>

using namespace std;

bool prompt(const string& message)
{
char c;
cout << message << " (y/n <Enter>) ";
cin >> c;
// cin.ignore();
cout << '\n';
return c == 'y';
}

int main()
{
cout << boolalpha;

while (true)
{
cout << "prompt() returned " << prompt("") << endl;
}
return 0;
}

In Visual C++ 6 (service pack 5, I think). It worked as expected.

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

[Mike Smith]

Re: Code critique

John Harrison wrote:[color=blue][color=green]
>>This should be avoided, but is not terrible, particularly for small
>>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]

How so? I use this all the time.

--
Mike Smith

[Mike Smith]

Re: Code critique

John Harrison wrote:[color=blue][color=green]
>>This should be avoided, but is not terrible, particularly for small
>>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]

How so? I use this all the time.

--
Mike Smith