I'm a newbie. Is this code ugly?

Discussion in 'C++' started by gert, Jan 20, 2010.

  1. gert

    gert Guest

    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";
    }
     
    gert, Jan 20, 2010
    #1
    1. Advertising

  2. On 2010-01-20, gert <> 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->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.
     
    Andrew Poelstra, Jan 20, 2010
    #2
    1. Advertising

  3. In message <4b56d0c2$0$828$>, io_x
    <> writes
    >
    >"gert" <> ha scritto nel messaggio
    >news:...
    >> 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+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
     
    Richard Herring, Jan 20, 2010
    #3
  4. In message <4b57269b$0$1132$>, io_x
    <> trolled
    >
    >"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
    >news:...
    >> In message <4b56d0c2$0$828$>, io_x
    >><>
    >> writes
    >>>
    >>>"gert" <> ha scritto nel messaggio
    >>>news:...
    >>>> 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=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"));
    }

    --
    Richard Herring
     
    Richard Herring, Jan 20, 2010
    #4
  5. gert

    gert Guest

    Thank you guys. I also love io_x's artwork :D
    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?
     
    gert, Jan 21, 2010
    #5
  6. On 2010-01-21, gert <> wrote:
    > Thank you guys. I also love io_x's artwork :D
    > 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.)
     
    Andrew Poelstra, Jan 21, 2010
    #6
  7. gert

    Fred Zwarts Guest

    io_x wrote:
    > "Andrew Poelstra" <> ha scritto nel
    > messaggio news:...
    >> On 2010-01-21, gert <> wrote:
    >>> Thank you guys. I also love io_x's artwork :D

    >> 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.
     
    Fred Zwarts, Jan 21, 2010
    #7
  8. In message <4b57ff58$0$1135$>, io_x
    <> writes
    >
    >"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
    >news:...
    >> In message <4b57269b$0$1132$>, io_x
    >> <> trolled
    >>>
    >>>"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
    >>>news:...
    >>>> In message <4b56d0c2$0$828$>, io_x
    >>>> <>
    >>>> writes
    >>>>>
    >>>>>"gert" <> ha scritto nel messaggio
    >>>>>news:...
    >>>>>> 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
     
    Richard Herring, Jan 21, 2010
    #8
  9. gert

    gert Guest

    > The C++ way to do that is:
    >
    > p = new Y;
    > /* ... */
    >
    > delete p;


    So how about Y p(); delepte p; Is this still C++ way?
     
    gert, Jan 21, 2010
    #9
  10. In message
    <>,
    gert <> 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
     
    Richard Herring, Jan 21, 2010
    #10
  11. gert

    LR Guest

    io_x wrote:

    >>> p=(T**)realloc(v, (n+128)*sizeof(T*));
    >>> if(p==0) R 0;
    >>> sz=n+128;

    >>
    >> Still no explanation for those magic 128s.


    > 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, Jan 22, 2010
    #11
  12. gert

    LR Guest

    io_x wrote:
    > "io_x" <> ha scritto nel messaggio
    > news:4b57ff58$0$1135$...


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


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

    LR
     
    LR, Jan 22, 2010
    #12
  13. In message <4b594ee2$0$1115$>, io_x
    <> writes
    >
    >"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
    >news:...
    >> In message <4b57ff58$0$1135$>, io_x
    >> <> writes
    >>>
    >>>>>
    >>>>>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?

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

    [...]

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

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

    --
    Richard Herring
     
    Richard Herring, Jan 22, 2010
    #13
  14. gert

    Jerry Coffin Guest

    In article <e4951ec7-f09b-4b74-9cf8-a7a9e19e400c@
    22g2000yqr.googlegroups.com>, says...
    >
    > 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::eek:stream &operator<<(std::eek: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::eek: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.

    --
    Later,
    Jerry.
     
    Jerry Coffin, Jan 22, 2010
    #14
  15. gert

    LR Guest

    io_x wrote:
    > "LR" <> ha scritto nel messaggio
    > news:4b58f327$0$4817$...
    >> io_x wrote:
    >>
    >>>>> p=(T**)realloc(v, (n+128)*sizeof(T*));
    >>>>> if(p==0) R 0;
    >>>>> sz=n+128;
    >>>>
    >>>> Still no explanation for those magic 128s.

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

    >
    > 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, Jan 23, 2010
    #15
  16. gert

    LR Guest

    io_x wrote:
    > "LR" <> ha scritto nel messaggio
    > news:4b58f392$0$4817$...
    >> io_x wrote:
    >>> "io_x" <> ha scritto nel messaggio
    >>> news:4b57ff58$0$1135$...

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

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

    >
    > 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
     
    LR, Jan 23, 2010
    #16
  17. gert

    Ian Collins Guest

    io_x wrote:
    >
    > #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.

    --
    Ian Collins
     
    Ian Collins, Jan 23, 2010
    #17
  18. gert

    LR Guest

    io_x wrote:
    > "LR" <> ha scritto nel messaggio
    > news:4b5b359f$0$4831$...



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


    Oops. Sorry about that.

    LR
     
    LR, Jan 25, 2010
    #18
  19. In message <4b5b5b92$0$1101$>, io_x
    <> writes
    >
    >"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
    >news:KJlK$...
    >> In message <4b594ee2$0$1115$>, io_x
    >> <> writes
    >>>>>>>{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?

    --
    Richard Herring
     
    Richard Herring, Jan 25, 2010
    #19
  20. In message <4b5f0875$0$1112$>, io_x
    <> writes
    >
    >"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
    >news:eek:...
    >> In message <4b5b5b92$0$1101$>, io_x
    >> <> writes
    >>>
    >>>"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
    >>>news:KJlK$...
    >>>> In message <4b594ee2$0$1115$>, io_x
    >>>> <> writes
    >>>>>>>>>{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).

    --
    Richard Herring
     
    Richard Herring, Jan 26, 2010
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Stuart D. Gathman

    Help beautify ugly heuristic code

    Stuart D. Gathman, Dec 8, 2004, in forum: Python
    Replies:
    14
    Views:
    673
    Stuart D. Gathman
    Jul 25, 2006
  2. Jeff
    Replies:
    2
    Views:
    456
  3. Replies:
    10
    Views:
    497
    Default User
    Jul 14, 2006
  4. Replies:
    10
    Views:
    609
    Default User
    Jul 14, 2006
  5. Jim Langston

    Ugly C looking code

    Jim Langston, Aug 26, 2006, in forum: C++
    Replies:
    5
    Views:
    417
    Ian Collins
    Aug 26, 2006
Loading...

Share This Page