Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > Code critique

Reply
Thread Tools

Code critique

 
 
Martin Eisenberg
Guest
Posts: n/a
 
      04-06-2004
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.
 
Reply With Quote
 
 
 
 
Petec
Guest
Posts: n/a
 
      04-06-2004
Martin Eisenberg wrote:
> 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


I'd be happy to critique your code, and I think as long as you put (long) in
the subject it'd be fine.

- Pete


 
Reply With Quote
 
 
 
 
David Harmon
Guest
Posts: n/a
 
      04-06-2004
On 6 Apr 2004 22:31:34 GMT in comp.lang.c++, Martin Eisenberg
<(E-Mail Removed)> wrote,
>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?


I think you should ask the specific questions that you wish to ask, and
post as much code as is needed for the context for the questions. The
more specific the questions, the better answers you will get. The more
relevant the code is to the specific questions, the better answers you
will get.

 
Reply With Quote
 
Martin Eisenberg
Guest
Posts: n/a
 
      04-06-2004
David Harmon wrote:

> I think you should ask the specific questions that you wish to
> ask,


Is my code in good style from the Standard C++ viewpoint? It doesn't
get any more specific than that.

> and post as much code as is needed for the context for the
> questions.


Which would be the whole program.


Martin
 
Reply With Quote
 
Alf P. Steinbach
Guest
Posts: n/a
 
      04-06-2004
* Martin Eisenberg <(E-Mail Removed)> schriebt:
> David Harmon wrote:
>
> > I think you should ask the specific questions that you wish to
> > ask,

>
> Is my code in good style from the Standard C++ viewpoint? It doesn't
> get any more specific than that.
>
> > and post as much code as is needed for the context for the
> > questions.

>
> Which would be the whole program.


Just post, put "(long)" in the subject line.

--
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
 
Reply With Quote
 
Martin Eisenberg
Guest
Posts: n/a
 
      04-07-2004
OK, thanks for the encouragements. Note that I'm well aware that
I won't solve the Collatz conjecture that way. It is the canvas,
C++ is the picture. Also, I know the standard doesn't speak to
those MSVC artifacts, but if you think they could be better
handled I'm very interested in hearing about that as well.
There are three files; word wrap is off.


Martin

// main.cpp
/* This program searches for a seed value
yielding a Collatz sequence that is long
in relation to the seed's magnitude.
*/

#ifdef _MSC_VER
// "identifier was truncated to '255' characters in the debug information"
#pragma warning(disable: 4786)
#endif

#include <iostream>
#include <iomanip>
#include <fstream>
#include <string>
#include "collatztree.h"

using namespace std;

const string KSeqFile = "collatz.txt";
const unsigned KCheckInterval = 10000000;

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

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
cout << '\n';
return c == 'y';
}

bool checkUserBreak()
{ return !prompt("Continue searching?"); }

void writeBest(const CollatzTree::Best& best,
ostream& os, unsigned nodeCount = 0)
{
os << "The best seed found ";
if(nodeCount > 0) os << "among the first " << nodeCount << " nodes ";
os << "is n = " << best.pNode->value
<< "\nwith number of iterations it(n) = " << best.depth
<< "\nand cost c(n) = it(n) / log2(n) = " << best.cost << ".\n";
}

void writeBest(const CollatzTree::Best& best, unsigned nodeCount)
{ writeBest(best, cout, nodeCount); }

void writeSequence(const CollatzTree::Best& best)
{
if(prompt("Write the longest sequence found to " + KSeqFile + "?")) {
ofstream ofs(KSeqFile.c_str());
writeBest(best, ofs);
ofs << '\n';
CollatzTree:Node pn = best.pNode;
ofs << pn->value;
while(!pn->isRoot()) {
pn = pn->parent;
ofs << ", " << pn->value;
}
ofs << '\n';
}
}

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

int main()
{
CollatzTree ct(writeBest, checkUserBreak, KCheckInterval);
cout << "Building Collatz tree...\n\n" << fixed << setprecision(3);
do ct.addLevel(); while(!ct.isFinished());
writeSequence(ct.getBest());
awaitEnter();
return 0;
}

// end of file ----------------------------------------------------------------

// collatztree.h
#ifndef COLLATZTREE_H
#define COLLATZTREE_H

