Structures, Classes, Parent and children...

[Geckoo]

Hello. Coding some single scripts using the Main() section in order to execute the whole code, now I am trying to understand how I should "cut" my code so as to gain some readability and maintenance. I made a simple example using a main class sorting some hypothetical tracks and movies which are on my shelves that are related to children Classes. It's just an example - not an efficient program. I want to understand how I should ponder a full process into efficient parts. Please take a look at the code and tell me what is no good - what could be improved. You are more skilled than me - and I know that your help will be clever and efficient. Thank you ++

How it works : User creates a new object using the Class createNewItem. According to its parameters, createNewItem selects one of the overloaded functions and creates an object for Track or DIVX. Each vector displays their objects at the end. It is for me a good exercie so as to integrate structures, classes, vectors, children/parent ++

#include <iostream>
#include <string>
#include <vector>
// class for musics
class myTracks {

public:
    int ID; // function declaration
    std::string duration(int d);

public:
    struct CD {
        std::string artist;
        std::string title;
        int year;
        std::string duration;
    };

    struct CD cd;
};
// function definition
std::string myTracks::duration(int d)
{
    int m = d / 60;
    int rSeconds = d % 60;
    // convert int seconds to minute (m's")
    std::string mm = std::to_string(m);
    std::string ss = std::to_string(rSeconds);

    return mm + "\'" + ss + "\"";
}
// class for movies
class myDIVX {

public:
    int ID;

public:
    struct movie {
        std::string director;
        std::string title;
        int year;
    };

    struct movie movie;
};
// a vector for each object
std::vector<myTracks> records;
std::vector<myDIVX> movies;
// a parent class item and its children
class item : public myTracks, public myDIVX {

public:
    enum category { Music, Movies, MusicAndMovies };
    // functions declaration - functions overloading
    void createNewItem(int id, std::string artist, std::string title, int year, int seconds);
    void createNewItem(int id, std::string director, std::string title, int year);
    // just for info
    inline std::string displayCategory(item::category c)
    {
        switch (c)
        {
        case item::category::Music:      
            return "A new entry in the Music section";
        case item::category::Movies:     
            return "A new entry in the Movies section";
        case item::category::MusicAndMovies:
            return "A new entry in the Music And Movies section";
        default:
            return "[Unknown]";
        }
    }
};
// function definition
void item::createNewItem(int ID, std::string artist, std::string title, int year, int seconds)
{
    myTracks myObj;

    myObj.ID = ID;

    myObj.cd.artist = artist;
    myObj.cd.title = title;
    myObj.cd.year = year;
    myObj.cd.duration = myObj.duration(seconds);

    records.push_back(myObj);   
}
// function declaration
void item::createNewItem(int ID, std::string director, std::string title, int year)
{
    myDIVX myObj;

    myObj.ID = ID;

    myObj.movie.director = director;
    myObj.movie.title = title;
    myObj.movie.year = year;

    movies.push_back(myObj);
}

int main() 
{
    item first;
    //std::cout << first.displayCategory(first.Music) << std::endl;
    first.createNewItem(1, "Jimi Hendrix", "Red House", 1967, 224);

    item second;
    //std::cout << second.displayCategory(second.Music) << std::endl;
    second.createNewItem(2, "Fleetwood Mac", "Little Lies", 1987, 216);

    item third;
    //std::cout << third.displayCategory(third.Music) << std::endl;
    third.createNewItem(3, "Green Day", "When I come Around", 1995, 178);

    item fourth;
    //std::cout << fourth.displayCategory(fourth.MusicAndMovies) << std::endl;
    fourth.createNewItem(4, "Prince", "Purple Rain", 1984, 245);
    fourth.createNewItem(1, "Albert Magnoli", "Purple Rain", 1984);

    item fifth;
    //std::cout << fifth.displayCategory(fifth.Movies) << std::endl;
    fifth.createNewItem(2, "Ridley Scott", "Blade Runner", 1982);

    std::cout << std::endl;
    std::cout << "*** Music section ***" << std::endl;
    std::cout << std::endl;

    for (auto o : records)
    {
        std::cout << o.cd.artist << std::endl;
        std::cout << o.cd.title << std::endl;
        std::cout << o.cd.year << std::endl;
        std::cout << o.cd.duration << std::endl;
        std::cout << std::endl;
    }

    std::cout << "*** Movies section ***" << std::endl;
    std::cout << std::endl;

    for (auto o : movies)
    {
        std::cout << o.movie.director << std::endl;
        std::cout << o.movie.title << std::endl;
        std::cout << o.movie.year << std::endl;
        std::cout << std::endl;
    }

    std::cout << "You have " << records.size() << " tracks in your playlist" << std::endl;
    std::cout << "You have " << movies.size() << " movies in your folder" << std::endl;

    return 0;
}

