1

As part of a BookGroup class that manages an array of Book objects, I'm asked to create a void add(Book* b) member function that adds the given Book b to an array of books in its correct place (from oldest to most recent year of publication). I'm required to shift the elements in the array towards the back of the array to make room for the new element in its correct place. I am not allowed to simply add to the end of the array and then sort or use any sorting function/sorting algorithm on the array.

I tried testing my add function and I get a seg fault. My approach was to add any new book at the end of the array and if that specific book's publication year was older (number is less) than the last book in the array, I would make the two books swap places. If not, the book would stay in the same spot at the very end of the array. I then continue this process.

I don't know what's causing the seg fault. As a side note, I was wondering if I'm supposed to use the delete function at any point in add()? I did a valgrind check on my compiler and it says there are a bunch of bytes lost somewhere in my program. My guess is that a good chunk of the bytes are probably coming from the add function, but I'm not sure and just wanted to double check.

bookCollection is supposed to be a statically allocated array of Book object pointers. There are two classes - Book.cc and BookGroup.cc.

I decided to show all of my code so people can compile it, but please post only what is necessary and refrain from posting all of it in the answers below.

BookGroup.cc:

#include <iostream>
#include <iomanip>
using namespace std;
#include "BookGroup.h"

BookGroup::BookGroup(int n){ 
  numOfBooks = n; 
}

void BookGroup::add(Book* b){ 

    if(numOfBooks != MAX_BOOKS){
        if(numOfBooks == 0){
            bookCollection[0] = b; //add first element
            ++numOfBooks; //increase numOfBooks by 1 and go to next statement
        }else{
            for(int i = numOfBooks-1; i >= 0; --i){ //start at end of array and work towards front where lowest years are
                if(b->getPubYear() < bookCollection[i]->getPubYear()){
                    bookCollection[i + 1] = bookCollection[i]; //swap positions if b is lower than last element
                    bookCollection[i] = b;
                }else{
                    b = bookCollection[i + 1]; //otherwise stay in the same spot (keep b at the end)
                    //break;
                }
            }
            ++numOfBooks;
        }

    }
    cout<<"Book could not be added to collection. No more space "<<endl; 
}

BookGroup.h:

#ifndef BOOKGROUP_H
#define BOOKGROUP_H
#define MAX_BOOKS 15
#include <string>
using namespace std;

class BookGroup
{
    public:
        BookGroup(int);
        BookGroup(BookGroup&);  
        ~BookGroup();
        void print();
        void add(Book*);
        Book* bookCollection[MAX_BOOKS];

    private: 
        int numOfBooks;

};
#endif

Book.cc: https://pastebin.com/9swrwYgx

Book.h: https://pastebin.com/mqDn2C30

makefile: https://pastebin.com/xHKDsVL1

main: https://pastebin.com/TBzyduMC

When I try to run it:

Declaring two book groups...

Initializing two book groups...

-- default Book ctor: Peter pan year: 1982

Segmentation fault

amouteru
  • 17
  • 3
  • Provide code as code-formatted text ( https://stackoverflow.com/editing-help ) right here, instead of linking, to make a [mre] please. – Yunnosch Feb 04 '20 at 05:40
  • Is `MAX_BOOKS` the size of the array? Keep in mind that an array of size N has valid indexes from 0 to N-1. Accessing index N at any point is an almost guaranteed method for segfaults. But please make an MRE, to avoid people having to ask clarification questions like this. – Yunnosch Feb 04 '20 at 05:46
  • MAX_BOOKS is the maximum number of books allowed in a book group. The variable numOfBooks counts the current number of books in the array – amouteru Feb 04 '20 at 05:47
  • Sure. But once you approach the max number your loop is set up to access beyond the arrays size, assuming that MAX_BOOKS is also the size of the array. So please, is that the arrays size? And please make an MRE. – Yunnosch Feb 04 '20 at 05:48
  • Making an MRE includes provoking the problem you describe and providing error message (including valgrind) as full, verbatim quotes in text form (i.e. not pictures). If any line numbers are mentioned, make sure that these lines can be identified in your MRE (lines numbers are not good, since they might change with editing). Label the lines with comments `// 1st valgrind message here`. If you get a segfault please spend some time pinpointing it. I.e. which line causes the segfault. https://ericlippert.com/2014/03/05/how-to-debug-small-programs/ https://stackoverflow.com/questions/2069367/how-to – Yunnosch Feb 04 '20 at 05:52
  • Please try (I know this is hard) to minimise your MRE. If you only have problems while inserting books, then removing everything not related to initialising or inserting should be removed. Try for the smallest code which can demonstrate your problem (i.e. keep it functional). This also helps pinpointing the error. – Yunnosch Feb 04 '20 at 05:56
  • You are doing well, adding information. Try inserting a few more outputs, to get closer to where the segfault occurs. Or use a debugger. – Yunnosch Feb 04 '20 at 05:59
  • For an assignment question this is quite good. See my comments above to improve it even further. Also have a look at https://meta.stackoverflow.com/questions/334822/how-do-i-ask-and-answer-homework-questions , but just for some background and for making it possibly even better, because it is already nice. Hope I could help you with my answer. Good look for your further participation on StackOverflow. – Yunnosch Feb 04 '20 at 06:32

1 Answers1

0

This

BookGroup::BookGroup(int n){ 
  numOfBooks = n; 
}

together with this

BookGroup suzy(2);

creates book groups which do not contain any pointers to valid books but DO pretend to contain 2.

Then here

for(int i = numOfBooks-1; i >= 0; --i)
{ //start at end of array and work towards front where lowest years are
    if(b->getPubYear() < bookCollection[i]->getPubYear()){

you start accessing index 1 (because of numBooks==2), which is not a valid pointer.

You should

  • correctly initialise your array, e.g. with NULL, to make sure that checks work cleanly later
  • double check that you only use valid pointer, everywhere, if necessary twice
  • not initialise an empty group with a non-zero number of pretended books
Yunnosch
  • 26,130
  • 9
  • 42
  • 54