Re: Errors
Tadeusz B. Kopec wrote:
On Sun, 11 Nov 2007 21:46:46 -0800, Latina wrote:
Hi I am doing a program oveloraded operator. I am having a
few errors on it.
Quite many, i would say. First of all, if it's not an exercise someone
gave to you but a class to be used somewhere, you should use
std::set<int> or make a class that's just a wrapper for it. Your class
has many problems.
class IntegerSet
{
private:
bool set[26];
First of all, it seems to be a set of integers from range
[0..25]. Is it what you wanted?
It looks like it.
The number 26 is very suggestive. Perhaps what he really wants
is SetOfLetters.
Second - it's wasting space. Use std::vector<bool> or if you
want a fixed size - std::bitset.
As has often been pointed out, std::vector<bool> is broken, and
should be avoided. And it's likely to take up just as much, or
more space than the original code. (On my machine, I get
sizeof(bool) == 1, sizeof(std::vector<bool>) == 40. And that
doesn't count the memory dynamically allocated by std::vector.)
Of course, you can do better. My SetOfCharacter uses an array
of 32 bytes, for 256 possible characters. For 26, I'm not sure
it's worth it. For that matter, for 256, I'm not sure its worth
it, unless you have a lot of instances. (My implementation of
SetOfCharacter does this, but it was written back in the days
when I was using a 16 bit machine, with a maximum of 64 KB
memory.)
int element;
What is this member variable for? The only use I could see is a loop
variable and for that a local variable would do.
public:
//Operator methods.
IntegerSet operator + (const IntegerSet &)const;//method union
//Methods
IntegerSet(); //default constructor
This comment is useless - doesn't say anything more that code does.
Constructors generally do need a comment concerning the post
conditions, however. Something to the order of "creates an
empty set", or:
//! \post
//! <tt>! isValid(i)</tt> for all i in [0..25]
IntegerSet(int x[], int k); //overload constructor
This comment is also useless. But this constructor needs a comment:
"x - array of integers to add to set
k - size of x
x must not contain numbers greater than 25 nor less then 0".
In which case, a char[] would be just as good:-). And of
course, it should be "int const x[]" or "char const x[]".
If he really is implementing SetOfLetter, I'd use char const*.
Allowing anything, ignoring non-alpha, and converting all upper
and lower case to the corresponding integral value. But since
we don't know what the class is for (its role or its
responsibilities), we can't say.
bool isValid(int)const;
// It should be rather "contains". There is nothing invalid in not being
kept in a specific set.
void insertElement(int);
void deleteElement(int);
void setString();
void inputSet();
};
IntegerSet set();
You declare a function named set. What for? You don't seem to use it.
IntegerSet::IntegerSet()
{
for(element=0; element>=25; element++)
set[element]= false;
}
The only thing this function does is assigning 0 to element.
It initializes the set to empty. Seems like a reasonable thing
to do for a default constructor. (Of course, using std::fill
would be more idiomatic. But his code seems quite reasonable.)
IntegerSet::IntegerSet(int x[], int k) {
for(element=0; element>=25; element++)
set[element]= false;
Again you assign 0 to element and nothing else;
Again, initializing the set to empty.
for(int j=0; j<k; j++)
{
element=x[j];
set[element]= true;
If x contains a number greater than 25 or less then 0, you have undefined
behaviour
An assert would be in order, if that is the precondition.
}
}
bool IntegerSet::isValid(int i)const
{
return set[i];
}
Do you intentionally omit bound checking?
//insert element to a set
void IntegerSet::insertElement(int element) {
set[element]=true;
}
// Comments are useful at method declaration not definition. Again you
omit bound checking.
And a comment like his isn't useful at all. It doesn't say
anything more than the function name. A comment specifying the
pre-condition would be useful, however.
I'd also call this function in the non-default constructor.
//delete element of a set
void IntegerSet::deleteElement(int element) {
set[element]=false;
}
Ditto
//overloaded operator + to compute the union of two sets IntegerSet
IntegerSet::operator+(const IntegerSet &right)const {
IntegerSet j;
for(int element=0; element<=25; element++) {
if(isValid(element) || right.isValid(element))
j.insertElement(element);
Since this is a member function:
j.set[ element ] = set[ element ] | right.set[ element ] ;
No if necessary.
I'd also use "result", rather than j, as the name. Names like
i, j, k should be reserved for loop indexes.
--
James Kanze (GABI Software) email:james.kanze@gmail.com
Conseils en informatique orient=E9e objet/
Beratung in objektorientierter Datenverarbeitung
9 place S=E9mard, 78210 St.-Cyr-l'=C9cole, France, +33 (0)1 30 23 00 34