Need help with the Big Three and this project. (a little long, sorry)

[degausser]

Alright so the project itself is almost done, I just need to alter some of the coding with whatever help I get from you guys.

I will give you guys 2 files, one is a header file and the other is the code file. The header file is just so you guys can see the members of the class, and how I should change my code accordingly.

Now, I should mention that my code compiles just fine, but I've set up tests along the way to see if it would have any problems, and there seems to be a problem with the way I'm trying to use my Database class, because this is an error I'm getting in the console:

GetNumClasses returned 1 but that's wrong!
Error adding the class to the database.
Database test failed. 

And I'm also not sure how to handle integer arrays with the copy constructor, as you'll see.

Anyway, here are the files:
classroom.h:

#ifndef CLASSROOM_H
#define CLASSROOM_H
#include <cstring>
#include "defines.h"
#include <iostream>

using namespace std;

class CClassroom
{
    char *m_pName;
    int *m_pStudent;
    int m_nTeacherID;
    int m_nClassSize;

public:

    CClassroom();
    CClassroom(const CClassroom& input);
    CClassroom(int size);
    ~CClassroom();

    int GetStudentEnrolled(int index);
    bool IsStudentEnrolled(int studentID);
    void SetStudentEnrolled(int index);
    char* GetClassName() {return m_pName;}
    void GetClassName(char input[]);
    void SetName(const char *input){strcpy(m_pName, input);}
    double GetClassSize();
    void SetClassSize(int nClassSize);
    double GetTeacherID();
    void SetTeacher(int input) { m_nTeacherID = input;}
    void AddStudent(int nStudentID);

    CClassroom& operator=(const CClassroom& rhs);

    friend ostream& operator<<(ostream& os,
                               const CClassroom& input);
    friend istream& operator>>(istream& is, CClassroom &input);

    friend bool operator==(const CClassroom& lhs, const CClassroom &rhs);
    friend bool operator!=(const CClassroom& lhs, const CClassroom &rhs);
};


inline int CClassroom::GetStudentEnrolled(int index)
{
    return m_pStudent[index];
} 

inline void CClassroom::SetStudentEnrolled(int index)
{
    m_pStudent[index] = index;
} 

inline double CClassroom::GetClassSize()
{
    return m_nClassSize;
} 

inline void CClassroom::SetClassSize(int input)
{
    m_nClassSize = input;
} 

inline double CClassroom::GetTeacherID()
{
    return m_nTeacherID;
}

#endif 

And classroom.cpp:

#include "teacher.h"
#include <iostream>
#include <fstream>
#include <cstring>
#include "leakdetection.h"
#include "classroom.h"

#ifdef MEMORY_LEAK_DETECTION
#define DEBUG_NEW new(__FILE__, __LINE__)
#define new DEBUG_NEW
#endif

using namespace std;

CClassroom::CClassroom()
{
    m_nClassSize = 0;
    m_pName = new char[MAX_CLASS_NAME_LENGTH];
    m_pStudent = new int[MAX_CLASS_SIZE];
} 

CClassroom::CClassroom(const CClassroom& input)
{
    m_nClassSize = input.m_nClassSize;
    m_nTeacherID = input.m_nTeacherID;
    m_pName = new char[MAX_CLASS_NAME_LENGTH];
    strcpy(m_pName, input.m_pName);
    m_pStudent = new int[MAX_CLASS_SIZE];
    for (int i = 0; i < MAX_CLASS_SIZE; i++)
    {
        m_pStudent[i] = input.m_pStudent[i];
    }
} 

CClassroom::CClassroom(int size)
{
    m_pName = NULL;
    m_pStudent = NULL;
    m_nClassSize = size;
}

void CClassroom::GetClassName(char input[])
{
    strcpy(input, m_pName);
}

bool CClassroom::IsStudentEnrolled(int studentID)
{
    for (int i = 0; i < m_nClassSize; i++)
    {
        if (studentID == m_pStudent[i])
        {
            return true;
        }
    }
    return false;
} 

void CClassroom::AddStudent(int nStudentID)
{
    for (int i = 0; i < m_nClassSize; i++)
    {
        if (m_pStudent[i] == NULL)
        {
            m_pStudent[i] == nStudentID;
        }
    }
} 

ostream& operator<<(ostream& os, const CClassroom& input)
{
    os << input.m_pName
       << endl
       << input.m_nTeacherID
       << endl
       << input.m_nClassSize
       << endl;
       for (int i = 0; i < input.m_nClassSize; i++)
        {
            os << input.m_pStudent[i]<< " ";
        }
    return os;
} 

