473,324 Members | 2,193 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,324 software developers and data experts.

Code critique

Hi,

I've written a program that I'd like to hear some opinions on
regarding my C++ usage. As a program it's smallish, but on Usenet 300
lines seem a bit much. Do you think it's appropriate to post the
code? Alternatively, maybe someone would be willing to put it on the
web for some days? I'm reluctant to sign up with a freespace provider
just for this one occasion; besides, most disallow use of their
space as pure file storage.

Thanks,
Martin

--
Quidquid latine dictum sit, altum viditur.
Jul 22 '05
54 2841
Kevin Goodsell wrote:
Martin Eisenberg wrote:

Conclusion using Modus Praematurus:
- I need to special-case for Windows.
You did notice that was tongue-in-cheek, yes?


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
Jul 22 '05 #51
"Martin Eisenberg" <ma*****************@PAMudo.edu> wrote in message
news:10***************@ostenberg.wh.uni-dortmund.de...
Hi Martin,
cin.ignore( std::numeric_limits<int>::max(), '\n' );


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)
typedef const Node* PNode;
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.


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)
- 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<Level> levelPool;
At destruction time, the 'levelPool' container
will safely destroy and deallocate all Level instances.


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.
Unfortunately, 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.


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.
void CollatzTree::addLevel()
{
if(isFinished_) return;
const Level& oldTop = *levels_.back();
levels_.push_back(new Level());
levels_.back()->reserve(getNewLevelSize());


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


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
--
http://ivan.vecerina.com/contact/?subject=NG_POST <- e-mail contact form
Jul 22 '05 #52
"Martin Eisenberg" <ma*****************@PAMudo.edu> wrote in message
news:10***************@ostenberg.wh.uni-dortmund.de...
Hi Martin,
cin.ignore( std::numeric_limits<int>::max(), '\n' );


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)
typedef const Node* PNode;
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.


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)
- 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<Level> levelPool;
At destruction time, the 'levelPool' container
will safely destroy and deallocate all Level instances.


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.
Unfortunately, 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.


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.
void CollatzTree::addLevel()
{
if(isFinished_) return;
const Level& oldTop = *levels_.back();
levels_.push_back(new Level());
levels_.back()->reserve(getNewLevelSize());


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


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
--
http://ivan.vecerina.com/contact/?subject=NG_POST <- e-mail contact form
Jul 22 '05 #53
Ivan Vecerina wrote:
Hi Martin,
Hey!

<abridged>
"Martin Eisenberg" <ma*****************@PAMudo.edu> wrote in
message news:10***************@ostenberg.wh.uni-dortmund.de...
>> typedef const Node* PNode;
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)


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.
> after adding a data member such as:
> std::deque<Level> levelPool;

/-: Sorry, somehow these lines did not make it into consciousness.
There's no question here.
Or maybe build a local structure and assign that to the member
if all is well in the end?


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

--
It is not the strongest of the species
that survive, nor the most intelligent,
but the one most responsive to change.
--Charles Darwin
Jul 22 '05 #54
Ivan Vecerina wrote:
Hi Martin,
Hey!

<abridged>
"Martin Eisenberg" <ma*****************@PAMudo.edu> wrote in
message news:10***************@ostenberg.wh.uni-dortmund.de...
>> typedef const Node* PNode;
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)


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.
> after adding a data member such as:
> std::deque<Level> levelPool;

/-: Sorry, somehow these lines did not make it into consciousness.
There's no question here.
Or maybe build a local structure and assign that to the member
if all is well in the end?


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

--
It is not the strongest of the species
that survive, nor the most intelligent,
but the one most responsive to change.
--Charles Darwin
Jul 22 '05 #55

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

12
by: hokiegal99 | last post by:
Is there a forum where one could post a Python script and have it critiqued by others? Something like: Y would be more efficent if you did it this way, or doing it that way could cause problems...
37
by: Eric | last post by:
There is a VB.NET critique on the following page: http://www.vb7-critique.741.com/ for those who are interested. Feel free to take a look and share your thoughts. Cheers, Eric. Ps: for those...
54
by: Martin Eisenberg | last post by:
Hi, I've written a program that I'd like to hear some opinions on regarding my C++ usage. As a program it's smallish, but on Usenet 300 lines seem a bit much. Do you think it's appropriate to...
19
by: TC | last post by:
Are there any good sites or forums for a web critique? I went to alt.html.critique and it's pretty dead.
9
by: bowsayge | last post by:
Inspired by fb, Bowsayge decided to write a decimal integer to binary string converter. Perhaps some of the experienced C programmers here can critique it. It allocates probably way too much...
8
by: G Patel | last post by:
I wrote the following program to remove C89 type comments from stdin and send it to stdout (as per exercise in K&R2) and it works but I was hoping more experienced programmer would critique the...
188
by: christopher diggins | last post by:
I have posted a C# critique at http://www.heron-language.com/c-sharp-critique.html. To summarize I bring up the following issues : - unsafe code - attributes - garbage collection -...
2
by: winston | last post by:
I wrote a Python program (103 lines, below) to download developer data from SourceForge for research about social networks. Please critique the code and let me know how to improve it. An...
2
by: matt | last post by:
this is my first program in this language ever (besides 'hello world'), can i get a code critique, please? it's purpose is to read through an input file character by character and tally the...
0
by: DolphinDB | last post by:
Tired of spending countless mintues downsampling your data? Look no further! In this article, you’ll learn how to efficiently downsample 6.48 billion high-frequency records to 61 million...
0
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
0
by: jfyes | last post by:
As a hardware engineer, after seeing that CEIWEI recently released a new tool for Modbus RTU Over TCP/UDP filtering and monitoring, I actively went to its official website to take a look. It turned...
0
by: ArrayDB | last post by:
The error message I've encountered is; ERROR:root:Error generating model response: exception: access violation writing 0x0000000000005140, which seems to be indicative of an access violation...
1
by: CloudSolutions | last post by:
Introduction: For many beginners and individual users, requiring a credit card and email registration may pose a barrier when starting to use cloud servers. However, some cloud server providers now...
1
by: Defcon1945 | last post by:
I'm trying to learn Python using Pycharm but import shutil doesn't work
1
by: Shællîpôpï 09 | last post by:
If u are using a keypad phone, how do u turn on JavaScript, to access features like WhatsApp, Facebook, Instagram....
0
by: Faith0G | last post by:
I am starting a new it consulting business and it's been a while since I setup a new website. Is wordpress still the best web based software for hosting a 5 page website? The webpages will be...
0
isladogs
by: isladogs | last post by:
The next Access Europe User Group meeting will be on Wednesday 3 Apr 2024 starting at 18:00 UK time (6PM UTC+1) and finishing by 19:30 (7.30PM). In this session, we are pleased to welcome former...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.