Thread: What's wrong with the CDs* findCDs fuctions?

  1. #1
    Registered User
    Join Date
    Apr 2016
    Posts
    9

    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

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    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.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    Registered User
    Join Date
    Apr 2016
    Posts
    9
    We were told to use these struct definitions, plus we don't do OOP.

  4. #4
    Registered User
    Join Date
    Apr 2016
    Posts
    9
    plus my professor hates vectors

  5. #5
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    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.
    Last edited by laserlight; 04-27-2016 at 04:48 AM.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #6
    Registered User
    Join Date
    Apr 2016
    Posts
    9
    That's what my uncle said! My professor said that you don't need to include another library.

    Quote Originally Posted by laserlight View Post
    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)
    .

  7. #7
    Registered User
    Join Date
    Apr 2016
    Posts
    9
    I had not see any of this before I was asked to write this program.

  8. #8
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    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.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. vectors and fuctions
    By mixalissen in forum C++ Programming
    Replies: 13
    Last Post: 05-05-2008, 11:48 AM
  2. Problem with cos, sin fuctions
    By xxxhunter in forum C Programming
    Replies: 7
    Last Post: 11-16-2007, 03:33 AM
  3. 2 fuctions please
    By dac in forum C++ Programming
    Replies: 3
    Last Post: 02-13-2006, 05:55 AM
  4. Need help with fuctions
    By stillwell in forum C++ Programming
    Replies: 13
    Last Post: 01-25-2005, 10:09 AM
  5. Replies: 8
    Last Post: 04-11-2004, 11:19 PM

Tags for this Thread