[seeplus]

As a quick look, a class/struct should not refer to global variables. records and movies should be part of main and passed as params to functions as required. These classes shouldn't know anything about records/movies. These describe 1 movie or 1 record.

[seeplus]

As a first re-factor (without splitting into private/public), consider:

#include <iostream>
#include <string>
#include <vector>

// class for music
class Track {
public:
	int tId {};

	struct CD {
		std::string artist;
		std::string title;
		int year {};
		std::string duration;

		CD() {}
		CD(const std::string& artist_, const std::string title_, int year_, int secs) :
			artist(artist_), title(title_), year(year_), duration(sduration(secs)) {}
	};

	CD cd;

	static std::string sduration(int d);

	Track() {}
	Track(int tId_, const std::string& artist_, const std::string& title_, int year_, int seconds_) :
		tId(tId_), cd(artist_, title_, year_, seconds_) {}
};

std::string Track::sduration(int d) {
	return std::to_string(d / 60) + "\'" + std::to_string(d % 60) + "\"";
}

// class for movies
class Divx {
public:
	int dId {};

	struct movie {
		std::string director;
		std::string title;
		int year {};

		movie() {}
		movie(const std::string& direc, const std::string& title_, int y) :
			director(direc), title(title_), year(y) {}
	};

	movie movie;

	Divx() {}
	Divx(int id, const std::string& director, const std::string& title, int year) :
		dId(id), movie(director, title, year) {}
};

// a parent class item and its children
class Item : public Track, public Divx {
public:
	enum Category {
		Music, Movies, MusicAndMovies
	};

	Item(int id, const std::string& artist, const std::string& title, int year, int seconds);
	Item(int id, const std::string& director, const std::string& title, int year);

	static std::string displayCategory(Item::Category c) {
		switch (c) {
			case Item::Category::Music:
				return "A new entry in the Music section";
			case Item::Category::Movies:
				return "A new entry in the Movies section";
			case Item::Category::MusicAndMovies:
				return "A new entry in the Music And Movies section";
			default:
				return "[Unknown]";
		}
	}

	Category cat;
};

Item::Item(int ID, const std::string& artist, const std::string& title, int year, int seconds) :
	Track { ID, artist, title, year, seconds }, cat { Category::Music } {}

Item::Item(int ID, const std::string& director, const std::string& title, int year) :
	Divx { ID, director, title, year }, cat { Category::Movies } {}

int main() {
	std::vector<Item> collect;

	collect.emplace_back(1, "Jimi Hendrix", "Red House", 1967, 224);
	collect.emplace_back(2, "Fleetwood Mac", "Little Lies", 1987, 216);
	collect.emplace_back(3, "Green Day", "When I come Around", 1995, 178);
	collect.emplace_back(4, "Prince", "Purple Rain", 1984, 245);

	collect.emplace_back(1, "Albert Magnoli", "Purple Rain", 1984);
	collect.emplace_back(2, "Ridley Scott", "Blade Runner", 1982);

	std::cout << "\n*** Music section ***\n";

	size_t records {}, movies {};

	for (const auto& o : collect)
		if (o.cat == Item::Category::Music) {
			++records;
			std::cout << o.cd.artist << '\n';
			std::cout << o.cd.title << '\n';
			std::cout << o.cd.year << '\n';
			std::cout << o.cd.duration << '\n';
		}

	std::cout << "\n*** Movies section ***\n" << '\n';

	for (const auto& o : collect)
		if (o.cat == Item::Category::Movies) {
			++movies;
			std::cout << o.movie.director << '\n';
			std::cout << o.movie.title << '\n';
			std::cout << o.movie.year << '\n';
		}

	std::cout << "You have " << records << " tracks in your playlist\n";
	std::cout << "You have " << movies << " movies in your folder\n";
}

But using this method, each Item holds info for both Track and Divx - with only 1 used.

Have you considered using polymorphism here?

[jonnin]

I do not like local structs inside classes. Its fine, but as soon as I do that I will discover at an inconvenient time that it should have been its own object because I want to reuse it or get at it outside that object or whatever else. I just don't do it anymore. You can argue the other side, that they are so tightly coupled that it belongs there and nothing else would ever need to get at it, but from experience that is a rare thing.

