Code critique

[Buster]

Ivan, thanks for the response.



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?

'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$aT1.42015@newsread1.news.pas.earthlink.net

..)

 

[Kevin Goodsell]

Easy. Premises:
- newline has two characters on Windows and one on Unices.

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.

- Every second call to the function excerpted at
the top does not wait for me to touch anything.

This sounds like either a bug in your implementation or possibly a
character left in the stream from some other part of the program.

Conclusion using Modus Praematurus:
- I need to special-case for Windows.

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

 

[Mike Smith]

Unfortunately MSVC 6 does not implement this usage correctly.

How so? I use this all the time.

 

[Martin Eisenberg]

Martin Eisenberg wrote:

You shouldn't.

You did notice that was tongue-in-cheek, yes?


Martin

 

[Kevin Goodsell]

Kevin Goodsell wrote:




You did notice that was tongue-in-cheek, yes?

No, but my Latin is rusty.

-Kevin

 

[Martin Eisenberg]

No, but my Latin is rusty.

The fun is, I don't know Latin at all! I explained how I came to the
(erroneous, I know that now) conclusion that the spurious newline
thing was a Windows problem, with a (probably ill-formed play on
the inference rule Modus Ponens and like animals.


Martin

 

[Ivan Vecerina]

Hi Martin,

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?

(thanks to Buster for pointing out the related fix in the C++ standard)

What are some pitfalls that this abstraction would promote?

This might be seen as a matter of taste.
However, creating a new name (PNode) without actually
simplifying anything is more error prone than anything else.
I would have expected PNode to be a Node*, not a const Node*.
Other than the 'less typing' argument, is there any reason
not to just use "const Node*" instead of "PNode" ? (i.e. for clarity)

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?

No: I intended it to be a standard container such as deque.
The aim is to keep using a container of pointers, which you
can efficiently sort, shuffle, move etc.
However, instead of allocating the pointed-to objects
on the heap, you allocate them within a separate container.
When this separate container is destroyed, you get
the equivalent of garbage collection for free.

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?

Adding a try..catch block usually isn't the simplest solution,
but it is an option. Alternatively, you could try using
a custom-made 'owning' container of pointers.
But I think the previously mentioned alternatives are better.

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

Is that reasonable?

It would seem easier to call levels_.reserve(...),
ensuring that no exception will be thrown.

In good (= reliable & maintainable) C++ code, uses of
try..catch blocks are be limited to locations where
errors are actually handled (or converted).
Rethrowing the same exception often hints at bad
design - IMHO.


Cheers,
Ivan

 

[Martin Eisenberg]

Hi Martin,

Hey!

message news:[email protected]...

This might be seen as a matter of taste.
However, creating a new name (PNode) without actually
simplifying anything is more error prone than anything else.
I would have expected PNode to be a Node*, not a const Node*.
Other than the 'less typing' argument, is there any reason
not to just use "const Node*" instead of "PNode" ? (i.e. for
clarity)

I created the typedef because it is part of CollatzTree's public
interface. But now that you make me think about it, I see that it's
not really an abstraction at all -- what's hidden is so simple that
using PNode corretly requires full knowledge.

/-: Sorry, somehow these lines did not make it into consciousness.
There's no question here.

I know there is such an idiom. What is it called, so I can go ogle?

It would seem easier to call levels_.reserve(...),
ensuring that no exception will be thrown.

Yes, I now recall you already wrote that. Neat!

In good (= reliable & maintainable) C++ code, uses of
try..catch blocks are be limited to locations where
errors are actually handled (or converted).
Rethrowing the same exception often hints at bad
design - IMHO.

That's interesting. I'll take a closer look at those points in my
code.

Thanks again, Ivan, for all the good advice!


Martin