Click here to Skip to main content
14,084,501 members
Rate this:
 
Please Sign up or sign in to vote.
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:

#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:

>>> $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
Updated 2 days ago
Comments
Gerry Schmitz 3 days ago
   
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
Rate this: bad
 
good
Please Sign up or sign in to vote.

Solution 1

Simply write a SetChannel function with the [] operator to your channels. That is the normal coding pattern.
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)
   
Comments
SuperMiQi 2 days ago
   
Thank you very much for your reply.
Rate this: bad
 
good
Please Sign up or sign in to vote.

Solution 2

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:
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. ;-)
   
Comments
SuperMiQi 2 days ago
   
This is indeed what I was looking for.
Thank you very much for your reply.

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

  Print Answers RSS
Top Experts
Last 24hrsThis month


Advertise | Privacy | Cookies | Terms of Service
Web02 | 2.8.190518.1 | Last Updated 16 May 2019
Copyright © CodeProject, 1999-2019
All Rights Reserved.
Layout: fixed | fluid

CodeProject, 503-250 Ferrand Drive Toronto Ontario, M3C 3G8 Canada +1 416-849-8900 x 100