首页
学习
活动
专区
圈层
工具
发布
社区首页 >问答首页 >我自己的性病::载体

我自己的性病::载体
EN

Code Review用户
提问于 2016-08-10 21:55:47
回答 3查看 3.5K关注 0票数 4

我已经创建了自己的std::vector,但这是我第一次使用模板和新/删除。代码工作,但肯定有很多事情是错误的。如果我用正确的方式编码,你能读懂这段代码并告诉我吗?

(main是一种测试。)

代码语言:javascript
复制
#ifndef __STDVECTOR__
#define __STDVECTOR__

#include <iostream>

using namespace std;

template <typename T>
    class StdVector{
        private:
            T *buffer;
            unsigned int capacity;
        public:
            //Constructor.
            StdVector(){
                capacity=0;
                buffer=new T[capacity];
            }
            //Copy constructor.
            StdVector(const StdVector &asv){
                int i;

                capacity=asv.getCapacity();
                buffer=new T[asv.getCapacity()];
                for (i=0; i<capacity; i++){
                    buffer[i]=asv[i];
                }
            }
            //Destructor.
            ~StdVector(){
                delete []buffer;
            }
            void push_back(T obj){
                StdVector oldSV(*this);
                int i;

                capacity++;
                delete []buffer;
                buffer=new T[capacity];
                for (i=0; i<oldSV.getCapacity(); i++){
                    buffer[i]=oldSV[i];
                }
                buffer[i]=obj;
            };
            T getBuffer() const{
                if (capacity==0){
                    throw exception();
                }
                return *buffer;
            };
            T &operator[](int index) const{
                if (index>=capacity){
                    //Out of range.
                    throw exception();
                }
                else{
                    return buffer[index];
                }
            }
            StdVector &operator=(const StdVector &obj){
                capacity=obj.getCapacity();
                delete []buffer;
                buffer=new T[capacity];
                buffer=obj.getBuffer();
                return *this;
            }
            unsigned int getCapacity() const{
                return capacity;
            };
    };

#endif

int main(){
    try{
        StdVector<int> test;
        StdVector<string> test2;
        unsigned int i;

        test.push_back(5);
        test.push_back(4);
        test.push_back(3);
        test.push_back(2);
        test.push_back(1);
        test.push_back(0);
        test.push_back(-1);
        test.push_back(-2);
        test.push_back(-3);
        test.push_back(-4);
        test.push_back(-5);
        for (i=0; i<test.getCapacity(); i++){
            cout << test[i] << endl;
        }
        test2.push_back("Hello");
        test2.push_back(" ");
        test2.push_back("World");
        test2.push_back(".");
        cout << "---------------" << endl;
        for (i=0; i<test2.getCapacity(); i++){
            cout << test2[i];
        }
        cout << endl;
    }
    catch(...){
        cout << "Exception." << endl;
    }
    return 0;
}
EN

回答 3

Code Review用户

回答已采纳

发布于 2016-08-11 10:32:09

不要使用双下划线.

代码语言:javascript
复制
#ifndef __STDVECTOR__
#define __STDVECTOR__

具有双下划线的标识符保留在实现中。

请参阅:在C++标识符中使用下划线的规则是什么?

停止使用

代码语言:javascript
复制
using namespace std;

这类东西能破解这么多代码。将其放入头文件将使您在开放源码项目中被禁止使用,因为您破坏了包含头的任何编译单元的全局命名空间。即使在您自己的源文件中,这也是个坏主意,因为它可能会导致很难发现错误。

请参阅:为什么C++中的“使用命名空间性病”被认为是不好的做法?

不要构建未添加的对象。

你目前的设计有缺陷。

代码语言:javascript
复制
    template <typename T>
    class StdVector{
            T *buffer;
            ...
                buffer=new T[capacity];

因为您只有一个大小值(一个capacity),所以您的代码变得非常低效率(正如我们在向后推时所看到的那样)。

你应该有两个尺码。

  1. 容量:为向量中的对象分配的空间数量。
  2. 大小:向量中的对象数。

如果没有这两种大小,它们必须是相同的值。这意味着您不能预先分配空间,每次添加或删除元素时,都必须调整向量的大小,这包括分配新空间和将所有元素复制到新分配的空间中。

我在文章中详细介绍了以下内容:

向量-资源管理分配

更喜欢使用初始化程序列表.

代码语言:javascript
复制
        StdVector(){
            capacity=0;
            buffer=new T[capacity];
        }

这一切都很好而且很有效。但这是个坏习惯。如果更改成员类型,则可能会使类效率低下,因为成员是在类主体输入之前构造的。然后你在身体里修改它们。

代码语言:javascript
复制
        StdVector()
            : buffer(new T[capacity])
            , capacity(0)
        {}

三的

规则您的赋值运算符在代码中的位置很低。我一开始以为你违反了三条规则。将赋值运算符靠近构造函数。

赋值操作符不例外安全.

这个赋值操作符是经典的第一次尝试。

代码语言:javascript
复制
            StdVector &operator=(const StdVector &obj){
                capacity=obj.getCapacity();
                delete []buffer;
                buffer=new T[capacity];
                buffer=obj.getBuffer();
                return *this;
            }

但是,如果存在异常(在T的构造函数中),则容易泄漏并使对象处于不一致的状态。

你应该使用“复制”和“互换”成语。

代码语言:javascript
复制
            StdVector &operator=(StdVector tmp) // notice the pass by value
            {                                   // this creates a copy.
                tmp.swap(*this);
                return *this;
            }
            void swap(StdVector& other) noexcept
            {
                using std::swap;
                swap(capacity,   other.capacity);
                swap(buffer,     other.buffer);
            }

我在以下文章中详细介绍了这一点:

向量-资源管理副本交换

推倒无效的

每次添加元素时,都要复制整个向量。但是你做了两次这样做是为了使你的设计所带来的最初的低效更加低效。

代码语言:javascript
复制
            void push_back(T obj){
                StdVector oldSV(*this);
                int i;

                capacity++;
                delete []buffer;
                buffer=new T[capacity];
                for (i=0; i<oldSV.getCapacity(); i++){
                    buffer[i]=oldSV[i];
                }
                buffer[i]=obj;
            };

你可以扔掉这样的一本:

代码语言:javascript
复制
            void push_back(T obj){    
                int newCapacity = capacity + 1;
                T*  newBuffer   = new T[newCapacity];

                for (int i = 0; i < capacity; ++i){
                    newBuffer[i]=capacity[i];
                }
                swap(capacity, newCapacity);
                swap(buffer,   newBuffer);
                delete [] newBuffer;
            };

还是不太好。但比原来的要好。

用于循环递增迭代器的

.

  1. 更喜欢内联地声明循环变量。
  2. 更喜欢使用前缀增量(并不是所有迭代器都像int那样有效)。

如下所示:

代码语言:javascript
复制
                for (int i = 0; i < capacity; ++i){
                    newBuffer[i]=capacity[i];
                }

不好命名的方法.

这不能得到缓冲区。

代码语言:javascript
复制
            T getBuffer() const{
                if (capacity==0){
                    throw exception();
                }
                return *buffer;
            };

它返回向量中第一个元素的副本。

效率.

通常,在C++中,operator[]不检查对元素的访问。因为如果调用代码已经进行了检查,则不需要为方法中的检查支付费用。

代码语言:javascript
复制
             T &operator[](int index) const{
                if (index>=capacity){
                    //Out of range.
                    throw exception();
                }
                else{
                    return buffer[index];
                }
            }

为了给我们检查访问,我们通常实现函数T& at(int index)。这为调用代码不检查的情况提供了对向量的检查访问。

代码语言:javascript
复制
for(int loop = 0;loop < v.size(); ++loop)
{
    v[loop] = stuff(); // no need to check `loop` is in bounds.
                       // we know it is in bounds because of the context.
}

int index;
std::cin >> index;
std::cout << v.at(index) << "\n"; // here we want checked access.

我会写:

代码语言:javascript
复制
T& operator[](int index)  {return buffer[index];}
T& at(int index)          {checkIndex(index);return buffer[index];}

Const正确性

代码语言:javascript
复制
             T &operator[](int index) const

这个函数不正确。您保证不会通过将函数标记为const来改变对象,但是返回一个非const的引用,从而允许对象发生变异。

代码语言:javascript
复制
 void bla(StdVector<int> const& data)
 {
     data[5] = 8; // You just mutated a const object.
 }

您应该为这个操作符定义两个版本,一个用于const,另一个用于非const用法。

代码语言:javascript
复制
  T const&  operator[](int index) const;
  T&        operator[](int index);
票数 7
EN

Code Review用户

发布于 2016-08-11 00:35:05

  • operator=显然,这部分代码从未被执行过。尝试使用operator=无法编译:错误:从‘int’到‘int*’-fpermissive buffer=obj.getBuffer()的无效转换;实际上getBuffer返回T。更糟糕的是,operator=严重地泄漏了内存: buffer=new T容量;buffer=obj.getBuffer();与new一起分配的东西永远丢失。可能需要的是buffer = new 容量;std:: copy ( obj.buffer,obj.buffer+ capacity,buffer);(与复制构造函数中的操作相同)。
  • operator[]返回一个非const指针,即fooX = y;即使fooconst向量也会编译。要正确处理const对象,需要两种形式的下标操作符: T& operator整型;const & operator整型 const;BTW,索引参数应为size_t。整数不适合表示什么是索引。沿着同一条线,如果索引超出容量,则为throw exception(),但允许具有负索引的访问。
票数 5
EN

Code Review用户

发布于 2016-08-11 08:15:21

代码语言:javascript
复制
#ifndef __STDVECTOR__
#define __STDVECTOR__

避免前导/双下划线。来自C++标准:

17.6.4.3.2全局名称global.names某些名称和函数签名始终保留在实现中:

  • 每个包含双下划线__或以下划线开头的大写字母的名称都保留给实现,供任何使用。
  • 以下划线开头的每个名称都保留给实现,以便作为全局命名空间中的名称使用。
代码语言:javascript
复制
#include <iostream>

在库代码中尽量少使用#include,避免使用#include <iostream>。无论何时#include <iostream>,大多数实现都将静态构造函数包含到每个翻译单元中。不要把这个成本强加给那些使用你的库的人。<iostream>使用您的测试代码,因此您应该将#include <iostream>重新定位到那里。

代码语言:javascript
复制
unsigned int capacity;

考虑一下std::size_t而不是unsigned int。从标准来看,

18.2类型support.types

  1. 类型size_t是一个实现-defined无符号整数类型,它的大小足以包含任何对象的字节大小。
代码语言:javascript
复制
StdVector(){
    capacity=0;
    buffer=new T[capacity];
}

间距有助于区分不同的语言结构。考虑在适当的地方添加空格(例如,在{之前和=周围)。

您应该更喜欢成员初始化列表,而不是构造函数体中的分配。您将从非POD类型获得性能提高,通过所有成员的统一初始化(const/references需要成员初始化)提高可读性,并防止常见错误(例如,在设置错误之前使用)。

代码语言:javascript
复制
void push_back(T obj){
    StdVector oldSV(*this);
    int i;

    capacity++;
    delete []buffer;
    buffer=new T[capacity];
    for (i=0; i<oldSV.getCapacity(); i++){
        buffer[i]=oldSV[i];
    }
    buffer[i]=obj;
};

考虑哪些类型的值可以传递到函数中。虽然像原始类型和小对象(2-3个单词)这样的东西复制起来很便宜,但并不是所有的东西都很便宜。在模板类型的情况下,您不知道传入的对象的大小。通过const引用传递对象。

在真正需要使用变量之前,不要引入它。int i不需要在函数的顶部声明,您所做的只是要求读者跟踪函数中稍后才需要的变量。在for循环之前声明它。

考虑为用户提供强有力的例外保证(如果附加失败会产生副作用吗?)如果new抛出std::bad_array_new_length异常的某些变体会发生什么?

票数 1
EN
页面原文内容由Code Review提供。腾讯云小微IT领域专用引擎提供翻译支持
原文链接:

https://codereview.stackexchange.com/questions/138389

复制
相关文章

相似问题

领券
问题归档专栏文章快讯文章归档关键词归档开发者手册归档开发者手册 Section 归档