code critique please

code critique please

Post by jaso » Fri, 23 Mar 2001 05:55:22



I would  just like some pointers on the program I have writen.
I am comming from a C background, and am teaching myself C++.
Basically, what I am looking for is some helpfull pointers to
make my code more readable, or better running.
As I said, this is all pretty new to me, and any advice anyone can give
would
be greatly appreciated.

Thank you in advance.
(This is not Homework, and I am not asking for anyone to write the
program, I am mearly asking for advice)

#include <iostream.h>
#include <string.h>
#include <stdlib.h>

class Test{
private:
 char FName[500];
 char LName[500];
 char Address[500];
 char Email[500];
 long Phone;
 long Cell;
public:
 char *GetFName(char *temp)
 {
  return(strncpy(FName, temp, sizeof(temp)));
 }
 char *GetLName(char *temp)
 {
  return(strncpy(LName, temp, sizeof(temp)));
 }
 char *GetAddy(char *temp)
 {
  return(strncpy(Address, temp, sizeof(temp)));
 }
 char *GetEmail(char *temp)
 {
  return(strncpy(Email, temp, sizeof(temp)));
 }
 long GetPhone(long temp)
 {
  return(temp);
 }
 long GetCell(long temp)
 {
  return(temp);
 }
 void GetInfo(void);

Quote:};

Test::Test();
Test::~Test();

void Test::GetInfo(void)
{
 Test temp;
 char tempch[500], // temp word
   *fname,
   *lname,
   *addy,
   *email;
 long tempnum,  // temp number
   phone,
   cell;

 cout << "Enter First Name:\n";
 cin >> tempch;
 fname = temp.GetFName(tempch);
 cout << "Enter Last Name:\n";
 cin >> tempch;
 lname = temp.GetLName(tempch);
 cout << "Enter Address:\n";
 cin >> tempch;
 addy = temp.GetAddy(tempch);
 cout << "Enter Email:\n";
 cin >> tempch;
 email = temp.GetEmail(tempch);
 cout << "Enter Phone Number:\n";
 cin >> tempnum;
 phone = temp.GetPhone(tempnum);
 cout << "Enter Cell Number:\n";
 cin >> tempnum;
 cell = temp.GetCell(tempnum);
 cout << fname << lname << addy << "\n"
<< email << "\n" << phone <<
"\n" <<
cell;

Quote:}

int main(void)
{
 Test temp;
 temp.GetInfo();
 return 0;
Quote:}

 
 
 

code critique please

Post by Victor Bazaro » Fri, 23 Mar 2001 06:10:28



> I would  just like some pointers on the program I have writen.
> I am comming from a C background, and am teaching myself C++.
> Basically, what I am looking for is some helpfull pointers to
> make my code more readable, or better running.
> As I said, this is all pretty new to me, and any advice anyone can
give
> would
> be greatly appreciated.

> Thank you in advance.
> (This is not Homework, and I am not asking for anyone to write the
> program, I am mearly asking for advice)

> #include <iostream.h>
> #include <string.h>
> #include <stdlib.h>

> class Test{
> private:
>  char FName[500];
>  char LName[500];
>  char Address[500];
>  char Email[500];
>  long Phone;
>  long Cell;
> public:

The following member functions are _misnamed_.  They _set_ the
corresponding data members, not _get_ them.

Quote:>  char *GetFName(char *temp)

make it

    char *GetFName(const char* temp)

and all other too.  The characters pointed to by the argument
are not changed.  Besides, it would be possible to use the
form "object.GetFName("Name").  Of course, "get versus set"
thing is whole other story...

Quote:>  {
>   return(strncpy(FName, temp, sizeof(temp)));

That will always copy 4 characters.  Use 'strlen' instead of 'sizeof'.
The same trouble in all other members.

BTW, it is a _very_ BAD IDEA(tm) to return a pointer to some private
data.  What the hell did you make it private for?

- Show quoted text -

Quote:>  }
>  char *GetLName(char *temp)
>  {
>   return(strncpy(LName, temp, sizeof(temp)));
>  }
>  char *GetAddy(char *temp)
>  {
>   return(strncpy(Address, temp, sizeof(temp)));
>  }
>  char *GetEmail(char *temp)
>  {
>   return(strncpy(Email, temp, sizeof(temp)));
>  }
>  long GetPhone(long temp)
>  {
>   return(temp);

What is this for?  If I follow your logic, this should
_set_ the Phone data member:

        return Phone = temp;

Quote:>  }
>  long GetCell(long temp)
>  {
>   return(temp);

Again, that's just straight-through flying.  Did you again
miss the point of calling this?

    return Cell = temp;

- Show quoted text -

Quote:>  }
>  void GetInfo(void);
> };

