Main  /  Edit  /  History  /   /  Users Area

坏代码1

1. mui/src/csui/csqt/widget/textbox.cpp(1249):

        bool TextBox::Validate()    {
            ...
            QRegExp r("[^\\\\/:*?\"<>|;=`,]+");
            if(!r.exactMatch(name))
            {
                return false;
            }
            return true;
        }

文件FusionDoc/UI/FusionDocUI/Cloud/FusionDocController.cpp:

if (pEvent->to() == Ns::OnlineState::eOnline) //Connected
    onConnectivityChanged(true);
else
    onConnectivityChanged(false);

应该简化成:

onConnectivityChanged(pEvent->to() == Ns::OnlineState::eOnline)

2. mui/src/csui/csqt/widget/textbox.cpp(1188):

            bool isLeft = false, isOuter = false;
            if (GetIconPosition() == OuterLeft)
                isLeft = true, isOuter = true;
            else if (GetIconPosition() == OuterRight)
                isLeft = false, isOuter = true;
            else if (GetIconPosition() == InnerLeft)
                isLeft = true, isOuter = false;
            else if (GetIconPosition() == InnerRight)
                isLeft = false, isOuter = false;

可以简化成:

            bool isLeft = GetIconPosition() == OuterLeft || GetIconPosition() == InnerLeft;
            bool isOuter = GetIconPosition() == OuterLeft || GetIconPosition() == OuterRight;

3. mui/src/csui/csqt/widget/textbox.cpp(1198):

            if (isLeft)
            {
                boxLayout->setDirection(QBoxLayout::LeftToRight);

                if (isOuter)
                {
                    boxLayout->setContentsMargins(
                        0, 
                        0, 
                        TextMargins().right, 
                        0);
                    boxLayout->setSpacing(ImageMargins().right + TextMargins().left);
                }
                else
                {
                    boxLayout->setContentsMargins(
                        ImageMargins().left, 
                        0, 
                        TextMargins().right, 
                        0);
                    boxLayout->setSpacing(qMax(ImageMargins().right, TextMargins().left));
                }
            }
            else
            {
                boxLayout->setDirection(QBoxLayout::RightToLeft);

                if (isOuter)
                {
                    boxLayout->setContentsMargins(
                        TextMargins().left, 
                        0, 
                        0, 
                        0);
                    boxLayout->setSpacing(TextMargins().right + ImageMargins().left);
                }
                else
                {
                    boxLayout->setContentsMargins(
                        TextMargins().left, 
                        0, 
                        ImageMargins().right, 
                        0);
                    boxLayout->setSpacing(qMax(TextMargins().right, ImageMargins().left));
                }
            }

其中setContentsMargins的原型是:

    void setContentsMargins(int left, int top, int right, int bottom);

这段代码可以简化成:

        auto direction = isLeft ? QBoxLayout::LeftToRight : QBoxLayout::RightToLeft;
        boxLayout->setDirection(direction);

        int left   = isLeft ? isOuter ? 0: ImageMargins().left : TextMargins().left;
        int right  = isLeft ? TextMargins().right : isOuter ? 0 : ImageMargins().right;
        boxLayout->setContentsMargins(left, 0, right, 0);

        auto spacing = isOuter ? (ImageMargins().right + TextMargins().left) : qMax(ImageMargins().right, TextMargins().left);
        boxLayout->setSpacing(spacing);

4. 错误的断言

if (state == protein::ResourceState::eResource_Local_Exist) {
                auto& resolver = protein::GetIAssetLibraryManager()->GetPathResolver();
                protein::UTF8String imagePath;
                resolver.ResolveFilePath(thumb, imagePath);
                fpath = xirang2::file_path(imagePath.string());
                XR_PRE_CONDITION(!fpath.empty());
            }

这里的fpath由ResolveFilePath的输出参数设置,实际取决于磁盘上是否存在,磁盘上不存在该文件不应导致程序崩溃。如果磁盘文件按照设计理当存在,那么这里应该抛出异常,如果容忍不存在,那么应该检测并返回错误码。

另一类错误:

auto info = mMuiManager.getLibrary(record.owner().id());
XR_PRE_CONDITION(info.valid());

record上的owner就是info,却调用一次getLibrary(),多余且造成迷惑。