consistency is a big part of design. if an audio CD has a duration, why does a movie not? Consistency helps you in many ways: it makes it easier to condense your objects into fewer types, so you can tap inheritance and other tools easier. If you don't need it, fine.. but it feels weird to have it here and not there. I think if you poked at this design, you might find that you can make a base class with like 10 strings, an ID number, a duration, and a couple of other similar fields that can be used for a CD, DVD, or other item via inheritance. It hard to think this way, but working backwards from 'what does everything have in common' to build the specific "what does only a DVD have, what does only an audio CD have" is the way to build up such a thing.

[Geckoo]

Thank you for all your comments which help me. At first, don't try to understand the coherence of this script. I am not developing an application in order to sort my items. It's just like an exercise. Don't try to understand why there is a duration for Tracks and not for Movies - this is not the goal. @seeplus - thank you for the refactoring. I guess that I have to take a look at Polymorphism with C++. It could be better using this way. Thank you ++

[Geckoo]

Hum. It seems to me that C++ is more a matter of thinking than an understanding of semantics and computation (if you understand this sentence, I offer you a cookie). How to organize software design around objects, rather than functions and logic? Captivating.

I will post soon another version of this disorganized code - something more logical according to OOP ++

[seeplus]

It seems to me that C++ is more a matter of thinking than an understanding of semantics and computation (if you understand this sentence, I offer you a cookie). How to organize software design around objects, rather than functions and logic? Captivating.

For OOP, yes. This applies to design in any OOP language - not just C++. The OOP design is extremely important. There are many books/internet resources just devoted to OOP design - just do a search for OOP design!

[jonnin]

Yes, "but" each object is an awful lot like a small, classroom sized program, where the class variables are kinda like 'main' variables or even globals, and from there it largely breaks back down into functions and logic again. Its a layer on top of the functions and objects for organization, at its core. Lots of complex stuff in there beyond that, but sometimes it helps to see it this way?

[Geckoo]

Hello. This is the second attempt to reach an OOP concept for a simple process. In the next code, I create a parent class with all parameters which are common to all items - title, year and duration. However each child has its own single parameter which makes a difference between a musician and a director. This way I can reach some polymorphism - I think.

#include <iostream>
#include <string>
#include <vector>
// parent
class Item {

public:
    std::string duration(int d);
    void update_info(std::string title, int year, int duration);

public:
    struct common_info {
        std::string title;
        int year {};
        std::string duration;
    };

    struct common_info ci;
};

std::string Item::duration(int d)
{
    return std::to_string(d / 60) + "\'" + std::to_string(d % 60) + "\"";
}
// update common info for this item
void Item::update_info(std::string title, int year, int seconds)
{
    ci.title = title;
    ci.year = year;
    ci.duration = Item::duration(seconds);
}
// child for music
class Track : public Item {

public:
    std::string artist;
};
// child for movies
class Movie : public Item {

public:
    std::string director;
};

std::vector<Track> tracks;
std::vector<Movie> movies;

void createNewItem(int cat, std::string who, std::string title, int year, int seconds)
{
    if (cat == 1) 
    {
        Track t;
        t.artist = who;
        Item* i = &t;
        // common parameters
        i->update_info(title, year, seconds);
        tracks.push_back(t);
    }

    if (cat == 2)
    {
        Movie m;
        m.director = who;
        Item* i = &m;
        // common parameters
        i->update_info(title, year, seconds);
        movies.push_back(m);
    }
}

int main() 
{
    createNewItem(1, "Jimi Hendrix", "Red House", 1967, 224);
    createNewItem(1, "Fleetwood Mac", "Little Lies", 1987, 216);
    createNewItem(1, "Green Day", "When I come Around", 1995, 178);
    createNewItem(2, "Ridley Scott", "Blade Runner", 1982, 7025);
    createNewItem(2, "Tarkovsky Andrei", "Stalker", 1979, 9800);

    std::cout << "** Music section **" << std::endl;

    for (auto &tt : tracks)
    {
        std::cout << tt.artist << std::endl;
        std::cout << tt.ci.title << std::endl;
        std::cout << tt.ci.year << std::endl;
        std::cout << tt.ci.duration << std::endl;
        std::cout << std::endl;
    }

    std::cout << "** Movies section **" << std::endl;

    for (auto &mm : movies)
    {
        std::cout << mm.director << std::endl;
        std::cout << mm.ci.title << std::endl;
        std::cout << mm.ci.year << std::endl;
        std::cout << mm.ci.duration << std::endl;
        std::cout << std::endl;
    }

    std::cout << "You have " << tracks.size() << " tracks in your playlist\n";
    std::cout << "You have " << movies.size() << " movies in your folder\n";

    return 0;
}