> Test::Test();
> Test::~Test();

> void Test::GetInfo(void)
> {
>  Test temp;
>  char tempch[500], // temp word
>    *fname,
>    *lname,
>    *addy,
>    *email;
>  long tempnum,  // temp number
>    phone,
>    cell;

>  cout << "Enter First Name:\n";
>  cin >> tempch;
>  fname = temp.GetFName(tempch);
>  cout << "Enter Last Name:\n";
>  cin >> tempch;

And what's going to happen with "fname"

- Show quoted text -

Quote:>  lname = temp.GetLName(tempch);
>  cout << "Enter Address:\n";
>  cin >> tempch;
>  addy = temp.GetAddy(tempch);
>  cout << "Enter Email:\n";
>  cin >> tempch;
>  email = temp.GetEmail(tempch);
>  cout << "Enter Phone Number:\n";
>  cin >> tempnum;
>  phone = temp.GetPhone(tempnum);
>  cout << "Enter Cell Number:\n";
>  cin >> tempnum;
>  cell = temp.GetCell(tempnum);
>  cout << fname << lname << addy << "\n"
> << email << "\n" << phone <<
> "\n" <<
> cell;

Is this test output?  You may want to surround it with some
sort of "#ifdef _DEBUG", then.  If it's not test output, then
I don't understand why it's here.  The method is called "GetInfo",
not "PrintInfo"...

Quote:> }

> int main(void)
> {
>  Test temp;
>  temp.GetInfo();
>  return 0;
> }

Victor
--
Please remove capital A's from my address when replying by mail

 
 
 

code critique please

Post by jaso » Fri, 23 Mar 2001 06:33:29


Thankyou for the help.
Greatly appreciated.

-- Jason

 
 
 

code critique please

Post by John Harriso » Fri, 23 Mar 2001 06:25:10



Quote:> I would  just like some pointers on the program I have writen.
> I am comming from a C background, and am teaching myself C++.

There are few C mistakes as well as C++ ones.

Quote:> Basically, what I am looking for is some helpfull pointers to
> make my code more readable, or better running.
> As I said, this is all pretty new to me, and any advice anyone can give
> would
> be greatly appreciated.

> Thank you in advance.
> (This is not Homework, and I am not asking for anyone to write the
> program, I am mearly asking for advice)

> #include <iostream.h>

<iostream.h> is non-standard, you should prefer <iostream>

Quote:> #include <string.h>
> #include <stdlib.h>

> class Test{
> private:
>  char FName[500];
>  char LName[500];
>  char Address[500];
>  char Email[500];

Big waste of space, learn how to use the string class, no waste of space and
more flexible

    std::string FName;
    std::string LName;
    std::string Address;
    std::string Email;

Quote:>  long Phone;
>  long Cell;
> public:
>  char *GetFName(char *temp)
>  {
>   return(strncpy(FName, temp, sizeof(temp)));
>  }

This function is called GetFName but it sets the value of FName, maybe you
got the parameters the wrong way round or maybe you choose a really bad
name. Also sizeof(temp) is the sizeof the pointer not the size of the array
that was used to call GetFName. There is no way to get the size of an array
from a pointer to that array. If you need the size of the array you'll have
to pass it in as a seperate parameter. If you learn how to use the string
class, as suggested above, then all this messing around becomes unnecessary,
just write

std::string GetFName() const
{
    return FName;

Quote:}

and

void SetFName(std::string fn)
{
    FName = fn;

Quote:}

So much easier.

Quote:>  char *GetLName(char *temp)
>  {
>   return(strncpy(LName, temp, sizeof(temp)));
>  }

Ditto

Quote:>  char *GetAddy(char *temp)
>  {
>   return(strncpy(Address, temp, sizeof(temp)));
>  }

Ditto

Quote:>  char *GetEmail(char *temp)
>  {
>   return(strncpy(Email, temp, sizeof(temp)));
>  }

Ditto

Quote:>  long GetPhone(long temp)
>  {
>   return(temp);
>  }

This doesn't get the phone, try

long GetPhone() const
{
    return Phone;

Quote:}
>  long GetCell(long temp)
>  {
>   return(temp);
>  }

Ditto

Quote:>  void GetInfo(void);
> };