下面的检测属于多余:

if (!buffer_temp.empty())
    buffer.swap(buffer_temp);
XR_PRE_CONDITION(!buffer.empty());

5. 错误设计MuiManager::tagLibrary和getLibrary

void tagLibrary(const xirang2::file_path& path, const xirang2::string& tag)
{
    auto pos = libraryList.find(path);
    if (pos == libraryList.end())
        return;

    auto& info = pos->second;
    info.tag = tag;
    libraryTag.insert(std::make_pair(tag, path));
}

LibraryInfo getLibrary(const xirang2::string& tag) const
{
    auto iter = libraryTag.find(tag);
    XR_PRE_CONDITION(iter != libraryTag.end());
    return (const_cast<MuiManagerImp&>(*this)).getLibrary(iter->second);
}

该函数是MuiManager::tagLibrary的转发目标,因此设计要求等价于MuiManager::tagLibrary。

分析:

该函数的设计和实现没有对边界情况做仔细的分析(更不必说严肃仔细了)。因该要考虑的几个问题:

  1. path的定义域是什么?超出定义域的错误,应该如何处理?
  2. tag的定义域是什么?实现有哪些正确性问题?
  3. 关联函数应该有哪些?
  4. 该方法是否具有一般性?

从实现来看,path的定义域似乎是清楚的,就是那些已经被加载的library的path。如果这就是定义域,那么就不应该通过if语句加以检测,而要用前条件来断言,因为超定义域调用函数是一种逻辑错误。

这个错误反映出没有自觉地分析函数的定义域和工作能行范围。

如果限定path必须是已经加载的library,相当于说:对于给定tag,其关联library必定已经加载。getLibrary(名字不恰当,意思是getLibraryByTag)的实现表明设计者正是这么想的。然而,这一点却是没有保证的。如果要保证这一点,就必须在卸载library的调用中加以处理,比如从tag表中删除对应的条目。当然,保证方式可能有多种,比如阻止卸载也是可能的,但是不同的方案需要变更和维护与之关联函数的行为也是极不相同的。如果不想作出这种保证,就应该解除tag和关联library之间的强硬关联关系。

这个问题暴露出对维护同组函数逻辑自洽性的要求缺乏理解,也没有对函数之间的协调性和自洽性做过分析。

进一步,从业务逻辑角度出发,tag对library必须已经加载的要求是否合理?表面上看来,有一定的合理性,这种约束可以简化实现代码的边界条件处理,从上面的getLibrary的实现代码可窥一斑。但是实际上未必。系统简化是否合理,首先是要看这种简化对业务逻辑的实现有何影响。tag和特定library内容之间的约束建立是很早的,甚至可以早到在配置文件中说明,这就远远早于程序运行了。那么,这里library必须已经加载的前条件就是欠妥当的。不但为程序改进、拥抱变化制造障碍,也阻碍产品顺利使用这些API。这种阻碍和那种要求用户代码必须以特定方式完成某个功能的限制不是一回事。

进一步,如果对给定的library path,以不同的tag调用两次,会发生什么?这里的实现代码显然包含了矛盾。当某个tag已存在时libraryTag.insert(std::make_pair(tag, path))不会有任何动作,但是对应library中的tag却改变了。这就导致一种矛盾的状态:明明已经tag了,但是通过tag查询到的却是另一个library。而如果通过library询问其tag,多个library可能报告相同的tag,这难以说清楚的状态细节,就是产生bug的温床。

有tag方法,有get方法,那么是不是就够了?远远不够。怎么测试tag方法是正确的?怎么测试多次调用tag方法仍然是正确的?还应该注意到,tag和library必须已经加载之间的约束使得测试代价也上升了,不但测试代码变复杂,运行测试也变得困难和慢得多。

关联函数问题,涉及到增删查改,元素与集合的问题。这里有增,但是没有删查,至于改,如果是打算支持的,那么实现是错误的。缺少集合方法,导致测试困难。测试困难往往是设计缺陷的体现。因此,TD有权也有义务指出测试实现上的困难和不合理,这是帮助指出设计上的缺陷,而不是劳而无功地用复杂的代码“完成”测试用例。从tag可以查询library path,那反过来,是不是需要从library path反查tag?这也是需要考虑的。把tag放在library上不是唯一的方案,也未必是最简单的。这里看上去简单,是因为没有做数据一致性的处理。

