2
\$\begingroup\$

I have problems with dealing with derived classes that inherit from the same base class.

I have 2 types of songs:

  • Songs with lyrics that have: Title, Tags (sad, happy etc.), lyrics (one line), writer.
  • Instrumental song: Title, Tags (same as lyric songs), instrumentals (a string), performer, BPM.

I thought it would be a good idea to implement a base class Song, since Title and Tags are fields that are common to the both types of the songs. Then, implement the class LyricSong and InstrumentalSong that inherit from Song.

class Song
{

public:
    Song(std::string title, std::string tags);

    std::string getTitle() const { return this->_title; }
    std::string getTags() const { return this->_tags; }

    // Since i had the need in virtual functions and since all virtual functions have to be 
    // implemented, i used default string and int to be returned.
    virtual std::string getLyrics() const { return std::string(); }
    virtual std::string getLyricsBy() const { return std::string(); }
    virtual std::string getInstruments() const { return std::string(); }
    virtual std::string getPerformer() const { return std::string(); }
    virtual int getBPM() const { return 0; }

    static songContainer readSongsFromFile(std::string songsFileName);

private:
    std::string _title;
    std::string _tags;

};

//---------------------------------------------------------------------------//

class LyricSong : public Song
{

public:
    LyricSong(std::string title, std::string tags, std::string lyrics, std::string lyricsBy);

    std::string getLyrics() const { return this->_lyrics; }
    std::string getLyricsBy() const { return this->_lyricsBy; } 

private:
    std::string _lyrics;
    std::string _lyricsBy;
};

//---------------------------------------------------------------------------//    
class InstrumentalSong : public Song
{

public:
    InstrumentalSong(std::string title, std::string tags,\
                     std::string instruments, std::string performer, int BPM);

    std::string getInstruments() const { return this->_instruments; }
    std::string getPerformer() const { return this->_performer; }
    int getBPM() const { return this->_BPM; }


private:
    std::string _instruments;
    std::string _performer;
    int _BPM;
};

Issues that have arisen:

  • The code is cumbersome and messy.
  • Every time I'll need to add new song types, it will force me to modify Song (add more functions and make them virtual - Polymorphic issues).
  • Will there be a way to know what type of Song I work with, in case of iterating over an array of Song pointer that point to various song types?

Are there any common design principles that can help me with this?

\$\endgroup\$
2

3 Answers 3

2
\$\begingroup\$

There are a couple of things that you could do to improve your code.

Make your base class destructor virtual

By default, a compiler-generated destructor is concrete rather than virtual, but this leads to problems with collections of objects. See this question for more details on why.

Prefer to pass complex objects by reference

All of the constructors should take const std::string & as parameters to avoid pointless duplication of strings within the code.

Use polymorphism

In order to have a generic Song as a base class, only the things that are common to all songs should be in that. So getLyrics would only be implemented in LyricSong and getInstruments would only be implemented in InstrumentalSong. This means that a heterogenous collection of songs could only use functions (concrete or virtual) that were defined for the Song class. As a practical matter this is usually OK if the class design is a rational one. For example, let's say that the common thing you want to do is to print the song details.

class Song
{
public:
    Song(const std::string &title, const std::string &tags)
    : _title{title}, _tags{tags}
    {}
    virtual ~Song() {}

    std::string getTitle() const { return this->_title; }
    std::string getTags() const { return this->_tags; }
    friend std::ostream& operator<<(std::ostream& out, const Song &s) {
        return s.printTo(out);
    }

private:
    virtual std::ostream& printTo(std::ostream &out) const {
        return out;
    }
    std::string _title;
    std::string _tags;
};

Delegate to derived classes

As shown in the sample above, the base class has a friend function that prints the details. Delegating that function to derived classes can be accomplished by calling a virtual helper function called printTo in this example.

Provide constructor code

The constructor implementations for your classes were not included for review, which makes it a little harder to see what you're doing. To address that, I wrote these versions of your classes:

class LyricSong : public Song
{
public:
    LyricSong(const std::string &title, const std::string &tags, const std::string &lyrics, const std::string &lyricsBy)
    : Song{title, tags}, _lyrics{lyrics}, _lyricsBy{lyricsBy}
    {}
    std::string getLyrics() const { return this->_lyrics; }
    std::string getLyricsBy() const { return this->_lyricsBy; } 
    std::ostream& printTo(std::ostream &out) const {
          return out  << "\"" << getTitle() 
          << "\" with tags = " << getTags() 
          << " and lyrics = \"" << getLyrics() 
          << "\", lyrics by " << getLyricsBy()
          << "\n";
    }
private:
    std::string _lyrics;
    std::string _lyricsBy;
};