> Test::Test();
> Test::~Test();

Putting these declarations outside of the class doesn't make a lot of sense,
in fact I'm not even sure its legal.

Quote:

> void Test::GetInfo(void)
> {
>  Test temp;
>  char tempch[500], // temp word
>    *fname,
>    *lname,
>    *addy,
>    *email;
>  long tempnum,  // temp number
>    phone,
>    cell;

>  cout << "Enter First Name:\n";
>  cin >> tempch;
>  fname = temp.GetFName(tempch);
>  cout << "Enter Last Name:\n";
>  cin >> tempch;
>  lname = temp.GetLName(tempch);
>  cout << "Enter Address:\n";
>  cin >> tempch;
>  addy = temp.GetAddy(tempch);
>  cout << "Enter Email:\n";
>  cin >> tempch;
>  email = temp.GetEmail(tempch);
>  cout << "Enter Phone Number:\n";
>  cin >> tempnum;
>  phone = temp.GetPhone(tempnum);
>  cout << "Enter Cell Number:\n";
>  cin >> tempnum;
>  cell = temp.GetCell(tempnum);
>  cout << fname << lname << addy << "\n"
> << email << "\n" << phone <<
> "\n" <<
> cell;
> }

GetInfo makes very little sense. It does operate on the object it's called
on (temp in main()), instead it operates on another local variable called
temp in GetInfo. Suppose you had

int main()
{
    Test test1, test2;
    test1.GetInfo();
    test2.GetInfo();

Quote:}

you would expect the first GetInfo to do something to or with test1 and the
second with test2, but the way you've written it neither test1 or test2 are
affected at all by GetInfo. Instead a local variable called temp in GetInfo
is modified and the thrown away (twice). You should drop the temp variable
from GetInfo just write

fname = GetFName(tempch);

instead of

fname = temp.GetFName(tempch);

Of course as explained above and below GetFName is wrong too, so you'll have
to change that as well.

Quote:

> int main(void)
> {
>  Test temp;
>  temp.GetInfo();
>  return 0;
> }

You obviously did absolutely zero testing on this code. That would be my
biggest piece of advice, learn how to test your code, applies to C and C++.

I think you also have a big confusion between methods which change an object
(these should be called SetSomething) and one's which query an object (these
should be called GetSomething). So from your example above

long Test::GetPhone() const
{
    return Phone;    // this gets the objects Phone value

Quote:}

void Test::SetPhone(long p)
{
    Phone = p;    // this sets the objects Phone value

Quote:}

john
 
 
 

code critique please

Post by John Harriso » Fri, 23 Mar 2001 06:28:14


[snip]

Quote:> GetInfo makes very little sense. It does operate on the object it's called

should read

GetInfo makes very little sense. It does not operate on the object it's
called

missing 'not'

john

 
 
 

code critique please

Post by Phli » Fri, 23 Mar 2001 06:29:46



> I would  just like some pointers on the program I have writen.
> I am comming from a C background, and am teaching myself C++.
> Basically, what I am looking for is some helpfull pointers to
> make my code more readable, or better running.
> As I said, this is all pretty new to me, and any advice anyone can give
> would
> be greatly appreciated.

I Rx the book /Accelerated C++/ by Koenig & Moo.

Quote:> #include <iostream.h>

Use #include <iostream>

Quote:> #include <string.h>

Use #include <string> (and maybe <cstring> if you actually need things like
'strcpy').

Quote:> #include <stdlib.h>

