C ++中对象的集合（代码审查） [英] Collection of objects in C++ (code review)
问题描述
Hello Everyone，
我创建了以下代码段，我得到了预期的行为，但也许你有更好的方式来分享我去相同的结果。
以下是我想与大家分享的代码：
#include < iostream >
#include < memory >
#include < vector >
#include < 算法 >
使用 命名空间标准;
模板< class T， size_t N>
constexpr size_t size（T（&）[N]）{返回 N; }
class 组
{
private ：
vector< signed long> _channels;
int _id { 0 };
public ：
Group（）= delete ;
组（已签名 长渠道[]， size_t 大小， int id）
{
_channels.assign（渠道，渠道+大小）;
_id = id;
}
int getId（） const
{
return _id;
}
int channelCount（） const
{
return _channels.size（）;
}
已签名 long getChannel（ int index） const
{
if （index< 0 || index> = channelCount（））
throw < span class =code-string> 无效的频道索引;
return _channels [index];
}
vector< signed long>频道（） const
{
return _channels;
}
};
class 群组
{
private ：
vector< unique_ptr< Group>> _groups;
int _groupIndex { 0 };
public ：
Groups（）{};
int 添加（签名 long channels []， size_t size）
{
_groupIndex ++;
_groups.push_back（make_unique< Group>（channels，size，_groupIndex））;
return _groupIndex;
}
int 大小（） const
{
return _groups.size（）;
}
void 清除（）
{
return _groups.clear（）;
}
组运算符 []（ int index）
{
return getIten（index）;
}
组getIten（ int index）
{
if （index< 0 || index> = Size（））
throw 无效的组索引;
return * _groups [index] .get（）;
}
};
int main（）
{
群组;
签名 long grp1Channels [] = { 0 ， 3 ， 5 };
groups.Add（grp1Channels，size（grp1Channels））;
已签名 long grp2Channels [] = { 6 ， 8 ， 10 };
groups.Add（grp2Channels，size（grp2Channels））;
cout<< groups.Size（）<< ENDL;
cout<< groups [ 0 ]。channelCount（）<< ENDL;
for （ signed long 频道：群组[ 0 ]。频道（））
{
cout<< channel<< ENDL;
}
cout<<组[ 0 ]。getId（）<< ENDL;
return 0 ;
}
输出如下：
>>> $ main<<<<
2
3
0
3
5
< span class =code-digit> 1
我关注getItem看看是否返回Group而不是组指针很好。目前这是处于只读模式，但是如果我想改变一个频道的值，会发生什么。你觉得最灵活的方式是什么？
非常感谢你提前。
祝你好运。
MiQi
我的尝试：
我有创建了上面的代码。
main<<<<
2
3
0
3
5
< span class =code-digit> 1
我关注getItem看看是否返回Group而不是组指针很好。目前这是处于只读模式，但是如果我想改变一个频道的值，会发生什么。你觉得最灵活的方式是什么？
非常感谢你提前。
祝你好运。
MiQi
我的尝试：
我有创建了上面的代码。
只需用编写一个SetChannel函数[]运营商到您的频道。这是正常的编码模式。
void SetChannel（Group& group， int index）
{
_channels [index] = group;
}检查索引是否有效，以及使用引用或指针优化代码以避免临时对象。 （这有时可能是个大问题）
从技术上讲，你只需要一个std :: vector< std :: vector< long>>。您可以重写您编写为全局函数的其他几个成员函数。然后，您可以利用std :: vector的全部功能，而不仅仅是您在Group of Groups实现中提供的非常有限的选择。而且你可以充分利用该实现的高效率，而不是想知道你自己的实现是不是最理想的。
但是要回答你的问题，我注意到了您应该修复以下内容，因为它是一种常见的编码约定。如果你覆盖你的类的索引操作符，那么你应该提供两个变体：一个应该声明为const并返回所需元素的副本，另一个不应该是const，并返回对该元素的引用，以便它可以从外部修改：
const 组 operator []（ int ） const ; // 返回副本
Group& operator []（ int ）; // 返回参考
如果你这样做，它间接回答了你的问题：在你当前的实现中，getIten（）具有与索引操作符完全相同的功能（范围检查除外）。因此，您应该提供两个版本：一个是const函数返回一个副本，另一个是非const返回一个引用。
请注意，这个答案是基于常见的编码约定与性能无关。当然，遵循通用编码约定通常也有助于获得良好的性能。 ; - ）
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.
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.
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)
Technically, you have little more than astd::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. ;-)
这篇关于C ++中对象的集合（代码审查）的文章就介绍到这了，希望我们推荐的答案对大家有所帮助，也希望大家多多支持IT屋！