CClassroom& CClassroom::operator=(const CClassroom& rhs)
{
    if(&rhs == this)
        {
        return *this;
        }
    delete [] m_pName;
    delete [] m_pStudent;
    m_nClassSize = rhs.m_nClassSize;
    m_pName = new char[MAX_NAME_LENGTH];
    m_nTeacherID = rhs.m_nTeacherID;
    strcpy(m_pName, rhs.m_pName);
    m_pStudent = new int[MAX_CLASS_SIZE];
    for (int i = 0; i < MAX_CLASS_SIZE; i++)
    {
        m_pStudent[i] = rhs.m_pStudent[i];
    }
    return *this;
} 

istream& operator>>(istream& is, CClassroom& input)
{
     if(input.m_pName == NULL)
     {
         input.m_pName = new char[MAX_NAME_LENGTH];
     }
     if(input.m_pStudent == NULL)
     {
        input.m_pStudent = new int[MAX_CLASS_SIZE];
     }
     is.getline(input.m_pName, MAX_NAME_LENGTH);
     is >> input.m_nTeacherID >> input.m_nClassSize;
     for (int i = 0; i < input.m_nClassSize; i++)
     {
        is >> input.m_pStudent[i];
     }
     is.ignore();
     return is;
}

CClassroom::~CClassroom()
{
    delete [] m_pName;
    delete [] m_pStudent;
}

bool operator==(const CClassroom& lhs, const CClassroom &rhs)
{
    // check if either is null, we shouldn't compare strings
    // using strcmp if they're null
    if(rhs.m_pName == NULL || lhs.m_pName == NULL)
    {
        if(rhs.m_pName == NULL && lhs.m_pName == NULL &&
           (lhs.m_nTeacherID == rhs.m_nTeacherID) &&
           (lhs.m_nClassSize == rhs.m_nClassSize) &&
           (lhs.m_pStudent == rhs.m_pStudent))
        {
            return true;
        }
        else
        {
            return false;
        }
    }
     if((lhs.m_nTeacherID != rhs.m_nTeacherID) ||
        (lhs.m_nClassSize != rhs.m_nClassSize) ||
        strcmp(rhs.m_pName, lhs.m_pName) ||
        (lhs.m_pStudent != rhs.m_pStudent))
     {
         return false;
     }
     return true;
} 

bool operator!=(const CClassroom& lhs, const CClassroom &rhs)
{
    return !(lhs == rhs);
} 

My concerns are with the following methods:
- Classroom()
- CClassroom(const CClassroom& input)
- CClassroom(int size)
- IsStudentEnrolled(int studentID)
- AddStudent(int nStudentID)
- operator=(const CClassroom& rhs)
- operator>>(istream& is, CClassroom& input)

Also, this is the code snippet where my program is failing from (the test portion of it)

if(classData.GetNumClasses() != 10)
{
     cout << "GetNumClasses returned " << classData.GetNumClasses() << " but       that's wrong! \n";
     cout << "Error adding the class to the database" << endl;
     
     return false;
}

And this is the code it's running in order for me to get that error:
http://codepad.org/ceyJNvnc

I can post the entire project if need be, but considering how big it is I'll have to paste it on codepad and link you guys..

Anyway, thank you in advance!

[degausser]

Bump, please

[degausser]

Bump again :(

[xismn]

How many LOC is the entire project? In your test snippet, what is classData? What does the GetNumClasses() method do exactly? How and where are CClassroom instances being stored?
Help me help you ;)
Also, you've got an unnecessary condition on line 137.

[degausser]

It's pretty long - main() is 230 lines on its own, then I have 13 other files, both header files and cpp files.

classData is just the name I'm using to access my Database class.
GetNumClasses() just returns the number of classes a student has enrolled into.
And I don't think I understand what you mean by 'how and where are CClassroom instances being stored'?

If more people decide to help, I know they'll ask the same thing, so I'll just give links to my code through codepad.
main.cpp: http://codepad.org/fLDAy51V

I'm pretty sure everything is coded correctly here:
student.h: http://codepad.org/NZyJD8Vi
student.cpp: http://codepad.org/Olb866pP
teacher.h: http://codepad.org/pqgIleTx
teacher.cpp: http://codepad.org/H33nCszy
defines.h: http://codepad.org/WFj3biIU
leakdetection.h: http://codepad.org/HJB163Ix (provided by our professor)
leakdetection.cpp: http://codepad.org/SZNmwp6S (provided by our professor)
test.cpp: http://codepad.org/aeXN8mdI (test cases to make sure the constructors/getters work correctly)