#include <vector>
#include <list>
#include <utility>
#include <memory>

/* The class CollatzTree creates, level for level, the
tree of terminating Collatz sequences that do not
exceed the range of type unsigned. It also tracks
the seed value n with largest quotient of the number
of iterations from n and log2(n), and periodically
outputs the most expensive seed found so far.
Smaller values are favored in case of equal cost.
The user can interrupt the class' operation.
The possibility of memory exhaustion is ignored.
*/

class CollatzTree {
public:
struct Node;
struct Best;
typedef void (*WriteBest)(const Best&, unsigned nodeCount);
typedef bool (*CheckUserBreak)();
typedef const Node* PNode;

struct Node {
Node(unsigned value, PNode parent);
bool isRoot() const;

unsigned value;
PNode parent;
};

struct Best {
PNode pNode;
unsigned depth;
float cost;
};

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

CollatzTree(WriteBest writeBest,
CheckUserBreak checkUserBreak, unsigned checkInterval);
~CollatzTree();
void addLevel();
const Best& getBest() const;
bool isFinished() const;

private:
typedef std::vector<Node> Level;
typedef std::vector<Level*> Levels;
typedef std:air<unsigned, unsigned> Children;

PNode addInitial(unsigned value, PNode parent);
bool insert(unsigned value, PNode parent);
Children getChildren(unsigned value);
size_t getNewLevelSize();

const WriteBest writeBest_;
const CheckUserBreak checkUserBreak_;
const unsigned checkInterval_;
Levels levels_;
Best best_;
unsigned nodeCount_;
bool isFinished_;
};

#endif // COLLATZTREE_H
// end of file ----------------------------------------------------------------

// collatztree.cpp
#include "collatztree.h"

#if defined(_MSC_VER) && _MSC_VER == 1200 // MSVC 6
namespace std {
#include <math.h>
}
#else
#include <cmath>
#endif

#include <limits>

// CollatzTree ----------------------------------------------------------------
// public

CollatzTree::Node::Node(unsigned value, CollatzTree:Node parent)
: value(value), parent(parent)
{}

bool CollatzTree::Node::isRoot() const
{ return parent == 0; }

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

/* The constructor creates the entries up to 8 so that
getChildren() need not handle 1 as a spurious child of 4.
*/

CollatzTree::CollatzTree(CollatzTree::WriteBest writeBest,
CollatzTree::CheckUserBreak checkUserBreak, unsigned checkInterval)
: writeBest_(writeBest), checkUserBreak_(checkUserBreak),
checkInterval_(checkInterval), nodeCount_(4), isFinished_(false)
{
PNode pn = addInitial(1, 0);
pn = addInitial(2, pn);
best_.pNode = pn;
best_.depth = 1;
best_.cost = 1;
addInitial(8, addInitial(4, pn));
}

CollatzTree::~CollatzTree()
{
Levels::iterator pl = levels_.begin();
for(; pl != levels_.end(); ++pl) delete *pl;
}

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

Children children;
Level::const_iterator parent = oldTop.begin();
for(; parent != oldTop.end() && !isFinished_; ++parent) {
children = getChildren(parent->value);
if(children.first != 0)
isFinished_ = insert(children.first, parent);
if(children.second != 0 && !isFinished_)
isFinished_ = insert(children.second, parent);
}
}

const CollatzTree::Best& CollatzTree::getBest() const
{ return best_; }

bool CollatzTree::isFinished() const
{ return isFinished_; }

// private --------------------------------------

CollatzTree:Node
CollatzTree::addInitial(unsigned value, CollatzTree:Node parent)
{
Level* pl = new Level();
levels_.push_back(pl);
pl->push_back(Node(value, parent));
return &pl->back();
}

bool CollatzTree::insert(unsigned value, CollatzTree:Node parent)
{
static const float KLn2 = std::log(2.f);
static const unsigned KMaxNodeCount = std::numeric_limits<unsigned>::max();

levels_.back()->push_back(Node(value, parent));
unsigned depth = levels_.size() - 1;
float cost(depth / std::log(float(value)) * KLn2);
if(cost >= best_.cost && (cost > best_.cost || value < best_.pNode->value)) {
best_.pNode = &levels_.back()->back();
best_.depth = depth;
best_.cost = cost;
}
if(++nodeCount_ % checkInterval_ == 0) {
writeBest_(best_, nodeCount_);
if(checkUserBreak_()) return true;
}
return nodeCount_ == KMaxNodeCount;
}

