classes and pointers, is this right???

[PatrickIRL]

Hi all,

Been messing around with this code for a bit of fun. Just curious if I'm using the class and pointers as they should be used. I've done something similar in java (sorry).

(FYI - definitely NOT homework or anything like that, just curious, that's all)

Any feedback appreciated,

Patrick

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

class Test{
		
public:
	void setFirstname(char *firstname){
	    strcpy(_firstname,firstname);
	}

	char *getFirstname(){
	    return _firstname;
	}

private:
	char	_firstname[20];
};

void main()
{
    Test test;

    test.setFirstname(&quot;Patrick&quot;);

    cout << test.getFirstname() << &quot;\n\n&quot;;
}

 

[tokra]

By returning a char * from getFirstname you allow your callers to mess with the underlying data which kinda defeats the purpose of encapsulation.

You can return a const char * so users can't screw with the data
---or---
you can return a copy of the data there are two ways of doing that
1: Use a std::string found in <string> //note the lack of .h
2:

char *ans=new char[20];
strcpy(ans,_firstname);
return ans;

This is a sucky solution because your caller has to remember to delete the returned value.
---or---
Modify an input paramater

char *getFirstname(char *buffer,int buffersize)
{
buffer[0]='\0';
strncat(buffer,_firstname,buffersize-1);//no buffer overflows
return buffer;
}

WR

 

[tokra]

I noticed that your code used <iostream.h> most of the standard .h files are deprecated and you can't mix and match old .h's with the the new ones. Note this only applies to the standard libs.

use <iostream>
ex

#include <iostream>
using namespace std;
int main()
{
cout << &quot;Hello world&quot;;
return 0;
}
---or---
#include <iostream>
using  std::cout;
int main()
{
cout << &quot;Hello world&quot;;
return 0;
}
---or---

#include <iostream>
int main()
{
std::cout << &quot;Hello world&quot;;
return 0;
}

Of these solutions 2 or 3 is preferred because you're not unpacking the entire std namespace.



WR

 

[Lajin]

alrighty tokra, could you please repeat your first post in here, ya kinda confused me when making your statement about something messing with underlying data, I am raher new to C++ and would like to learn as much as possible, the one problem is that everyone who actually understands the language speaks like teh language it's self, therefrore it makes it rather difficult to learn new things if they are not explained in simpletens terms

 

[tokra]

pointers are pointers to locations in memory. Any function that has a valid pointer can modify the data that pointer contains regardless of who actually owns the data. Your getFirstname functions returns the pointer to your internal data. Run the following demonstration code to get a better idea of what I mean.

class Test{
        
public:
    void setFirstname(char *firstname){
        strcpy(_firstname,firstname);
    }

    char *getFirstname(){
        return _firstname;
    }

private:
    char    _firstname[20];
};

int main()
{
    Test test;
	test.setFirstname("George");
	std::cout << test.getFirstname() << &quot;\n\n&quot;;
	char *tempc=test.getFirstname();
	strcpy(tempc,&quot;Vladimir&quot;);    

	std::cout << test.getFirstname() << &quot;\n\n&quot;;
	return 0;
}

WR

 

[bulgakov]

Basically the rules of encapsulation state that data
that belongs to a class (attributes of the class)
should only be changed by the owning class. Thus
high cohesion and low coupling.

This statement is true if a class has private data,
this prevents encapsultaion from being broken,
as other classes can not change the data directly.

If however a pointer to the memory location that
stores the data is obtained from the owning
class then whoever has the pointer can change
what it points to, or the contents of what it points
to. So a method that returns a pointer
to a piece of memory allocated by a class allows
other classes to change the value of the classes
attributes (broken encapsulation).

An exception to this rule is when attributes are declared
as protected. This gives access to them outside of the
owning class, but only to classes that derive from
the owning class.

If your attributes are public, then anyone with an instance
of the owning class can change them directly

i.e. MyClass.MyIntAttribute = 8;

Only microsoft program this badly. Their C++ is very
NOO (Not Object Orientated) see all MFC classes.

As a general rule pass the value of an attribute
as a const pointer or a const reference or better
still a copy (however not a good idea for large
classes) i.e.

class Colour
{
public :
Colour();
virtual ~Colour();
};

class Shape
{
private :
Colour MyColour;
public :
Shape();
virtual ~Shape();

colour GetMyColour() {return MyColour;} //ok as this
//returns a
// copy to MyColour

const colour *GetMyColour() {return &MyColour;} //ok as
//address (pointer)
//is const

const colour &GetMyColour() {return MyColour;} //ok as
//reference is const

colour &GetMyColour() {return MyColour;} //BAD returns
// a reference to
// actual attribute
//address

colour *GetMyColour() {return &MyColour;} //BAD returns
//a pointer to
//actual address of
//attribute
};

However be aware if you do only declare the const
pointer functions someone may do this

Colour *Col = (Colour*)MyShapeInstance.GetColour();

This is casting from a const to a non-const and thus
whats being pointed to can change. So the suggestion from
the last post is not as good as the copy return
(however not a good idea for large classes - but
ok for built in types).

If you do return a copy make sure if the class you
are returning does not contain pointers, otherwise
you will need to write a copy constructor...

Colour(const &Colour);

otherwise you can end up with a copy whose attributes
are pointers to the same memory as the original. If
you don;t fully understand this just follow the rule
of always writing a copy constructor.

Of course there are other issues, like making your accessor
functions (functions that give access to a classes
attributes) const...

colour GetMyColour() const {return MyColour;}

this prevents anyone from changing the functionality
of GetMyColour to doing silly things like changing the
value of MyColour. Then of course there is

const colour * const GetMyColour() const {return &MyColour;}

Anyway ....

Really the solution to the problem is to use a CString
class and return a copy of it.

 

[Lajin]

now again, I must ask, please use simple terms man, I am rather young and VERY new to this, for example, when you say returns to a copy, I am saying what on earth are you talking about!? Which copy? What copy? I am much better at understanding if someone speaks my illustrious language of stupid

 

[teriviret]

There's something else I noticed about this example that I thought I should point out. It is vulnerable to buffer-overrun exploits! This is unfortunately a very common bug that allows unscrupulous people to break into computers, and the same type of thing that some of Microsoft's patches this year were fixing in Microsoft Windows.

You see, the member variable _firstname is only 20 characters long. However, in the &quot;setFirstname&quot; method you are using the standard C library function &quot;strcpy&quot;, which could very well copy many more than 20 characters.

Two good rules to remember:
1. Never trust your input (i.e. check to make sure parameters are within the limits).
2. Don't abuse your output (i.e. don't stuff something into a container that is too small).

If you must use fixed-size arrays to copy into, get into the habit of using &quot;strncpy&quot; instead of &quot;strcpy&quot;, since &quot;strncpy&quot; will limit the number of characters copied so you don't overrun your buffer.

 

[PatrickIRL]

Cool, thanks for the feedback, much appreciated. I like the last point, thanks teriviret, not something I would have considered.

Patrick