The coding within these is what I'm unsure of (and it's the same from above):
classroom.h: http://codepad.org/vQXWrrAr
classroom.cpp: http://codepad.org/hOwJ1LQc
database.h: http://codepad.org/wINc3DFg
database.cpp: http://codepad.org/uMrwLAZw

[degausser]

The code compiles - I'll go to add student, teacher and class perfectly fine.

However, once I choose to print the schedule of the student or teacher, it won't print out the names of the students I've made up/added.

If I make up a new class, it won't print (display) the people enrolled in that class.

[andywestken]

@degausser

Ive had a look though the posted code and also the file on http://codepad.org and there are a few mistakes:
- CDatabase::operator= isn't returning *this at the end
- CDatabase::CreateDatabase isn't returning true (I expect) at the end
- and operator== is being used instead of operator= on line 65 (in the original code above) of classroom.cpp

        if (m_pStudent[i] == NULL)
        {
            m_pStudent[i] == nStudentID;
        }

- And CClassroom's default constructor isn't initializing m_nTeacher to zero.

But, aside from that, the bits of code relevant to your test case look fine, assuming
- GetNumClasses() is just an accessor for CDatabase's m_nNumClasses member?
- the test case is more or less

void Test()
{
    CDatabase classData;

    for(int i = 0; 10 > i; ++i)
    {
        CClassroom *ptr = new CClassroom;
        char temp[32] = "";
        sprintf(temp, "Classroom #%d", i + 1);
        ptr->SetName(temp);
        classData.AddClassroom(ptr);
    }

    if(classData.GetNumClasses() != 10)
    {
        cout << "GetNumClasses returned " << classData.GetNumClasses() << " but       that's wrong! \n";
        cout << "Error adding the class to the database" << endl;

        return false;
    }

    // etc

    return true;
}

Andy

PS Due to the DEBUG_NEW, I assume you're using VC++. If so, are you compiling at warning level 4? If not, it might of helped you spot the misuse of operator== and the missing returns.

[degausser]

Following your instructions, I have made the following corrections:
- CDatabase::operator= is now returning *this at the end of it.
- CDatabase::CreateDatabase: added 'return true' at the end of it.
- I fixed the dumb mistake on my part on line 65 - it is now using the assignment operator instead.
- Fixed the default constructor to initalize m_nTeacherID to 0.

- Your assumption is correct! That's all it's doing.

I'm using Code::Blocks, and it never gave me a warning for that particular part, or I never really paid attention, heh.

Anyway, after all these changes, I still have the same problem: I can add a student, teacher and class perfectly fine.

However, once I choose to print the class roster or a student's schedule (a student whom I've added), it won't display the roster - it'll display the teacher teaching the class, but it won't display the students enrolled in said class.
When I ask to display a student's schedule, it won't show the class they're enrolled in.

Finally, thank you so much for taking the time out of your day and review my code - I know it's not especially short, and a thank you is very much deserved.

[cire]

From a cursory examination of the code that you posted here, I have a few questions.

What is the member m_nClassSize supposed to represent? If, as I suspect, it is meant to indicate the number of students enrolled in the class, it is not being used correctly. It is initially set to zero, never changed, and used as a condition in for loops that will never execute.

What is the member m__pStudent supposed to be pointing to? An array of student ids? If it is supposed to be an array of student ids, what is CClassroom::SetStudentEnrolled supposed to be doing? The set and get function seem superfluous to me. Shouldn't AddStudent look something like the following?

void CClassroom::AddStudent(int nStudentID)
{
    if ( !IsStudentEnrolled(nStudentID) )    // student is not already enrolled?
    {
        m_pStudent[m_nClassSize] = nStudentID ;   // Store the id
        ++m_nClassSize ;                          // increase the class size.
    }
} 

Also, I don't see the point of using dynamic memory in your class. Just using a normal array would work fine the way you're managing memory.

[degausser]

(1) m_nClassSize is in fact supposed to indicate the number of students enrolled in the class. I don't know how to fix that, though.

(2) It's an array of student IDs. As far as SetStudentEnrolled, I don't know.
The function prototypes were given to us by our professor - we're supposed to fill them in.

(3) When I first started doing the project, I was doing it with normal arrays. However, as the semester came closer to being over, we started going over dynamic memory in class, and as such our professor told us to implement that into our project.

Finally, your code snippet of the altered AddStudent method made it so that this program now works perfectly.
Honestly, there is so much stuff going on in this program I completely overlooked that part of it, heh.

