I'm a newbie. Is this code ugly?

[gert]

I'm using a class which can sinksort an array of it's own objects and
an example T class, which can have names and stuff...
I was in doubt about what to do about nearly any line, so I would love
any of your recommendations...

class Sortable
{
public:
char *key;
Sortable *temp;
Sortable ** sink(int len, Sortable **s)
{
int hit=1;
while(len>1&&hit){
len--;
hit=0;
for(int n=0; n<len; n++)
if(strcmp(s[n]->key, s[n+1]->key)>0){
temp=s[n];
s[n]=s[n+1];
s[n+1]=temp;
hit=1;
}
}
return s;
}
Sortable(){
}
};
class T: public Sortable
{
public:
char *num, *surname;
T(char *num, char *key, char *surname)
{
this->key=key;
this->num=num;
this->surname=surname;
}
};
main()
{
T a("a","Mann","Thomas");
T b("b","Satie","Erik");
T c("c","Goldfarb","Sarah");
T d("d","Ravel","Maurice");
T e("e","Hideyuki","Tanaka");
T f("f","Twain","Mark");
T *array[]= {&a, &b, &c, &d, &e, &f};
a.sink(6, (Sortable **) array);
for(int i=0; i<6; i++)
cout <<array->surname<<"\t"<<array->key<<"\n";
}

 

[Andrew Poelstra]

I'm using a class which can sinksort an array of it's own objects and
an example T class, which can have names and stuff...
I was in doubt about what to do about nearly any line, so I would love
any of your recommendations...

Well, the short answer is yes, this is pretty ugly. Are you
coming from C or a complete newbie? If it is the latter you
should probably find a better textbook (or teacher).

class Sortable
{
public:
char *key;

Look into std::string, rather than using char * to store
text. Also, don't use tabs on Usenet; even assuming that
they can make it through all the necessary newsservers
(a big if), they normally display as 8 spaces, which is
far too much on such a space-constrained medium. Try using
2 or 4 space tabs instead.

Sortable *temp;

This is unnecessary, and /certainly/ should not be public
even if it was necessary.

Sortable ** sink(int len, Sortable **s)

Instead of using a pointer-to-pointer, which is almost never
necessary in C++ (this is not C!), try using a std::list or
similar container. Your code will be cleaner and you will
avoid the double indirection, the first parameter, and all
the array boundary-crossing risks.

{

If you're going to declare a temp object, do it in here.

int hit=1;
while(len>1&&hit){
len--;
hit=0;
for(int n=0; n<len; n++)
if(strcmp(s[n]->key, s[n+1]->key)>0){
temp=s[n];
s[n]=s[n+1];
s[n+1]=temp;
hit=1;
}
}

I strongly recommend you find a book on sorting algorithms, or
post this code on comp.programming for feedback. This looks like
a bubble sort, which does not scale well.

return s;
}
Sortable(){
}

There is no need for an empty constructor. The compiler will
provide one for you.

};
class T: public Sortable
{
public:
char *num, *surname;
T(char *num, char *key, char *surname)
{
this->key=key;
this->num=num;
this->surname=surname;
}
};
main()

int main() or int main(void), though I have been told the latter is
a C-ism and not really appropriate in C++. In any case, main() returns
int.

{
T a("a","Mann","Thomas");
T b("b","Satie","Erik");
T c("c","Goldfarb","Sarah");
T d("d","Ravel","Maurice");
T e("e","Hideyuki","Tanaka");
T f("f","Twain","Mark");
T *array[]= {&a, &b, &c, &d, &e, &f};
a.sink(6, (Sortable **) array);
for(int i=0; i<6; i++)
cout <<array->surname<<"\t"<<array->key<<"\n";
}

What I have told you is an incomplete first pass over your code. If you
look into the things I suggested and check out the C++ FAQ, you will see
a lot of improvement.

 

[Richard Herring]

what about this?
Horrible.

#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
using namespace std;

class T{
public:
char *num_, *surname_;
char *key_;

C++ isn't C -- use std::string in preference to dynamic arrays.

};