数据一致性是程序正确性的一种,而数据的单点表达是维护数据一致性的有效手段。

library需要tag,那么其他的集合,instance group, pantone color library,有没有类似的需求?有没有必要推广到其他情形?这也是要考虑的。至少,对于instance group而言,in document组是一个特殊的,通过tag获取也有合理性。如果还推广到pantone clolor,因为是模式重复,这不会增加学习成本,也几乎不增加实现成本。但是潜在的应用空间扩大了,概念之间的模式一致性也更好了。

6. 意义不明的代码

bool MuiManagerImp::refreshLibrary(const xirang2::file_path& path) {
    XR_PRE_CONDITION(libraryList.count(path));
    auto& libImp = libraryList[path];
    xirang2::string uiname = libImp.uiname;
    return fetchAssetList_(path, xirang2::string(K_AssetType_Materialappearance));
}

libraryList[path]取回引用后,并未实际使用它。uiname也是如此。这段代码除了让人困惑,毫无实际作用。必须坚决删除掉。

7. 不小心边界处理

template <typename T, size_t N>
inline typename insertable_array<T, N>::iterator insertable_array<T, N>::
insert(iterator loc, const T &t) {
    assert(loc >= begin() && loc < begin() + N && loc <= end() && "broken insertable_array insert");
    for(iterator i = end(); i != loc; --i) { *i = *(i-1); }
    *loc = t;
    ++m_offset;
    return loc;
}

当i指向end时,*i = *(i-1)越位了,除非分配的存储至少是(N+1).而分配N+1大小的存储也有额外的问题,即这个溢出的元素何时销毁的问题.

template <typename T>
inline typename pod_vector<T>::reverse_iterator pod_vector<T>::rbegin() {
    return m_end;
}
template <typename T>
inline typename pod_vector<T>::const_reverse_iterator pod_vector<T>::rbegin() const {
    return m_end;
}

template <typename T>
inline typename pod_vector<T>::iterator pod_vector<T>::rend() {
    return m_data;
}
template <typename T>
inline typename pod_vector<T>::const_iterator pod_vector<T>::rend() const {
    return m_data;
}

*rbegin应该访问的是最后一个元素而不是end,rend对应begin前面一个元素的位置而非begin.上面的实现偏差了一个位置。这里的代码不完全,理论上,这个“差一”是可以在reverse_iterator里面修正掉的,但实际上,reverse_iterator是T*类型.

8. 不老练的代码

inline bool lru_resource_manager::
get(Hashable const &key, Data &data) {
    boost::shared_lock<SharedMutex> rlock(m_mtx);
    LRUMapIter h_it = m_map.find(key);
    if(h_it == m_map.end()) { return false; }

    //upgrade the lock to unique for write
    boost::upgrade_lock<SharedMutex> uplock(m_mtx);
    boost::upgrade_to_unique_lock<SharedMutex> wlock(uplock);

    LRUListIter l_it = h_it->second;
    DataPair const &dp = *l_it;
    h_it->second = m_list.insert(m_list.end(), dp);
    m_list.erase(l_it);
    data = dp.res;
    return true;
}

inline bool lru_resource_manager::
put(Hashable const &key, Data &data) {
    uintmax_t res = StaticResourceDelegate::get_quantity(data);
    if(res > m_maxQuantity) { return false; }

    boost::unique_lock<SharedMutex> wlock(m_mtx);
    m_totalQuantity += res;

    LRUListIter l_it;
    for(l_it = m_list.begin() ; m_totalQuantity > m_maxQuantity && l_it != m_list.end(); ++l_it) {
        m_map.erase(l_it->key);
        m_totalQuantity -= ResourceDelegate::get_quantity(l_it->res);
        m_list.erase(l_it);
    }

    DataPair dp;
    l_it = m_list.insert(m_list.end(), dp);
    l_it->key = key;
    l_it->res = data;
    std::pair<LRUMapIter, bool> ret = m_map.insert(std::make_pair(key, l_it));
    if(!ret.second) {
        //todo throw exception
    }
    return true;
}