class InstrumentalSong : public Song
{
public:
    InstrumentalSong(const std::string &title, const std::string &tags,\
                     const std::string &instruments, const std::string &performer, int BPM)
    : Song{title, tags}, _instruments{instruments}, _performer{performer}, _BPM{BPM}
    {}
    std::string getInstruments() const { return this->_instruments; }
    std::string getPerformer() const { return this->_performer; }
    int getBPM() const { return this->_BPM; }
    std::ostream& printTo(std::ostream &out) const {
          return out  << "\"" << getTitle() 
          << "\" with tags = " << getTags() 
          << " and instruments = \"" << getInstruments() 
          << "\", performed by " << getPerformer()
          << ", at " << getBPM()
          << " BPM\n";
    }
private:
    std::string _instruments;
    std::string _performer;
    int _BPM;
};

Create a test function

In order to see how this is actually used, I created a test function and I'd encourage you to the same in future code you write. It's also often extremely useful for code reviews to show an example of how your code is intended to be used. In this case, I created a simple test function named getSong that returns a Song pointer that is one of the two derived kinds. I could have made it randomly return one or the other but chose instead to alternate. Finally, the main routine exercises the code using a std::unique_ptr so that destructors are automatically called.

#include <iostream>
#include <string>
#include <memory>
// #include the code shown above

Song *getSong(int type)
{
    std::string title{"Song of "};
    if (type & 1) {
        title += std::to_string(type);
        return new LyricSong(title, "happy", "la la", "Mr. Singer");
    } else {
        title += std::to_string(type);
        return new InstrumentalSong(title, "sad", "sax", "Mr. Reed", 60); 
    }
}

int main()
{
    for (int i=0; i < 12; ++i) {
        std::unique_ptr<Song> c{getSong(i)};
        std::cout << "song #" << i << " is " << *c;
    }
}

Don't include a collection in a virtual base class

Your songContainer is better left as an external rather than internal variable. Otherwise it would only be possible to have a single collection of songs for any given program instance which is an unnecessary restriction on its functionality.

\$\endgroup\$
3
\$\begingroup\$

Polymorphic classes must include a virtual destructor. Otherwise they are not truly polymorphic: you cannot delete such an instance using the pointer to the base class.

A class hierarchy should (typically) be designed according to the Liskov Substitution Principle. This means that any derived class must be substitutable for the base class: anywhere a base class is expected to work, the derived class must as well. Your hierarchy conforms to this, but I wanted to point it out.

Will there be a way to know what type of Song I work with, in case of iterating over an array of Song pointer that point to various song types?

That's what dynamic_cast is for - it returns nullptr when the object is not of the desired type.

Song * song = ...;
auto isong = dynamic_cast<InstrumentalSong*>(song);
if (isong) { 
  /* it's instrumental */
  ...
}
auto lsong = dynamic_cast<LyricSong*>(song);
if (lsong) {
  /* it's lyric */
  ...
}

The type of isong is deduced to InstrumentalSong*, etc.

A basic question to be asked is: do you even care about a class structure? What do you do with all that song data?

\$\endgroup\$
1
  • \$\begingroup\$ Good review! You might want add a note of caution about using dynamic_cast -- it can throw a std::bad_cast exception if used for a reference type rather than a pointer. \$\endgroup\$
    – Edward
    Aug 30, 2015 at 20:34
1
\$\begingroup\$

Your base class your LyricSong and InstrumentalSong should only have the methods that are in common to both. This means that BaseSong should only define the following methods:

  • getTitle
  • getTags

Then LyricSong can inherit from BaseSong and define:

  • getLyrics
  • getWriter

whereas InstrumentalSong can inherit from BaseSong and define:

  • getIntrumentals
  • getPerformer
  • getBPM

When you add a new kind of song, you can decide which class to inherit from based on which methods are valid for it. For instance, if it has all of the methods of InstrumentalSong plus some more, then inherit from InstrumentalSong. If it has the methods of LyricSong plus some more, then inherit from LyricSong, etc. This way you won't have to modify any of your existing classes when you create a new song class.

\$\endgroup\$
3
  • \$\begingroup\$ But lets assume we have : LyricSong lSong; and BaseSong* bSong = lSong..and then somewhere in the code: bSong->getBpm(); ..this will give an error..thats why i had to declare all other function as virtual functions in BaseSong \$\endgroup\$
    – Alex Goft
    Aug 30, 2015 at 19:10
  • \$\begingroup\$ What does the assignment say should happen if you try to call getBPM on a lyrical song? \$\endgroup\$
    – ErikR
    Aug 30, 2015 at 19:23
  • \$\begingroup\$ They main thing is that i need to be able to menage during run time a data structure of type BaseSong. But i think i can work with it by defining another field - Type (to prevent calling methods of not suitable song types)..what do you think? \$\endgroup\$
    – Alex Goft
    Aug 30, 2015 at 19:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.