class ArrArrT{
public:
T** v;

C++ isn't C -- use std::vector in preference to dynamic arrays.

int n;
int sz;

ArrArrT(){v=0; n=0; sz=0;}

int add(char *num, char *key, char *surname)

This function returns a boolean value, so don't pretend it's a number.
(Since the "failure" condition actually indicates that malloc failed,
and you should be delegating that kind of memory management to
std::vector, it would be better to make the function void and leave it
to vector to throw an exception if it can't allocate.)

{if(sz<=n){T **p;
p=(T**)realloc(v, (n+128)*sizeof(T*));

C++ isn't C -- use std::vector in preference to malloc and friends.
What's the magic number supposed to be for?

if(p==0) return 0;
sz=n+128;
v =p;
}
v[n]=(T*) malloc(sizeof(T));
if(v[n]==0) return 0;

v[n]->num_=num; v[n]->key_=key; v[n]->surname_=surname;
++n;
return 1;
}

void sort()
{int i, hit=1, len=n;

hit appears to be a Boolean flag, not a number -- declare it as such.

T *temp;

C++ isn't C. Declare local variables at the point of use -- if you need
them at all.

Someone else can comment on this O(N^2) code from an algorithmic POV.

while(len>1&&hit)
{len--;
hit=0;
for(i=0; i<len; ++i)
if(strcmp(v->key_,v[i+1]->key_)>0)
{temp=v; v=v[i+1]; v[i+1]=temp; hit=1;}

Use std::swap for swapping.

}
}

~ArrArrT(){int i=n;
for(--i; i>=0; --i)
free(v);
free(v);
}
};

int main(void)
{int i;

Don't separate declaration and initialization.

ArrArrT a;
i=1;
i*=a.add("a","Mann","Thomas");
i*=a.add("b","Satie","Erik");
i*=a.add("c","Goldfarb","Sarah");
i*=a.add("d","Ravel","Maurice");
i*=a.add("e","Hideyuki","Tanaka");
i*=a.add("f","Twain","Mark");

This code works, after a fashion, because those strings are all
literals. What would happen if you were reading values from a file?

if(i==0) {cout << "Memory error\n"; return 0;}
a.sort();

for(i=0; i<a.n; ++i)
cout <<a.v->surname_<<"\t"<<a.v->key_<<"\n";
return 0;
}

 

[Richard Herring]

no problem, i make local copy

Still horrible.

#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#define R return

Using macros to represent language keywords is inexcusable obfuscation.
Are you J*ff R*lf?

using namespace std;

char* faiMemCopia(char* v)

Why are you (badly) reinventing strcpy() ?

{int i, n;
char *p;
if(v==0) R 0;
n=strlen(v);
if(n<=0) R 0;

Why? There's nothing intrinsically wrong with a zero-length string.

p=(char*) malloc(n+1);
if(p==0) R 0;
for(i=0; i<n; ++i)
p=v;
p=0;
R p;
}


class T{
public:
char *num_, *surname_;
char *key_;

T(){num_=0; surname_=0; key_=0;}

int alloca(char *num, char *key, char *surname)

This sets up the class invariant. Put it in a constructor.

{num_=0; surname_=0; key_=0;
num_=faiMemCopia(num);
if(num_==0) return 0;
surname_=faiMemCopia(surname);
if(surname==0)
{la0:
free(num_); num_=0;
R 0;
}
key_=faiMemCopia(key);
if(key_==0){free(surname_); surname_=0;
goto la0;

Nice spaghetti.

}
R 1;
}

void Tfree(void)
{free(num_); free(key_); free(surname_);
num_=0; surname_=0; key_=0;
}

This releases resources. Put it in a destructor.

};

class ArrArrT{
public:
T** v;
int n;
int sz;

ArrArrT(){v=0; n=0; sz=0;}

int add(char *num, char *key, char *surname)
{if(sz<=n){T **p;
p=(T**)realloc(v, (n+128)*sizeof(T*));
if(p==0) R 0;
sz=n+128;

Still no explanation for those magic 128s.

v =p;
}
v[n]=(T*) malloc(sizeof(T));
if(v[n]==0) R 0;
if( v[n]->alloca(num, key, surname)==0)
R 0;
++n;
R 1;
}

void sort()
{int i, hit=1, len=n;
T *temp;

while(len>1&&hit)
{len--;
hit=0;
for(i=0; i<len; ++i)
if(strcmp(v->key_,v[i+1]->key_)>0)
{temp=v; v=v[i+1]; v[i+1]=temp; hit=1;}
}
}

~ArrArrT(){int i=n;
for(--i; i>=0; --i)
{v->Tfree();
free(v);
}
free(v);
}

T& operator[](int i)
{static T no;
if(i<0||i>=n)
{cout << "\n\aIndice fuori dei limiti\n"; R no;}
R *(v);
}

};

int main(void)
{int i;
ArrArrT a;
i=1;
i*=a.add("a","Mann","Thomas");
i*=a.add("b","Satie","Erik");
i*=a.add("c","Goldfarb","Sarah");
i*=a.add("d","Ravel","Maurice");
i*=a.add("e","Hideyuki","Tanaka");
i*=a.add("f","Twain","Mark");
if(i==0) {cout << "Memory error\n"; R 0;}
a.sort();

for(i=0; i<a.n; ++i)
cout <<a.v->surname_<<"\t"<<a.v->key_<<"\n";

for(i=0; i<a.n; ++i)
cout <<a.surname_<<"\t"<<a.key_<<"\n";

R 0;
}

Sheesh.

#include <string>
#include <vector>
#include <algorithm>
#include <ostream>
#include <iostream>
#include <iterator>

using namespace std; // SSM

struct Artist
{
string surname_, firstname_;

Artist(string const & surname, string const & firstname)
: surname_(surname), firstname_(firstname) {}
};

ostream & operator<<(ostream & s, Artist const & t)
{ return s << t.firstname_ << ' ' << t.surname_; }

struct CompareSurname
{
bool operator()(Artist const & a, Artist const & b) const
{ return a.surname_ < b.surname_; }
};

int main()
{
vector<Artist> artists;
artists.push_back(Artist("Mann", "Thomas"));
artists.push_back(Artist("Satie", "Erik"));
/* etc... */
sort(artists.begin(), artists.end(), CompareSurname());
copy(artists.begin(), artists.end(), ostream_iterator<Artist>(cout, "\n"));
}

 

[gert]

Thank you guys. I also love io_x's artwork
Sorry for mixing up forename and surname.

I strongly recommend you find a book on sorting algorithms, or
post this code on comp.programming for feedback. This looks like
a bubble sort, which does not scale well.

Yes, sinking sort is just an other name for bubble sort

Instead of using a pointer-to-pointer, which is almost never
necessary in C++ (this is not C!), try using a std::list or
similar container. Your code will be cleaner and you will
avoid the double indirection, the first parameter, and all
the array boundary-crossing risks.

Yes, I'm coming from C.
I also admit the example is a bit stupid. Who would ever need a class
to sort arrays of instances of itself?
But anyway, this was an exercise given to me and I am trying to make
the best of it.
We're not allowed to use the STL. And it has to be an array of
objects, no structs, no vektors.
So is this justification for using a pointer-to-pointer? Or would
there still a better alternative?

This is unnecessary

Unnecessary? I should've put it further down like you said later on,
but you need a temp somewhere don't you?

 

[Andrew Poelstra]

Thank you guys. I also love io_x's artwork
Sorry for mixing up forename and surname.

You dropped my name! But I forgive you.

Yes, sinking sort is just an other name for bubble sort

Ah. Given that it has almost zero overhead it actually works
quite well for sorting a few dozen items, but when you go
beyond that the O(n^2) will catch up with you.

Yes, I'm coming from C.

So am I - I'm actually far more a C programmer than a C++ programmer.
One of the most important lessons you can learn is that C and C++,
while sharing a lot of the same keywords and 1/3 of the name, are
very different languages, and in general require very different
mindsets to work with.

I also admit the example is a bit stupid. Who would ever need a class
to sort arrays of instances of itself?
But anyway, this was an exercise given to me and I am trying to make
the best of it.
We're not allowed to use the STL. And it has to be an array of
objects, no structs, no vektors.

Fair enough. But I think that C++ is perhaps a poor choice of language
for this low-level programming practice.

So is this justification for using a pointer-to-pointer? Or would
there still a better alternative?

Yep. In function calls you can use the suffix [] instead of the
prefix *. They mean the same thing but the former makes it clear
that you want a pointer to an array, not just a single object.
As for your second level of indirection, consider using pass by
reference instead.

Unnecessary? I should've put it further down like you said later on,
but you need a temp somewhere don't you?

Yes. I should have been clearer. Though, a very common exercise
in beginning C++ is to write a simple swap function, first to
learn about pass-by-reference, and then later to learn about
templates. (And as has been mentioned, there is a std::swap()
function available for you to use.)

 

[Fred Zwarts]

do you mean something like
void swap(T&, T&);
the swap exercise you refer is one not good use of reference &:

Why not?

if to be good it has to be
void swap(T*, T*)
using in swap(&v, &w)

Be careful when using pointers instead of references!
With pointers a check for NULL is needed.
So, I would prefer "void swap (T&, T&)",
not void swap "(T*, T*)", because of simpler code.

 

[Richard Herring]

"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
In message <[email protected]>, io_x
<[email protected]>
writes

"gert" <[email protected]> ha scritto nel messaggio
I'm using a class which can sinksort an array of it's own objects and
an example T class, which can have names and stuff...
I was in doubt about what to do about nearly any line, so I would love
any of your recommendations...

what about this?

Horrible.

This code works, after a fashion, because those strings are all literals.
What
would happen if you were reading values from a file?

no problem, i make local copy

Still horrible.

#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#define R return

Using macros to represent language keywords is inexcusable obfuscation. Are
you J*ff R*lf?

using namespace std;

char* faiMemCopia(char* v)

Why are you (badly) reinventing strcpy() ?

{int i, n;
char *p;
if(v==0) R 0;
n=strlen(v);
if(n<=0) R 0;

Why? There's nothing intrinsically wrong with a zero-length string.

yes here i would say "if(n<0) R 0;"

And under what circumstances would strlen() ever return a negative
value?

[...]

here i have one array of T* ;
the problem is always the same:
i have a class Y
Y *p;
p is class pointer
than i want build for p the Y class
than i don't know how use constructor-destructor for Y for that object

what i can do is
p=(Y*) malloc(sizeof(Y));
p->inizialize();
and
for deallocate it
p->deinizialize();
free(p);

The C++ way to do that is:

p = new Y;
/* ... */

delete p;

and the constructor and destructor of Y will be called without any work
on your part.

 

[gert]

The C++ way to do that is:

p = new Y;
/* ... */

delete p;

So how about Y p(); delepte p; Is this still C++ way?

 

[Richard Herring]

In message

So how about Y p();

(I don't think you meant that. It declares a function called p which
takes no arguments and returns a Y. Try just:

Y p;

;-)

delepte p; Is this still C++ way?

No. If you don't use new, you don't use delete, either.
This is the C++ way:

{
Y p; // here the constructor of Y is called
} // here the destructor of Y is called as p goes out of scope.

 

[LR]

for me 128 is a nice number

How would anyone reading your code know that 128 is a nice number?

How would know, and if they could, would they care, that 128 is a nice
number, for you?

Are those two uses of 128 related in some way? If so, how would someone
reading the code know that?

LR

 

[LR]

"io_x" <[email protected]> ha scritto nel messaggio
news:[email protected]...

yes i write horrible code, but not all the sheepe are white;

I completely fail to understand your point. Please explain this.

LR

 

[Richard Herring]

it seems strlen() here returned one unsigned type: size_t
here size_t is "unsigned int" of 32 bit but this means
strlen can return 0xF0000000 that is a negative number
seen it like int;

Only because you are perversely choosing to cast it to a signed type
when it is not.

and for me that negative number is not ok

So you should be declaring the variable n as size_t, not int.

[...]

thank you, i did not think about that, thanks

why the default operator delete
has one argument of type size_t?

It doesn't. A class-specific operator delete may have a second argument
of type size_t.

void operator delete(void*, size_t);

there is something hidden in the call delete?

Don't confuse the delete expression "delete p;" with the deallocation
function named "operator delete()". They do different things.

for example here
T a(1,1,1);
T *p=new T(a)

Why the unnecessary copy? Just write:

T * p = new T(1,1,1);

...
delete p;

[snip horrid C-style code]

 

[Jerry Coffin]

I'm using a class which can sinksort an array of it's own objects and
an example T class, which can have names and stuff...
I was in doubt about what to do about nearly any line, so I would love
any of your recommendations...

Without passing judgment about "ugly", I'd say the design and code
are quite a bit different from what you'd expect in well-written,
idiomatic C++. To accomplish the same general task, I'd typically
write something more like this:

class T {
// the num member is read in, but never used...
std::string key_, num_, surname_;
public:
T(std::string num, std::string key, std::string surname)
: key_(key), num_(num), surname_(surname)
{}

bool operator<(T const &other) const {
return key_ < other.key_;
}
friend std:stream &operator<<(std:stream &os, T const &t) {
return os << t.surname << "\t" << t.key;
};

int main() {
std::vector<T> t;
t.push_back(T("a","Mann","Thomas"));
t.push_back(T("b","Satie","Erik"));
t.push_back(T("c","Goldfarb","Sarah"));
t.push_back(T("d","Ravel","Maurice"));
t.push_back(T("e","Hideyuki","Tanaka"));
t.push_back(T("f","Twain","Mark"));

std::sort(t.begin(), t.end());
std::copy(t.begin(), t.end(),
std:stream_iterator<T>(std::cout, "\n"));
return 0;
}

Of course, there are quite a few variations, but I hope this at least
gives some general idea.

 

[LR]

yes, this could be the wrong number,
if for example someone has 1000000s of little double dinamic arrays

So it's possible that someone, perhaps even you might want to change
that instance of 128 in future?

Perhaps you ought to do something like:

static const size_increase = 128;
p=(T**)realloc(v, (n+size_increase)*sizeof(T*));
if(p==0) return 0; // I think R 0; is harder to read.
sz=n+size_increase;

Which will also save you from making the mistake of not hunting down the
second related occurrence.

LR

 

[LR]

the sheep white as programmer write all good code follow what
people say are the good laws for write code
the sheep black as programmer not

It seems to me, after reading this group for sometime, that most of the
contributors are more interested in following what more people think of
as being good rules for writing code.

The reasons for this are varied and there is always much debate.

There seems to be one area where there is substantial agreement. Code
is not only (or merely or simply) a means to instruct a computer to
follow some instructions, but a means to communicate ideas to other
people clearly.

The reasons for this are varied and there is always much debate.

However, I think, since you are interested in finding out if the code
was ugly, you may want to, for starters, ask if the way the code is
written aids clarity or hinders it.

I am otherwise at a loss as to why the question in the subject was asked
to begin with. Particularly since you imply that you are not one of the
people who wants to follow good rules. Doesn't this suggest that the
answer to your question obvious? And why would you ask here?

There are people who are interested in not following good rules and in
particular avoiding clarity. You may find much of value to you here
http://en.wikipedia.org/wiki/Obfuscated_code

LR

 

[Ian Collins]

#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#define R return

This is an abomination, no sane programmer would read any further.

 

[LR]

"LR" <[email protected]> ha scritto nel messaggio


i'm no the one that ask that question (if i remember all posts)

Oops. Sorry about that.

LR

 

[Richard Herring]

i choose int
so i choose that the valid array of chars
could have len in the range 0..max_int
at last here in my sys

Not a valid reason. By that logic, if you chose double, you could have
lengths up to DBL_MAX.

Why do you have a problem with giving n the obvious type (because it is
what strlen returns) of size_t? What do you think the length range would
be in that case?

 

[Richard Herring]

"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
In message <[email protected]>, io_x
{int i, n;
char *p;
if(v==0) R 0;
n=strlen(v);
if(n<=0) R 0;

Why? There's nothing intrinsically wrong with a zero-length string.

yes here i would say "if(n<0) R 0;"

And under what circumstances would strlen() ever return a negative value?

it seems strlen() here returned one unsigned type: size_t
here size_t is "unsigned int" of 32 bit but this means
strlen can return 0xF0000000 that is a negative number
seen it like int;

Only because you are perversely choosing to cast it to a signed
type when it
is not.

and for me that negative number is not ok

So you should be declaring the variable n as size_t, not int.

i choose int
so i choose that the valid array of chars
could have len in the range 0..max_int
at last here in my sys

Not a valid reason. By that logic, if you chose double, you could
have lengths
up to DBL_MAX.

Why do you have a problem with giving n the obvious type (because it is what
strlen returns) of size_t? What do you think the length range would
be in that
case?

size_t could be the "the obvious type" but with "int" i can check
for overflow checking if there is some value negative
in some places where could be overflow or afther too

if one program has
size_t t=90;

and there is one
t+=v;
and v make overflow t;

So you're using wrapping as a poor substitute for checking that t is in
range. On most systems, t will reach a value which exceeds the
available memory long before it wraps to a negative value.

If range checking is important, check the range. Just as
std::vector::at() (and probably operator[] too) does, using unsigned
subscripts.

what that program has to do:
0) check and return to the caller one error result
1) abort the program, the sys, the OS
2) nothing has to happen

my answer is 0

Right. But for many values of v, your solution will actually give (2).