> class Test{
> private:
>  char FName[500];

Use 'string FName;'. You have encountered the first problem with
old-fashioned strings; they don't manage their storage very well. 500 is a
terrible waste; let 'string' hide the details.

Quote:>  char LName[500];
>  char Address[500];
>  char Email[500];
>  long Phone;

Is a Phone number an integer? Will you ever add two phone numbers?

Quote:>  long Cell;
> public:

Put publics at the top, privates at the bottom. This helps those who read
your classes.

Quote:>  char *GetFName(char *temp)
>  {
>   return(strncpy(FName, temp, sizeof(temp)));
>  }

Are these Gets or Sets? Someone has told you to make data members private.
Forget that until you A> use 'string's to make them simpler, and B> follow
OAOO. See below.

Quote:>  char *GetLName(char *temp)
>  {
>   return(strncpy(LName, temp, sizeof(temp)));
>  }
>  char *GetAddy(char *temp)
>  {
>   return(strncpy(Address, temp, sizeof(temp)));
>  }
>  char *GetEmail(char *temp)
>  {
>   return(strncpy(Email, temp, sizeof(temp)));
>  }
>  long GetPhone(long temp)
>  {
>   return(temp);
>  }
>  long GetCell(long temp)
>  {
>   return(temp);
>  }
>  void GetInfo(void);
> };

> Test::Test();
> Test::~Test();