I guess that it could be improved. Let me know. Thanks ++

[seeplus]

L18 - you don't need to specify struct as part of the type.

L8 - as duration() doesn't use any of the class's members it can be static.

L9 - pass std::string by const ref not by value to avoid an unnecessary copy. or L28 use std::move(). Same for L48 et al.

cat - You used enum for this in the previous. This is better than using 'unknown' numbers.

L45, 46 - tracks and movies shouldn't be global.

L52 - 57 - You don't need to deal with pointers. You can just do:

Track t;

    t.artist = who;
    t.update_info(title, year, seconds);
    tracks.push_back(t);

Same for L62 - 67

If common_info, Item, Track and Movie have appropriate constructors, then you don't need update_info - you can use emplace_back() rather than push_back().

It also doesn't make sense here to have common_Info as a separate struct in Item. Why not have these are direct member variables?

Keeping the global variables and common_info, then something like:

#include <iostream>
#include <string>
#include <vector>

// parent
class Item {
public:
	enum class Media {TRACK, MOVIE };

	static std::string sduration(int d);

	Item(const std::string& title, int year, int duration) : ci(title, year, duration) {}

	struct common_info {
		std::string title;
		int year {};
		std::string duration;

		common_info(const std::string& title_, int year_, int duration_) :
			title(title_), year(year_), duration(sduration(duration_)) {}
	};

	common_info ci;
};

std::string Item::sduration(int d) {
	return std::to_string(d / 60) + "\'" + std::to_string(d % 60) + "\"";
}

// child for music
class Track : public Item {
public:
	Track(const std::string& who, const std::string& title, int year, int seconds) :
		artist(who), Item(title, year, seconds) {}

	std::string artist;
};

// child for movies
class Movie : public Item {
public:
	Movie(const std::string& who, const std::string& title, int year, int seconds) :
		director(who), Item(title, year, seconds) {}

	std::string director;
};

std::vector<Track> tracks;
std::vector<Movie> movies;

void createNewItem(Item::Media cat, const std::string& who, const std::string& title, int year, int seconds) {
	if (cat == Item::Media::TRACK)
		tracks.emplace_back(who, title, year, seconds);
	else if (cat == Item::Media::MOVIE)
		movies.emplace_back(who, title, year, seconds);
}

int main() {
	createNewItem(Item::Media::TRACK, "Jimi Hendrix", "Red House", 1967, 224);
	createNewItem(Item::Media::TRACK, "Fleetwood Mac", "Little Lies", 1987, 216);
	createNewItem(Item::Media::TRACK, "Green Day", "When I come Around", 1995, 178);
	createNewItem(Item::Media::MOVIE, "Ridley Scott", "Blade Runner", 1982, 7025);
	createNewItem(Item::Media::MOVIE, "Tarkovsky Andrei", "Stalker", 1979, 9800);

	std::cout << "** Music section **\n";

	for (const auto& tt : tracks) {
		std::cout << tt.artist << '\n';
		std::cout << tt.ci.title << '\n';
		std::cout << tt.ci.year << '\n';
		std::cout << tt.ci.duration << '\n';
		std::cout << '\n';
	}

	std::cout << "** Movies section **\n";

	for (const auto& mm : movies) {
		std::cout << mm.director << '\n';
		std::cout << mm.ci.title << '\n';
		std::cout << mm.ci.year << '\n';
		std::cout << mm.ci.duration << '\n';
		std::cout << '\n';
	}

	std::cout << "You have " << tracks.size() << " tracks in your playlist\n";
	std::cout << "You have " << movies.size() << " movies in your folder\n";
}

[Geckoo]

Impressive. Really good explanation. Nice example. Thank you for your help.
If I understand correctly, we can set an object directly using its class constructor? Maybe I don't use the right words, but I guess that you understand me. In a single line I saw that you create/set/store a new object using the function createNewItem().

[seeplus]

Yes - L53, 55 using .emplace_back which implicitly uses the constructor - rather than having to explicitly create a new object and then use .push_back().

A constructor can set the object members directly as well as create the object. Different constructors can be specified if they have different arguments/number of arguments.

[Geckoo]

Some overloaded constructor(s) ?
Another question. I use two vectors in my example. How can I use just one - maybe something with std::variant or std::any?

[JLBorges]