对比

        value_type* try_fetch(const key_type& key){
            auto pos = m_index.find(key);
            if (pos == m_index.end()){
                ++m_missed;
                return nullptr;
            }

            ++m_hitted;
            m_items.splice(m_items.begin(), m_items, pos->second);
            return &pos->second->second;
        }

        void set_(K key, V value, list_type& result) {
            auto pos = m_index.find(key);
            if (pos != m_index.end()){
                m_items.splice(m_items.begin(), m_items, pos->second);
                pos->second->second = std::move(value);
            }else{
                auto pnew = m_items.emplace(m_items.begin(), std::pair<K, V>(std::move(key), std::move(value)));
                m_index[key] = pnew;
            }

            shrink_(result);
        }

        void shrink_(list_type& res){
            while (m_index.size() > m_cap){
                auto rm = --m_items.end();
                m_index.erase(rm->first);
                res.splice(res.end(), m_items, rm);
            }
        }

另外,锁的使用方式也是有问题的.这里加锁后的操作是非常快速的,因此最廉价的互斥锁,甚至是spin lock即足够,采用可升级的读写锁相对代价高昂,是不明智的。

9. 形式一致性是重要的

            if (type == CollectionType::definition) {
                auto coll = this->manager().getLibrary(xirang2::file_path(collid.c_str(), xirang2::pp_convert_backslash));
                if (!coll.valid())  return;

                auto record = coll.getItem(assetid);
                if (!record.valid()) return;
                dest.addOrReplace(record.definition(), assetid);
            }
            else if (type == CollectionType::instance) {
                auto coll = this->manager().getInstanceGroup(xirang2::file_path(collid.c_str(), xirang2::pp_convert_backslash));
                if (!coll.valid())  return;

                auto record = coll.get(assetid);
                if (!record.valid()) return;
                dest.addOrReplace(record.instance(), assetid);
            }

上面的两段代码形式是极其相似的,逻辑过程则完全一致.但是无法抽取成公共的函数,抽取成模版也不合适.鉴于代码本身并不长,如果将差异部分做适配,统一调用名称,适配部分的代码在整个代码中的比重也不低.试做部分如下:

struct tag_library{};
struct tag_instgroup{};

LibraryInfo getCollection(MuiManager& mgr, xirang2::file_path& collid, tag_library){
    return mgr.getLibrary(collid);
}

InstanceGroup getCollection(MuiManager& mgr, xirang2::file_path& collid, tag_instgroup){
    return mgr.getInstanceGroup(collid);
}

为getItem/get的适配代码略.这些适配代码甚至有超过简单重复代码量的趋势.这些代码如果能够被复用,才有价值,如果只是局部可见的,只是徒增复杂性。因此,在设计MuiManager时应该充分考虑到形式上的一致性,形式上一致的代码才便于被泛化调用。

10. 避免不安全的代码

//get the token form config.txt for testing
static xirang2::string GetOAuth2Token()
{
    char* path = "c:/config1.txt";  // should be : const char* path = ...
    std::ifstream myfile(path);

    char buffer[512];  // 避免长度限定的buffer, 这里可以用std::string
    xirang2::string stoken;

    while (!myfile.eof())
    {
        myfile.getline(buffer, 512);  // 可以用: std::getline(myfile, buffer)
        std::string sbuf(buffer);
        auto pos = sbuf.find(">");
        if (pos != -1)
        {
            stoken = sbuf.substr(pos + 1, sbuf.length() - pos);
            //std::cout << "token is " << stoken << std::endl;
        }
    }

    return stoken;
}

这段代码虽然是为一个不严谨的临时性目的,但是也过于啰嗦,应该修改为:

static xirang2::string GetOAuth2Token(){
    std::ifstream myfile("c:/config1.txt");
    std::string token;
    myfile >> token; // 或: std::getline(myfile, token); 取决于token是否会包含空白字符
    return toke;
}

退一步,即使'>'是不可避免的,代码亦可以简化:

static xirang2::string GetOAuth2Token(){
    std::ifstream myfile("c:/config1.txt");
    std::string line;
    while(std::getline(myfile, line)){
        auto pos = std::find(line.begin(), line.end(), '>');
        if (pos != line.end()) return std::string(++pos, line.end());
    }
    return xirang2::string();
}

About

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


Navigation

Main Page


Valid CSS | Valid XHTML 1.0