I figured out the problem. There was an if conditional that should have been an if-else conditional. (Also I had accidentally submitted that post before I was finished writing it).
1 Like
I am glad that you were able to find the issue. Logic errors are the worst because they are not always easy to spot. I know that this was for a class, but if you are not using test units and you plan on getting into the industry, then you will need to turn that into a habit.
Java was my first language and C was my second. I never count basic because it has no real practical uses but yeah, I too was a 1337 h4xx0r of the TI calculators. I started off with TI Basic because I got tired of people taking my calculator and connecting to it to dump my games and notes. So I made an AIDS like virus that would systematically degrade the offending personâs calculator. I got into trouble with this because it spread like wildfire at my high school. But people learned to leave my stuff alone.
2 Likes
I do use test units. I did use test units on this one, but maybe not as much as I should have. To be fair, this is the largest and most complex program I have ever written for school. I feel like I underestimated it because of the fact that my program thus far have been rather pathetically easy for me. (At the same time, I am not an object-oriented person).
1 Like
For example, I used this program as a test unit for the printInvalidEmails() method:
#include<iostream>
#include<string>
#include<regex>
int main() {
std::string studentEmails[5] = {...}; //I manually entered the emails here
std::smatch validEmailMatches;
std::regex validMatch(R"[\w_\.]+@\w+\.[a-z]{2,3})");
bool validEmails[5];
int index;
for (index=0; index<5; index++) validEmails[index] = regex_match(studentEmails[index], validMatch);
std::cout << "The following emails have failed to match proper email format: " << std::endl << std::endl;
for (index=0; index<5; index++)
{
if (validEmails[index] std::cout << studentEmails[index] << std::endl;
}
return 0;
}
Also, currently, I am having trouble with the remove function definition.
void Roster::remove(const string& studentID)
{
int deleteIndex = getByID(studentID);
try
{
if (deleteIndex == -1) throw runtime_error("Function, getByID(), returned -1. No matching student was found.");
else
{
string name = classRosterArray[deleteIndex]->getFirstName() + ' ' + classRosterArray[deleteIndex]->getLastName();
cout << "Removing student, " << name << ", from the class roster. Please wait...\n";
//Where classRosterSize is a class private variable that is initialized to 5 by the constructor.
for (; deleteIndex < classRosterSize-1; deleteIndex++)
classRosterArray[deleteIndex] = classRosterArray[deleteIndex + 1];
delete classRosterArray[--classRosterSize];
cout << "Successfully removed, " << name << " from the class roster." << endl << endl;
}
}
catch(runtime_error& except)
{
cout << except.what() << endl;
cout << "Cannot delete student from roster." << endl;
}
}
A segmentation fault occurs where I run
delete classRosterArray[--classRosterSize];
Not deleting an index from the classRoster results in two bugs. A duplicate entry in the student roster occurs, and the destructor breaks because thereâs an index that isnât fully deleted from the classRosterArray. Maybe I am trying to implement the remove function wrong.
You could try classRosterSize - 1 instead and see if it just does not like the shorthand.
Someone else correct me if I am wrong, but it is good practice for you to use set and get methods when modifying class private variables. May be a Java convention.
If you are going to modify them directly, at least ensure the right scoping by using the this identifier. If this ->classRosterSize bombs out, then you should get a better trace with gdb.
There are a couple ways that you can deal with the destructor of an index. The quick and dirty is to set it to null and rely on the destruction of the roster class at the end to take care of the memory use. Not the best practice but you are not writing enterprise grade software here. Adain, gdb would be your friend here. Set a break point and see what is actually happening.
Nah, itâs not the shorthand thatâs the problem.
So I have it in a better state than it was, but Iâve hit a roadblock. It seems that that last bug is a cockroach - it just wonât die. Iâm thinking that maybe I have a bad implementation of the destructor.
What is it doing now?
Same thing as before. Though instead of trying to delete the variable after removing the elements in the array, I just didnât do that. As it stands, thereâs a segmentation fault after calling remove the second time - but when the student doesnât exist in the classrosterarray
If the classRosterArray is empty, youâll try to delete outside the array bounds. This perhaps might be the problem, difficult to tell.
I found a logic error. Idk why I did this, but for some reason I changed the for loop to stop when deleteIndex <= this->classRosterSize instead of <. Changing it back to < changed the error to a sigabort error instead of a segmentation fault. It also causes the output to be incorrect again. It outputs the last element twice which tells me that classRosterArray[4] still exists. However, when I try to delete classRosterArray[4] it puts me back at square 1. Though tbh, I guess now I am at square 1, but that wasnât the solution even though it is the logical solution. Ugh.
So I have edited the code this much to make it hopefully better.
string name = this->classRosterArray[deleteIndex]->getFullName();
this->classRosterSize -= 1;
cout << "Removing student, " << name << ", from the class roster. Please wait...\n";
for (; deleteIndex < this->classRosterSize; deleteIndex++)
this->classRosterArray[deleteIndex] = this->classRosterArray[deleteIndex + 1];
this->classRosterArray[this->classRosterSize] = {};
cout << "Successfully removed, " << name << " from the class roster." << endl << endl;
It never gets to that last statement⌠with or without me setting the last element of the array to an empty allocation. (Idk if that truly deallocates it tbh). Whatâs curious, though, is that it does get to print off the classRosterArray using the printAll method called in main. The output is different when I remove the deallocation attempt right after the for loop versus when I have it there. Actually, having the deallocation attempt provides me with an ideal output.
Ah, the joys of getting bounds checking right and off by one errors.
This is why container types are one of the most valuable things in every language standard library.
In any case the for loop lgtm.
Whatâs inside the array?
With the = in that for loop, youâre doing copy assignment for each element of the array, one by one. As a rule of thumb, nothing else in your code should be holding any references to any elements of it, or any members of these elements while that memory shuffling is happening. That other code might have a bad time with everything shifting.
On the other hand, if theyâre just pointers to Student, then each Student doesnât really change with your for loop, as youâd just be copy assigning pointers.
Re⌠endl endl were you perhaps reaching for a std::flush(), thinking output should be line buffered, or did you really want two blank lines.
Edit: nevermind I see ->GetFullName( theyâre pointers.
I donât understand most of what you were trying to say tbh.
Yes, I am trying to reassign elements in the array. The method that that code is in is called void Roster::remove. Earlier in the code, deleteIndex is set to a value that the user has chosen to remove. I can paste the full version of the function definition. Iâll also show you the void Roster::add to show you how the student pointers are added to the classRosterArray.
Roster::Roster()
{
// This defines the classRosterArray of pointers.
for (auto& index : this->classRosterArray) index = nullptr;
// rosterIndex corresponds to a single index within the array.
// This variable is modified when the add function is called.
this->rosterIndex = -1;
// classRosterSize is the current size of the classRosterArray.
// This variable is modified when the remove function is called.
this->classRosterSize = 5;
}
Roster::~Roster()
{
// Automatically iterate through the classRosterArray and delete the data within.
for (auto& index : this->classRosterArray) index = {};
// Delete the classRosterArray.
delete *this->classRosterArray;
}
void Roster::add(const string& studentID, const string& firstName, const string& lastName, const string& emailAddress,
int age, int daysInCourse1, int daysInCourse2, int daysInCourse3, DegreeProgram degreeProgram)
{
int daysInCourseArray[3] = {daysInCourse1, daysInCourse2, daysInCourse3};
try
{
if (this->rosterIndex + 1 < 5) this->classRosterArray[++this->rosterIndex] =
new Student(studentID, firstName, lastName, emailAddress, age, daysInCourseArray, degreeProgram);
else throw runtime_error("Index out of range.");
}
catch(runtime_error& except)
{
cout << except.what() << endl;
cout << "Cannot add anymore students to the roster." << endl;
}
}
void Roster::remove(const string& studentID)
{
int deleteIndex = this->getByID(studentID);
if (deleteIndex == -1) cout << "Error: No student matching provided ID was found." << endl <<
"Cannot remove student from the roster.";
else
{
string name = this->classRosterArray[deleteIndex]->getFullName();
this->classRosterSize -= 1;
cout << "Removing student, " << name << ", from the class roster. Please wait...\n";
for (; deleteIndex < this->classRosterSize; deleteIndex++)
this->classRosterArray[deleteIndex] = this->classRosterArray[deleteIndex + 1];
this->classRosterArray[this->classRosterSize] = {};
cout << "Successfully removed, " << name << " from the class roster." << endl << endl;
}
}
Edit: I also decided to paste the constructor and destructor.
Was throwing an error in the printAll () function. Added a null check and it runs to completion.
void Roster::printAll() {
// Automatically index the classRosterArray and call the print() method for each student.
for (auto& index : classRosterArray) {
if (index == nullptr) { continue; }
index->print();
}
}⌠An array is a contiguous* piece of memory of a fixed size**, you can deallocate a whole array, but it always takes a fixed amount of memory.
Because the array is an array of pointers, the last = {} is the same as = nullptr. Youâve just overwritten the last thing in the array.
Since youâve been shifting pointers around⌠the only Student you want deallocated / deleted is this one from the middle: this->classRosterArray[deleteIndex], and thatâs only assuming that student was allocated using new previously, and assuming nothing else is using it. Since youâre creating a new Student in add method, you should use delete in the remove method to get rid of it.
âŚ
now that Iâm looking at it, hereâs a perhaps academic exercise (academic, cause any sane person would use an std::vector here)
What do you think about replacing the loop with:
classRosterSize--;
std::swap(classRosterArray[deleteIndex], classRosterArray[classRosterSize]);
or simply doing
classRosterArray[deleteIndex]=nullptr;
? (thereâs tradeoffs, but does any of that violate any ordering requirements?)
You need to pick whether youâre using nullptr or classRosterSize and whether you need a contiguous list of students or not.
In first case, youâre reordering Students, but touching less memory, and youâre just hiding a student, so you need to not print it. Also if you call add after remove, youâd be leaking memory unless you delete the student.
In the second case, where youâre relying that empty slots in your array are nullptr, you need a mechanism of tracking these empty slots for the case when you might want to call add after calling remove, and your print function needs to not try to print a Student from that array that nullptr is pointing at.
2 Likes