What's wrong with the CDs* findCDs fuctions?

It's supposed to find the artist the us

Code:

---

#include "CDs.h"
#include <iostream>
#include <string>
#include <cstring>
#include <fstream>


using namespace std;




//
CDs* createCDs(const char* filename,fstream &file)
{
       
        const char* _file;
        int max = 1;
        int min = 0;
        CD** cdPtrArray = new CD*[max];
        CDs* _cds = new CDs;
       
        file.open(filename);
       
        while (!file.eof())
        {
                int year, rating, num_tracks;
                string* title = new string[1];
                string* artist = new string[1];
                string* length = new string[1];
               
               
                file >> artist[0];
                file >> title[0];
                file >> year;
                file >> rating;
                file >> num_tracks;
                if (min == max)
                {
                        resize(_cds);
                }
               
                CD* c = createCD(artist, title, year, rating, num_tracks);
                cdPtrArray[min] = c;
               
                for (int i = 0; i < num_tracks; i++)
                {
                        file >> length[0];
                        file >> title[0];
                }
                min++;
        }       
       
        _cds -> num_cds = min;
        _cds -> max_cds = min;
        _cds -> cd_array = cdPtrArray;
        return _cds;
}


//
void destroyCDs (CDs* cds)
{
        int numCDs = cds -> num_cds;
        CD** _cd = cds -> cd_array;
       
        for ( int i = 0; i < numCDs; i++)
        {
                destroyCD (_cd[i]);
        }
        delete cds;
}


//
void displayCDs (CDs* cds)
{
        int numAlbum = cds -> num_cds;
       
        CD** _cd = cds -> cd_array;
       
       
        for (int i = 0; i < numAlbum; i++)
        { 
        cout << _cd[i] << endl;
        }
       
}


//
void resize (CDs* cds)
{
        int cd_nums = cds -> num_cds;
        int cd_max = cds -> max_cds;
        CD** cd = cds -> cd_array;
        CD** cdPtr = new CD*[cd_max * 2];
        for (int i = 0; i < cd_nums; i++)
        {
                cd[i] = cdPtr[i];
        }
        delete cd;
       
        cd_max * 2;
}


//
CDs* findCDs (CDs* cds, string* artist)
{
        CD* compact = NULL;
       
        int numAlbum = cds -> num_cds;
        CD** _cd = cds -> cd_array;
       
        for (int i = 0; i < numAlbum; i++)
        {
               
                if(artist == compact )
                {
                        _cd[i] = compact;
                        break;
                }
        }
        return cds;
}


#ifndef CDS_STRUCT
#define CDS_STRUCT


#include "CD.h"


using namespace std;


struct CDs
{
        CD** cd_array;
        int num_cds;
        int max_cds;
};






//
CDs* createCDs(string filename, fstream &file );


//
void destroyCDs(CDs* cds);


//
void displayCDs(CDs* cds);


//
void resize(CDs* cds);


//
CDs* findCDs(CDs* cds, string* artist);




#endif


#include "CD.h"
#include <iostream>


using namespace std;


//
CD* createCD (string* artist, string* title, int year, int rating, int num_tracks)
{
        CD* newCD = new CD;
        newCD -> artist = artist;
        newCD -> title = title;
        newCD -> year = year;
        newCD -> rating = rating;
        newCD -> num_tracks = num_tracks;
        newCD -> track_count = 0;
        newCD -> song_array = new Song*[num_tracks];
       
        return newCD;
}


//
void addSong (CD* cd, string* title, string* length)
{
        Song* song = createSong (title, length);
       
        int songNum = cd->track_count;
        cd -> song_array[songNum] = song;
        cd -> track_count++;
}


//
void destroyCD (CD* cd)
{
        int songNum        = cd->num_tracks;
        Song** cdSongs = cd->song_array;
       
        for ( int i = 0; i < songNum; i++)
        {
                destroySong (cdSongs[i]);
        }
        delete cd;
}

#ifndef CD_STRUCT
#define CD_STRUCT
#include <string>
#include "Song.h"


using namespace std;


struct CD
{
        string* artist;
        string* title;
        int year;
        int rating;
        int num_tracks;
        int track_count;
        Song** song_array;
};


//
CD* createCD(string* artist, string* title, int year, int rating,int num_tracks);


//
void addSong(CD* cd, string* title, string* length);


//
void destroyCD(CD* cd);


#endif

#include "Song.h"
#include <iostream>