Thank you so much for everything, seriously. You're a life saver!

[andywestken]

If you're using g++, check your compiler options include -Wall and -pedantic, to get it to warn you as much as possible.

Andy

[degausser]

I added those. -Wall was already included, -pedantic wasn't. Thanks!

[andywestken]

One last thing. To avoid the effort of pasting files individually up on codepad.org, you could use a site like mediafire.com, where you can upload whole zip files, etc.

Mediafire
http://www.mediafire.com/

Andy

[degausser]

Yeah, but if I went that route, how many people would actually download code from a stranger? I don't think many would.

Pasting my code on sites like codepad just seemed like the better route in this case, but thanks!

[andywestken]

I'd download a .zip/.gz/... file full of source, but not an executable. So not a self-extractor.

But I see what you mean: if they couldn't see it was safe up front.

Andy

[TheIdeasMan]

Just some other things:

I notice you have some get / set functions in your classes - we had a huge discussion about why these can be bad here:

http://www.cplusplus.com/forum/lounge/101305/

The public set functions are particularly bad - you wouldn't want anyone to set your bank balance to $0.00 would you?

Instead, make use of constructors with initialiser lists.

The get functions are much more benign, but you could think about why you need them. I notice you only use them in the test function - presumably a release version won't have this.

Another way of doing this is to have a Test Class, and utilise send & receive functions. The object class has a Send function which calls the Receive function in the Test class, with the data being sent as arguments. That way, in the release version you just get rid of one send function per class and omit the Test Class altogether.

All this might seem overkill for this example, but is better design IMO, because you no longer have get / set functions.

Remember that having public get & set functions is the same as making the member variables public.

Prefer to use std::string rather than char arrays.

I use the -Wextra compiler flag as well, as I understand it - it provides warnings that -Wall and -pedantic don't.

Another idea is to not use using namepace std; . you can do several things:

1. Put std:: before each std thing.
2. Have using statements like these for frequently used things:

using std::cout;
using std::cin;
using std::endl;

using std::string;

 

3. Do a mixture of the first 2.


Any way I hope this helps, and provides a little info to make your code better in the future. Cheers 8+)

[andywestken]

@TheIdeasMan

I don't disagree with your comments, but I think degausser might be a bit too early in his/her career for some of your suggestions (e.g. send & receive functions.); I assume he/she is working with what he/she has learnt so far.

The way the program is coded looks fine to me, for someone who is just learning about C++ and classes, but not yet got onto strings and vectors. I think worrying about details too early can be distracting. I can see other issues which I would address first -- but that's a job for the professor, I feel.

All being well, the professor -- who provided the leakdetection code -- will introduce the ideas you've mentioned as the course progresses.

@degausser

I think this is quite a good problem (a class hierarchy, memory allocation, ... ). When you've learnt more you could revisit it and see what you think then? But I think what you're written is pretty good, though there are issues your professor should pick you up on!

Andy

[TheIdeasMan]

andywestken wrote:

.... but I think degausser might be a bit too early in his career for some of your suggestions .....

No worries Andy :+) Well at least the germ of an idea is there - as you said degausser can look at it whenever it suits.

EDIT:

IMO, having an idea presented early is good - I have only just learnt the ideas for avoiding get / set functions (thanks to ne555), and I wish I had known it much earlier.

And yes, I second your comment that degausser's code was a pretty good effort.

[degausser]

Thanks guys.

@TheIdeasMan, see, when we first started going over getters/setters, I thought the same thing - isn't that just making the private members of a class public? But, seeing as how we had really just begun to go over them, I didn't think much of it.
Also, the semester is basically over - I just have to go in for my final this week, but my next class is Data Structures and I'm assuming we'll be covering the topics you guys brought up then?

If not I'd definitely like to learn more about how I could've made my code better!

[andywestken]

@degausser

Does your professor provide you with good, reference versions of the exercises to compare your work against, or just comment on your code?

@TheIdeaMan

Learning ideas as early as possible is good. But I've also come across the notion that you can only (really) learn so many on each step; so you should focus on a limited number of ideas at a time, getting to really know them, before moving on.

(I think the setter/getter debate ended up going a bit too far. Setters and getters do have their place. It's just that they should not be used unnecessarily or inappropriately, and do tend to get overused. But I don't think that can be properly discussed until someone has a grasp of the basics of object oriented design: first get used to classes in a simple way, then look at them in more detail. There are structural issues with degausser's code that, when repaired, would lead to elimination of most--but not all--of the setters.)

Andy