Collection of objects in C++ (code review)

Question

0.00/5 (No votes)

See more:

Hello Everyone,

I have created the following code segment, I get the expected behavior but maybe do you have a better way to share me to get to the same result.

Here is the code I would like to share you:

C++

#include <iostream>
#include <memory>
#include <vector>
#include <algorithm> 

using namespace std;

template<class T, size_t N>
constexpr size_t size(T (&)[N]) { return N; }

class Group
{
private:
    vector<signed long> _channels;
    int                 _id{0};

public:

    Group() = delete;
    Group(signed long channels[], size_t size, int id)
    {
        _channels.assign(channels, channels + size);
        _id = id;
    }

    int getId() const
    {
        return _id;
    }
    
    int channelCount() const
    {
        return _channels.size();
    }
    
    signed long getChannel(int index) const
    {
        if (index < 0 || index >= channelCount())
            throw "Invalid channel index";
            
        return _channels[index];
    }
    
    vector<signed long> Channels() const
    {
        return _channels;
    }
    
};

class Groups
{
private:
    vector<unique_ptr<Group>> _groups;
    int _groupIndex{0};
    
public:

    Groups(){};
    
    int Add(signed long channels[], size_t size)
    {
        _groupIndex++;
        _groups.push_back(make_unique<Group>(channels, size, _groupIndex));
        return _groupIndex;
    }
    
    int Size() const
    {
        return _groups.size();
    }
    
    void Clear() 
    {
        return _groups.clear();
    }
    
    Group operator[](int index)
    {
        return getIten(index);
    }
        
    Group getIten(int index)
    {
        if (index < 0 || index >= Size())
            throw "Invalid group index";
        
        return *_groups[index].get();
    }
    
};

int main()
{
   Groups groups;
   
   signed long grp1Channels[] = {0, 3, 5};
   groups.Add(grp1Channels, size(grp1Channels));
   
   signed long grp2Channels[] = {6, 8, 10};
   groups.Add(grp2Channels, size(grp2Channels));
   
   cout<< groups.Size() << endl;
   cout<< groups[0].channelCount() << endl;
   
   for (signed long channel : groups[0].Channels()) 
   {
     cout << channel << endl;
   }
   
   cout<< groups[0].getId() << endl;
   
   return 0;
}

The output is the following:

C++

>>> $main  <<<<
2
3
0
3
5
1

I have concerned on the getItem to see if returning Group and not Group pointer is fine. Currently this is in readonly mode but what happens if I want to change the value of a channel for example. What do you think would be the best flexible way ?

Thank you very much in advance.
Best regards.
MiQi

What I have tried:

I have created the code above.

Posted 15-May-19 11:59am

SuperMiQi

Updated 16-May-19 2:15am

Add a Solution

Comments

[no name] 15-May-19 19:12pm

Life is too short to mess with things that "work" just to see if it can be "better" (in this case).

https://martinfowler.com/bliki/Yagni.html

2 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

KarstenK · Answer 1 · 2019-05-15T20:18:00

Solution 1

Simply write a SetChannel function with the [] operator to your channels. That is the normal coding pattern.

C++

void SetChannel(Group &group, int index)
{
  _channels[index] = group;
}

Check that index is valid und for optimal code work with references or pointers to avoid temporary objects. (that can be a big issue sometimes)

Posted 15-May-19 20:18pm

KarstenK

Comments

SuperMiQi 16-May-19 8:20am

Thank you very much for your reply.

Stefan_Lang · Answer 2 · 2019-05-16T02:15:00

Technically, you have little more than a std::vector<std::vector<long>> . You could rewrite the few additional member functions you wrote as global functions. Then you could take advantage of the full functionality of std::vector rather than just the very limited selection you happened to provide in your Group of Groups implementation. And you can take full advantage of the high efficiency of that implementation, rather than wondering if your own implementation is suboptimal.

But to answer your question, I've noticed the following that you should fix, simply because it's a common coding convention. If you override an index operator for your class, then you should supply two variants: one should be declared const and return a copy of the required element, the other should not be const, and return a reference to that element, so that it can be modified from outside:

C++

const Group operator[](int) const; // return a copy
Group& operator[](int); // return a reference

If you do this, it indirectly answers your question: in your current implementation, getIten() has the exact same functionality as the index operator (except for the range check). Therefore you should provide two versions: one a const function returning a copy, and the other non-const returning a reference.

Note that this answer is based on common coding conventions and has little to do with performance. Although of course, following common coding conventions is typically helps to obtain good performance, too. ;-)