using namespace std;


//
Song* createSong (string* title, string* length)
{
        Song* newSong = new Song;
        newSong -> title = title;
        newSong -> length = length;
        return newSong;
}


//
void displaySong (Song* song)
{
        cout << song -> title << "\t" << song -> length << endl;
}


//
void destroySong (Song* song)
{
        delete song;
}


#ifndef SONG_STRUCT
#define SONG_STRUCT


#include <string>


using namespace std;


struct Song
{
        string* title;
        string* length;
};


//
Song* createSong(string* title, string* length);


//
void displaySong(Song* song);


//
void destroySong(Song* song);


#endif

---

The way I see it, the problem is that you have manual memory management and an overuse of pointers.

Let's consider your CD struct:

Code:

---

struct CD
{
    string* artist;
    string* title;
    int year;
    int rating;
    int num_tracks;
    int track_count;
    Song** song_array;
};

---

Why did you make artist and title pointers? Are they optional? Even if they were optional, given that they are strings, it would have been simpler to allow them to be empty strings. Why is song_array a Song**? From what I see, song_array is a dynamic 1D array, and Song is not a polymorphic base class. Therefore, if you really want to use manual memory management, it should have just been a Song*.

Personally, I would have written:

Code:

---

struct Song
{
    std::string title;
    std::string length;
};

struct CD
{
    std::string artist;
    std::string title;
    int year;
    int rating;
    int num_tracks;
    int track_count;
    std::vector<Song> songs;
};

struct Album
{
    std::vector<CD> cds;
};

---

Notice that I fully qualify the names, e.g., std::string instead of just string. The reason is that using directives like using namespace std; should not appear at file scope in header files.

Now, implementing findCDs is much easier. Using pre-C++11 syntax, I might write:

Code:

---

std::vector<CD> findCDs(const Album& album, const std::string& artist)
{
    std::vector<CD> cds;
    for (std::vector<CD>::size_type i = 0; i < album.cds.size(); ++i)
    {
        if (album.cds[i].artist == artist)
        {
            cds.push_back(album.cds[i]);
        }
    }
    return cds;
}

---

Maybe you don't want copies of the CD objects to be inserted into the vector to be returned. If so, maybe you could return a vector of pointers instead, but you still don't need to do manual memory management.

We were told to use these struct definitions, plus we don't do OOP.

plus my professor hates vectors

Quote:

---

Originally Posted by skiian

We were told to use these struct definitions

---

Sorry, but your teacher does not know how to program in modern C++, or is over-complicating your assignment to make it unnecessarily more difficult, so much so that I doubt your teacher will be able to write a correct program with the same struct definitions and material taught to you (unless he/she taught you about exceptions, but you failed to catch them when you should).

Anyway, do you have control over your function declarations? At the very least I would have expected:

Code:

---

CDs* findCDs(const CDs* cds, const string& artist);

---

This way, you would create a new dynamic array of CDs found, then return a pointer to the first element of the dynamic array. There is really no point in artist being a pointer, so if your teacher is mandating that too, then I'd say that he/she is doing great harm to your learning.

Quote:

---

Originally Posted by skiian

plus we don't do OOP.

---

I have not involved any OOP in my examples beyond the same level of OOP as in your original code.

Quote:

---

Originally Posted by skiian

plus my professor hates vectors

---

What is the rationale for that? std::vector is not always appropriate, but it is the container closest to a simple dynamic array in the standard library. Hating it is irrational.

That's what my uncle said! My professor said that you don't need to include another library.

Quote:

---

Originally Posted by laserlight

Sorry, but your teacher does not know how to program in modern C++, or is over-complicating your assignment to make it unnecessarily more difficult, so much so that I doubt your teacher will be able to write a correct program with the same struct definitions and material taught to you (unless he/she taught you about exceptions, but you failed to catch them when you should)

---

.

I had not see any of this before I was asked to write this program.

Quote:

---

Originally Posted by skiian

My professor said that you don't need to include another library.

---

That is a lame reason since:

• It is part of the standard library.
• To achieve the same benefits of std::vector means implementing a library for each dynamic array or other resource, so you do need to include another library anyway, except that you are writing it yourself too, which is a waste of time and invites bugs to join you.
• struct CD represents three different dynamically allocated resources (artist, title, and song_array), so it becomes necessary to catch exceptions so as to conditionally free previous resource allocations. It is simply bad design in C++; at least in C where there is no exception mechanism it is alright.