> void Test::GetInfo(void)
> {
>  Test temp;
>  char tempch[500], // temp word
>    *fname,
>    *lname,
>    *addy,
>    *email;
>  long tempnum,  // temp number
>    phone,
>    cell;

Only write comments that add value. Don't declare everything up at the top
of a function; put it all right where it's used.

And don't bother to collect the return value of a function if you are not
going to use it.

- Show quoted text -

Quote:>  cout << "Enter First Name:\n";
>  cin >> tempch;
>  fname = temp.GetFName(tempch);
>  cout << "Enter Last Name:\n";
>  cin >> tempch;
>  lname = temp.GetLName(tempch);
>  cout << "Enter Address:\n";
>  cin >> tempch;
>  addy = temp.GetAddy(tempch);
>  cout << "Enter Email:\n";
>  cin >> tempch;
>  email = temp.GetEmail(tempch);
>  cout << "Enter Phone Number:\n";
>  cin >> tempnum;
>  phone = temp.GetPhone(tempnum);
>  cout << "Enter Cell Number:\n";
>  cin >> tempnum;
>  cell = temp.GetCell(tempnum);

You are repeating yourself. Always express things OnceAndOnlyOnce (OAOO).
This implies here that you should write a function to actually get the
variable from the user, and that you should then write only the unique
things outside the function:

  temp.setFName (prompt ("First Name"));
  temp.setLName (prompt ("Last Name"));
  temp.setAddy (prompt ("Address"));

And stop calling things 'temp'.

--

============== http://phlip.webjump.com ==============
  --  Spaghetti Bad. Reuse Good.  --

 
 
 

code critique please

Post by Ron Natali » Fri, 23 Mar 2001 06:54:10



> #include <iostream.h>
> #include <string.h>
> #include <stdlib.h>

You should use the standard library includes NOT these.

Quote:

> class Test{
> private:
>  char FName[500];
>  char LName[500];
>  char Address[500];
>  char Email[500];

These are huge (staves off erros below), but wastest a lot of space.

Quote:>  char *GetFName(char *temp)
>  {
>   return(strncpy(FName, temp, sizeof(temp)));
>  }

These functions are misnomers.  They don't really "Get" anything.
They Set things.

Second, you're confused about what sizeof is.  sizeof is an
operator (not a function, the parens are not required here).
"sizeof temp" is really "sizeof char*", which is the size of
the pointer itself (typcially 4).

Further strnpy doesn't need to be told how big the source (second)
argument is, it figrues this out by scanning it for null.  What you
should be doing is passing in "sizeof FName" to make sure some yahoo
doesn't give you a pointer to something bigger that FName to copy.

Even then you're sort of hosed because sizeof won't null terminate it
and you don't have any good way of testing if it did or not (I assume,
unless your constructor is smart).

Quote:

> Test::Test();
> Test::~Test();

Where is the definition of these?

Quote:

>  cout << "Enter First Name:\n";
>  cin >> tempch;

What happens when some clown types 510 characters to this input?
You're hosed.

You'd be better off to nuke the char arrays and use the std::string
class.  These will be appropriately sized and you don't have to worry
about overflows.  It solves a whole heap of other problems.

 
 
 

code critique please

Post by Ron Natali » Fri, 23 Mar 2001 06:55:54



> >  {
> >   return(strncpy(FName, temp, sizeof(temp)));

> That will always copy 4 characters.  Use 'strlen' instead of 'sizeof'.
> The same trouble in all other members.

at most 4 characters.  You don't even want strlen here.  It will guarantee
that the FName will end up not null terminated.  Either use strcpy, or give
sizeof FName as the third arg.
 
 
 

code critique please

Post by jaso » Fri, 23 Mar 2001 07:05:34


wow, lots of mistakes....
thanks alot for the help.

This was just a little test to see where I am,
obviosly I have a ways to go.

Thank you for taking the time to do this,

Well, back to work/reading.

--  Jason

 
 
 

code critique please

Post by Mark Wilde » Fri, 23 Mar 2001 07:40:17



I would suggest that you don't add getters and setters until you actually
have a need for them. A narrow interface generally makes code more
maintainable.

 
 
 

code critique please

Post by Courageou » Fri, 23 Mar 2001 08:59:58


Quote:>    char *GetFName(const char* temp)

>and all other too.  The characters pointed to by the argument
>are not changed.  Besides, it would be possible to use the
>form "object.GetFName("Name").  Of course, "get versus set"
>thing is whole other story...

>>  {
>>   return(strncpy(FName, temp, sizeof(temp)));

>That will always copy 4 characters.  Use 'strlen' instead of 'sizeof'.
>The same trouble in all other members.

>BTW, it is a _very_ BAD IDEA(tm) to return a pointer to some private
>data.  What the hell did you make it private for?

The code was so badly implemented, it's hard to guess at what he's
trying to do. On systems that support it, the correct thing (in ANSI C
form) is to:

        return strdup(FName);

On systems without strdup, something more like...

        char* retval = malloc ( (strlen(FName)+1)*sizeof(char));
        return strcpy(retval,FName);

... is the correct thing.

I'll presume that the use of char* is an artifact; this aside, however,
what should be in use is std::string, not char*.

C//

 
 
 

code critique please

Post by Stephen How » Thu, 22 Mar 2001 11:37:49



Jason, everybody else has given useful advice. May seem hard at times but I
tell you what, your C++ will improve :-)

My small bit:

I have never liked strncpy() because unlike the other strxxx() functions it
is not guaranteed to nul-terminate the destination. I also do not like the
nul padding when the source string is shorter than the length (which I
believe originated because it was used to pad UNIX directory entries which
were 32 bytes).

But I would advice that if you do wish to use it, you do

size_t n;
char *dest;
char *src;
:
:
strncpy(dest, src, n);
dest[n+1] = '\0';            // ensure nul-terminated.

if dest holds n characters  + 1 for the '\0'

Stephen Howe

 
 
 

code critique please

Post by Mark Johns » Fri, 23 Mar 2001 12:58:45




Quote:>> char *GetFName(char *temp)
>> {
>>  return(strncpy(FName, temp, sizeof(temp)));
>> }

>Typically, "accessor" methods should be const and have one role, to
>return data.  Like the others said your "accessors" are setting and then
>returning values.  Instead have "mutator" methods to change the state of
>your object:

>void Test::SetFName( const char * name )
>{
>         if( !name || !*name )
>                  throw "::SetFName() invalid name"; // might be a
>                  little severe

>         FName = name;
>}

Sorry, I was thinking std::string when i wrote the above.

class Test
{
public:
        void SetFName( const char * name )
        {
                if( !name || !*name )
                    return;// invalid data

                FName = name;
        }

private:
        std::string FName;

- Show quoted text -

Quote:};

 
 
 

code critique please

Post by Mark Johns » Fri, 23 Mar 2001 12:55:20



Jason, a lot of folks posted some good info. Some of what I may say have
already been said:

Quote:>#include <iostream.h>
>#include <string.h>

These are non-standard header, instead use the standard C++ headers:

        <iostream>
        <string>

Quote:>class Test{
>private:
> char FName[500];
> char LName[500];
> char Address[500];
> char Email[500];

Don't use magic numbers, you should declare constant integers instead, but
better yet is to use std::string instead of char [] unless you have a good
reason not to.

Quote:> char *GetFName(char *temp)
> {
>  return(strncpy(FName, temp, sizeof(temp)));
> }

Typically, "accessor" methods should be const and have one role, to return
data.  Like the others said your "accessors" are setting and then returning
values.  Instead have "mutator" methods to change the state of your object:

const char * Test::GetFName() const
{
        return FName; // notice that it's returning a "const char*" !!

Quote:}

void Test::SetFName( const char * name )
{
        if( !name || !*name )
                throw "::SetFName() invalid name"; // might be a little severe

        FName = name;

Quote:}
> void GetInfo(void);

This is a horrible name for a function, very vague.  I would suggest that
you go check out or buy "Code Complete" by Steve McConnell.

Quote:

>Test::Test();
>Test::~Test();

Is this legal?

Quote:>void Test::GetInfo(void)
>{
> Test temp;
> char tempch[500], // temp word
>   *fname,
>   *lname,
>   *addy,
>   *email;
> long tempnum,  // temp number
>   phone,
>   cell;

Don't declare vars like this, this reeks of C programmer.  Define vars
closest to where you use them.

Quote:> cout << "Enter First Name:\n";
> cin >> tempch;
> fname = temp.GetFName(tempch);
> cout << "Enter Last Name:\n";
> cin >> tempch;
> lname = temp.GetLName(tempch);
> cout << "Enter Address:\n";
> cin >> tempch;
> addy = temp.GetAddy(tempch);
> cout << "Enter Email:\n";
> cin >> tempch;
> email = temp.GetEmail(tempch);
> cout << "Enter Phone Number:\n";
> cin >> tempnum;
> phone = temp.GetPhone(tempnum);
> cout << "Enter Cell Number:\n";
> cin >> tempnum;
> cell = temp.GetCell(tempnum);
> cout << fname << lname << addy << "\n"
><< email << "\n" << phone <<
>"\n" <<
>cell;
>}

Always a bad idea to have UI functionality in an class unless it is a UI
class.  Getting input like this should be done outside a class or done by a
dedicated class that performs only UI type functions.

Quote:>int main(void)
>{
> Test temp;
> temp.GetInfo();
> return 0;
>}

C++ doesn't require you to specify void.  Instead you should simple do:

int main()
{
        // etc..
        return 0;

Quote:}

The same goes for you methods.  Don't use void in the argument list, just
use empty parens...

Another suggestion is to adapt a style.  I would suggest adopting the
CoreLinux++ Style Guide (http://corelinux.sourceforge.net/cppstnd.php) or
even Microsofts. Don't event your own if possible, use a published guide.  
If you don't adopte any published ones be very thoughtful and create your
own.  This is help readabliity tremendously, and aid the way you name
variable, functions, and methods...

I would read through the CoreLinux++ style guide even if you don't like
their naming conventions because the philosophy behind the guide is very
informative about C++.  I like this style guide because it was formed out
of the works of Vlissides and Meyers and some of the other well know C++
industry folk.

good luck!

 
 
 

code critique please

Post by Michael Rubenstei » Fri, 23 Mar 2001 21:38:42






>Jason, a lot of folks posted some good info. Some of what I may say have
>already been said:

>>#include <iostream.h>
>>#include <string.h>

>These are non-standard header, instead use the standard C++ headers:

>            <iostream>
>            <string>

<string.h> is standard, though deprecated.  It is different from
<string>.
--
Michael M Rubenstein
 
 
 

1. Please critique a newbie's code

Hi all:
I am in the process of learning C++. I have finished a project for school,
and I just wanted to get some constructive criticism about my code. I would
like to know where can I improve it. Here is a link to the specs and my
files:

http://www.cse.fau.edu/~fcarpio/

Thanks in advance.

2. Need: Infrastructure Analyst,Java Web Developer & Silver light

3. Please critique this stream.

4. OS/2 drivers for Gateway 2000 laptop 2500

5. Derived type for template function results. Please critique

6. Error #048:015?

7. Please critique: Odd abstraction I read in a book.

8. please critique: visitor+observer+composite...

9. Please critique this OO language design

10. Please critique my resume HERE.

11. Please critique my resume. ;P

12. Code/Idea Critique