Re: I'm a newbie. Is this code ugly?

From:

Richard Herring <junk@[127.0.0.1]>

Newsgroups:

comp.lang.c++

Date:

Wed, 20 Jan 2010 11:34:40 +0000

Message-ID:

<4iB21VKQpuVLFw7l@baesystems.com>

In message <4b56d0c2$0$828$4fafbaef@reader5.news.tin.it>, io_x
<a@b.c.invalid> writes

"gert" <27hiro@googlemail.com> ha scritto nel messaggio
news:e4951ec7-f09b-4b74-9cf8-a7a9e19e400c@22g2000yqr.googlegroups.com...

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