Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > I'm a newbie. Is this code ugly?

Reply
Thread Tools

I'm a newbie. Is this code ugly?

 
 
gert
Guest
Posts: n/a
 
      01-20-2010
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[i]->surname<<"\t"<<array[i]->key<<"\n";
}
 
Reply With Quote
 
 
 
 
Andrew Poelstra
Guest
Posts: n/a
 
      01-20-2010
On 2010-01-20, gert <(E-Mail Removed)> wrote:
> 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[i]->surname<<"\t"<<array[i]->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.

 
Reply With Quote
 
 
 
 
Richard Herring
Guest
Posts: n/a
 
      01-20-2010
In message <4b56d0c2$0$828$(E-Mail Removed)>, io_x
<(E-Mail Removed)> writes
>
>"gert" <(E-Mail Removed)> ha scritto nel messaggio
>news:(E-Mail Removed)...
>> 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.

>#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+12*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[i]->key_,v[i+1]->key_)>0)
> {temp=v[i]; v[i]=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[i]);
> 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[i]->surname_<<"\t"<<a.v[i]->key_<<"\n";
> return 0;
>}
>
>
>


--
Richard Herring
 
Reply With Quote
 
Richard Herring
Guest
Posts: n/a
 
      01-20-2010
In message <4b57269b$0$1132$(E-Mail Removed)>, io_x
<(E-Mail Removed)> trolled
>
>"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
>news:(E-Mail Removed)...
>> In message <4b56d0c2$0$828$(E-Mail Removed)>, io_x
>><(E-Mail Removed)>
>> writes
>>>
>>>"gert" <(E-Mail Removed)> ha scritto nel messaggio
>>>news:(E-Mail Removed)...
>>>> 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.

> p=(char*) malloc(n+1);
> if(p==0) R 0;
> for(i=0; i<n; ++i)
> p[i]=v[i];
> p[i]=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+12*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[i]->key_,v[i+1]->key_)>0)
> {temp=v[i]; v[i]=v[i+1]; v[i+1]=temp; hit=1;}
> }
>}
>
> ~ArrArrT(){int i=n;
> for(--i; i>=0; --i)
> {v[i]->Tfree();
> free(v[i]);
> }
> 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[i]);
> }
>
>};
>
>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[i]->surname_<<"\t"<<a.v[i]->key_<<"\n";
>
> for(i=0; i<a.n; ++i)
> cout <<a[i].surname_<<"\t"<<a[i].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"));
}

--
Richard Herring
 
Reply With Quote
 
gert
Guest
Posts: n/a
 
      01-21-2010
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?

>> Sortable *temp;

>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?
 
Reply With Quote
 
Andrew Poelstra
Guest
Posts: n/a
 
      01-21-2010
On 2010-01-21, gert <(E-Mail Removed)> wrote:
> 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.

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


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.

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


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.

>>> Sortable *temp;

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


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


 
Reply With Quote
 
Fred Zwarts
Guest
Posts: n/a
 
      01-21-2010
io_x wrote:
> "Andrew Poelstra" <(E-Mail Removed)> ha scritto nel
> messaggio news:(E-Mail Removed) n...
>> On 2010-01-21, gert <(E-Mail Removed)> wrote:
>>> Thank you guys. I also love io_x's artwork

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

>
> 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.
 
Reply With Quote
 
Richard Herring
Guest
Posts: n/a
 
      01-21-2010
In message <4b57ff58$0$1135$(E-Mail Removed)>, io_x
<(E-Mail Removed)> writes
>
>"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
>news:(E-Mail Removed)...
>> In message <4b57269b$0$1132$(E-Mail Removed)>, io_x
>> <(E-Mail Removed)> trolled
>>>
>>>"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
>>>news:(E-Mail Removed)...
>>>> In message <4b56d0c2$0$828$(E-Mail Removed)>, io_x
>>>> <(E-Mail Removed)>
>>>> writes
>>>>>
>>>>>"gert" <(E-Mail Removed)> ha scritto nel messaggio
>>>>>news:(E-Mail Removed)...
>>>>>> 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?

[...]

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

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

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


--
Richard Herring
 
Reply With Quote
 
gert
Guest
Posts: n/a
 
      01-21-2010
> The C++ way to do that is:
>
> p = new Y;
> /* ... */
>
> delete p;


So how about Y p(); delepte p; Is this still C++ way?
 
Reply With Quote
 
Richard Herring
Guest
Posts: n/a
 
      01-21-2010
In message
<(E-Mail Removed)>,
gert <(E-Mail Removed)> writes
>> The C++ way to do that is:
>>
>> p = new Y;
>> /* ... */
>>
>> delete p;

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

--
Richard Herring
 
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
what is the difference between code inside a <script> tag and code in the code-behind file? keithb ASP .Net 1 03-29-2006 01:00 AM
Fire Code behind code AND Javascript code associated to a Button Click Event =?Utf-8?B?Q2FybG8gTWFyY2hlc29uaQ==?= ASP .Net 4 02-11-2004 07:31 AM
Re: Code Behind vs. no code behind: error Ben Miller [msft] ASP .Net 1 06-28-2003 01:46 AM
Re: C# Equivalent of VB.Net Code -- One line of code, simple Ian ASP .Net 0 06-25-2003 01:14 PM
Re: C# Equivalent of VB.Net Code -- One line of code, simple Ron ASP .Net 1 06-24-2003 07:18 PM



Advertisments