> I use two vectors in my example. How can I use just one?

Make the base class polymorphic. For example:

#include <iostream>
#include <string>
#include <set>
#include <iomanip>

// item.h
struct item
{
    std::string title ;
    int year = 2022 ;
    int duration = 1 ; // seconds

    item( std::string title, int year, int duration )
        : title(std::move(title)), year(year), duration(duration) { items.emplace(this) ; }

    virtual ~item() { items.erase(this) ; }

    virtual std::ostream& print( std::ostream& stm ) const = 0 ;

    static std::set<item*> items ; // http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-ptr

    protected:
        // http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-copy-virtual
        item( const item& ) = default ;
        item( item&& ) = default ;
        item& operator= ( const item& ) = default ;
        item& operator= ( item&& ) = default ;

    friend std::ostream& operator<< ( std::ostream& stm, const item& it ) { return it.print(stm) ; }
};

// item.cpp
std::ostream& item::print( std::ostream& stm ) const
{
    const char pf = stm.fill() ;
    return stm << title << ", " << year << ", " << duration/60 << ':'
               << std::setfill('0') << std::setw(2) << duration%60 << std::setfill(pf) ;
}

std::set<item*> item::items ;


struct track : item
{
    std::string artiste ;
    // ...
    track( std::string title, int year, int duration, std::string artiste )
        : item( title, year, duration ), artiste( std::move(artiste) ) {}

    virtual std::ostream& print( std::ostream& stm ) const override
    { return item::print( stm << "track{ " ) << ", " << artiste << " }" ; }
};

struct movie : item
{
    std::string director ;
    // ...
    movie( std::string title, int year, int duration, std::string director )
        : item( title, year, duration ), director( std::move(director) ) {}

    virtual std::ostream& print( std::ostream& stm ) const override
    { return item::print( stm << "movie{ " ) << ", " << director << " }" ; }
};

int main()
{
    const track autumn( "Autumn in New York", 1952, 236, "Lady Day" ) ;
    const movie stalker( "Stalker", 1979, 9677, "Tarkovsky" ) ;
    const movie story( "Tokyo Story", 1953, 136*60, "Ozu" );
    const track blue( "So What", 1959, 562, "Miles Davis" ) ;

    std::cout << autumn << '\n' << stalker << '\n' << story << '\n' << blue << "\n------------\n" ;

    for( const item* it : item::items ) std::cout << *it << '\n' ;
}

https://coliru.stacked-crooked.com/a/b31f2b5ecf2d58a5

[jonnin]

Another question. I use two vectors in my example. How can I use just one - maybe something with std::variant or std::any?

That is OK too ^^^ and you can also do it with unions or other crude tools.
If ALL you need is a container of different types of things, crude works. Polymorphism brings you a LOT of power, but it can also cause you to have to rewrite a lot of code when you missed it in the design, and the crude approaches repair that with minimal surgery. Don't forget that they are possible, in other words, but try to avoid them if you have time to do something better.

[Geckoo]

Thanks everyone for your comments.

@JLBorges Your code is really interesting with a beautiful (but really strange) format. I don't understand why you use some Structures instead of the previous Classes. I guess that you have a reason. Another unclear point - what mean the next lines?

item( const item& ) = default ;
item( item&& ) = default ;
item& operator= ( const item& ) = default ;
item& operator= ( item&& ) = default ;

I read the examples which are available on the shared link, but it's just Chinese for me :/

[seeplus]

The difference between a class and a struct is the default behaviour. For a struct, the default is all public. For a class, the default is all private. If you have everything public/protected (as here) then using a struct saves having to specify public: !!

Often though, a struct is used where there are no member functions (other than constructors(s)/destructor) and a class is used elsewhere. But this is by convention/coding guidance and is not part of the standard.

[jonnin]

he is manually setting/forcing those methods to the compiler generated default (and forcing the compiler to do so even if not needed or used?) versions rather than create custom ones. I am not sure that is necessary here (??) since they are generated by default in the same way as he did. You can also remove them so the compiler does NOT generate the defaults, which is often critical to do (same as above except delete instead of default for the keyword).

[Geckoo]

The difference between a class and a struct is the default behaviour. For a struct, the default is all public. For a class, the default is all private.

he is manually setting/forcing those methods to the compiler generated default (and forcing the compiler to do so even if not needed or used?) versions rather than create custom ones.

Clear. Thanks ++

[seeplus]

what mean the next lines

it generates the standard copy/move constructor/assignment but as protected members - not as public which they otherwise would have been. See his reference just below protected: