Main  /  Edit  /  History  /   /  Users Area

坏代码2

1. 基类的析构函数必须是virtual或protected的

class IMaterialDownloadObserver
{
public:
  virtual void statusUpdated() {};
};

这是一个模块内使用的接口类,两个问题:

辩: 除了通过语言来实现一个功能,还需要尽可能通过语言来表达设计,包括设计意图和约束.通过额外的文档来解释这方面的设计内容既不方便,也不可行.这类设计应该直接通过代码来体现,代码即文档,还可以通过代码来检查错误使用.

2. 类定义要严谨

``` class NS_BASE_API EventSite { friend class EventSiteImpl; friend class EventSourceImpl; protected: EventSite(); virtual ~EventSite(); public: typedef std::set<EventSource*> EventSources;

Ns::HString toDebugString() const;
const EventSources& eventSources() const;

Ns::int64 eventOrder() const;

void eventOrder(Ns::int64 order);

private: EventSiteImpl* m_pImpl; };

EventSite意图作为基类,因此,ctor和dtor都置为protected是正确的.但是,作为常识, 应该认识到自定义了default ctor后,还需要考虑其他的编译器自动生成函数:copy ctor, assignment.C++11后还应该仔细分辨move ctor/assignment. 以C++98的要求来说,这里没有定义copy cotr和assignment,而默认生成的行为恰恰是有严重错误的:

EventSite::EventSite(){ m_pImpl = new EventSiteImpl(this); } EventSite::~EventSite(){ if (m_pImpl){ delete m_pImpl; m_pImpl = NULL; } } ```

如果发生copy/assign,将导致重复delete的错误. 实际上这里并不期望允许复制(好不好另说),因此应该禁止copy ctor和assignment. 这种代码错误是一种风格上的错误.

3. 避免在类设计阶段决定对象如何分配

void
EventFilter::destroy()
{
    NEUTRON_ASSERT(clearBaseVirtualCalled());
    this->onPreDelete();
    NEUTRON_ASSERT(testBaseVirtualCalled());

    delete this;
}

delete this意味着该对象必须是通过new创建的, 但即便是事实也不要在这里如此处理,可以通过shared_ptr或者定制的unique_ptr(带状态的deletor)负责销毁对象.另外,这里的onPreDelete()通常意味着错误的设计,因该要求派生类的析构函数正确地加以实现.

3. 对于错误,制止优于预防,预防优于检查,发现错误要制造巨大声响

bool testBaseVirtualCalled() const;         ///< function to test that a virtual's base method was called
bool clearBaseVirtualCalled() const;        ///< Function to clear a flag before calling a virtual method
bool setBaseVirtualCalled() const;  

据代码中注释的解释,它期望如下的使用方式:

void Base::nonVirtual(...){
    NEUTRON_ASSERT(clearBaseVirtualCalled());
    onVirtual(...);
    NEUTRON_ASSERT(testBaseVirtualCalled());

    delete this;
}

void Base::onVirtual(...){
    ...
    NEUTRON_ASSERT(setBaseVirtualCalled());
}

首先,注释中delete this是不恰当的,应属笔误. 其次,这种检查方式要求派生类必须插入一句setBaseVirtualCalled(),但是此种检查有何意义呢?如果要求派生类必须实现onVirtual,直接申明onVirtual(...) = 0更好.如果派生类没有实现onVirtual,=0的方法将制止派生类实例化,根本无需检查,也就不必啰嗦地到处写一句setBaseVirtualCalled(),基类的实现者也不必多此一举地通过 clear和test做任何检查.这是制止优于检查的例子.

4. 注意标志符作用域

// .h
namespace Na {
class MaterialDownloadOp : public...{};
}
//.cpp
using namespace Na;
using namespace Ns;
using namespace autodesk::platform::core::string;
using namespace Autodesk::ContentService;

MaterialDownloadOp::MaterialDownloadOp() : ...{...}
  1. 大量地using namespace增大了名称冲突的风险.可以使用namespace alias, 即: namespace alias = XXX;的形式,也鼓励使用using directive, 减少键入的同时注意避免名称冲突.
  2. 应该缩小using namespace的作用域,可以在函数内,甚至块内使用using namespace
  3. 不应该如上MaterialDownloadOp,先行using namespace, 继而在全局实现.这会导致当不同名字空间中同名标志符指向混乱,且不一定能被编译器检测到.

About

Wiki++ is a wiki engine powered by CppCMS web development framework.


Navigation

Main Page


Valid CSS | Valid XHTML 1.0