CollatzTree::Children CollatzTree::getChildren(unsigned value)
{
static const unsigned
KHalfMaxValue = std::numeric_limits<unsigned>::max() / 2;

unsigned c1 = value * 2, c2 = (value - 1) / 3;
if(value > KHalfMaxValue) c1 = 0;
if(c2 % 2 == 0 || c2 * 3 + 1 != value) c2 = 0;
return Children(c1, c2);
}

/* Empirically determining the factors in getNewLevelSize() is easy,
but I have not proven that there are no outliers further up.
*/

size_t CollatzTree::getNewLevelSize()
{
static const float KFactors[] = {
0.f, 1.f, 1.f, 1.f, 1.f, 2.f, 1.f, 2.f, 1.f, 1.5f, 1.f,
1.34f, 1.25f, 1.4f, 1.29f, 1.34f, 1.21f, 1.25f, 1.23f, 1.32f
};
static const size_t NFactors(sizeof(KFactors) / sizeof(KFactors[0]));

float factor;
unsigned depth = levels_.size() - 1;
if(depth < NFactors) factor = KFactors[depth]; else factor = 1.28f;
return static_cast<size_t>(std::ceil(levels_[depth - 1]->size() * factor));
}

// end of file ----------------------------------------------------------------
 
Reply With Quote
 
Martin Eisenberg
Guest
Posts: n/a
 
      04-07-2004
<sigh> ...and sorry for forgetting the "(long)" in the subject.


Martin
 
Reply With Quote
 
Kevin Goodsell
Guest
Posts: n/a
 
      04-07-2004
Martin Eisenberg wrote:

> OK, thanks for the encouragements. Note that I'm well aware that
> I won't solve the Collatz conjecture that way. It is the canvas,
> C++ is the picture. Also, I know the standard doesn't speak to
> those MSVC artifacts, but if you think they could be better
> handled I'm very interested in hearing about that as well.
> There are three files; word wrap is off.


This is just a few quick comments, no time for a complete run-down right
now.

>
>
> Martin
>
> // main.cpp
> /* This program searches for a seed value
> yielding a Collatz sequence that is long
> in relation to the seed's magnitude.
> */
>
> #ifdef _MSC_VER
> // "identifier was truncated to '255' characters in the debug information"
> #pragma warning(disable: 4786)
> #endif


Sucks, don't it? Not much else you can do about this, though.

>
> #include <iostream>
> #include <iomanip>
> #include <fstream>
> #include <string>
> #include "collatztree.h"
>
> using namespace std;


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;

and you could even put these inside the functions that use those items.

>
> const string KSeqFile = "collatz.txt";
> const unsigned KCheckInterval = 10000000;


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.

>
> //-----------------------------------------------
>
> 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


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.

That's about all I can do right now.

-Kevin
--
My email address is valid, but changes periodically.
To contact me please use the address from a recent posting.
 
Reply With Quote
 
John Harrison
Guest
Posts: n/a
 
      04-07-2004
> 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;
>


Unfortunately MSVC 6 does not implement this usage correctly.

> and you could even put these inside the functions that use those items.
>


Has E Robert Tisdale got to you? It's not advice I would give.

> >
> > const string KSeqFile = "collatz.txt";
> > const unsigned KCheckInterval = 10000000;

>
> 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.
>
> >
> > //-----------------------------------------------
> >
> > 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

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


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


 
Reply With Quote
 
E. Robert Tisdale
Guest
Posts: n/a
 
      04-07-2004
John Harrison wrote:

>>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;
>>
>>and you could even put these inside the functions that use those items.

>
> It's not advice I would give.


And just exactly what advice would you give?

 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Stream input, critique my code? Donald Canton C++ 2 05-05-2004 07:48 PM
Code critique (again) Ryan Stewart HTML 4 05-04-2004 06:44 AM
Code critique (no URL available) Ryan Stewart HTML 3 04-26-2004 09:39 PM
Code Critique Please Rv5 C++ 3 11-16-2003 03:25 PM
Nasty code...but please critique it anyway :-) Michael Strorm C++ 26 11-10-2003 05:39 PM



Advertisments