是什么让这段代码变得糟糕

Question

在我的公司，我遇到了以下 2 个代码片段，乍一看我觉得非常不愉快，但本着向编写这些代码的工程师提供建设性反馈的精神，我正在尝试提出技术论证为什么这段代码不好：

FileTableEntry * FilerManager::GetFileTableEntry(uint32_t i) const {
  return &(GetFileTable()[i]);
}

…
for (uint32_t i = 0; i < container_.size(); ++i) {
   *GetFileTableEntry(i) = FileTableEntry();
   // GetFileTableEntry ultimately accesses a std::vector in FileManager
}

我的主要论点是：

此代码非常间接且具有误导性，不应使用 getter 初始化（FileManager 的一部分），但至少有一个 setter：更直接让代码更容易理解。
getter 完全泄露了 FileManager 的内部状态，所以此时对 FileManager 的封装没有任何意义。更糟糕的是，getter 承诺适用于 const 对象，但却被愉快地用于改变 FileManager 的内部状态。打破封装是使重构更难的必由之路。

是否还有其他我会遗漏的关于不编写这样的代码的论据？？

Answer 1

反对此代码的另一个论据是 GetFileTableEntry returns 的签名是在对象始终存在的情况下的指针。这需要引用，而不是指针：

FileTableEntry& FilerManager::GetFileTableEntry(uint32_t i) const {
    return GetFileTable()[i];
}

这应该可以解决您的第一点。您的第二点可以通过引用 const:

来解决

const FileTableEntry& FilerManager::GetFileTableEntry(uint32_t i) const {
    return GetFileTable()[i];
}

这将禁止调用者修改 GetFileTableEntry 返回的内部状态。

注意：为了使 GetFileTableEntry 函数有用，应该添加对传入索引的边界检查以尽早发现错误。

是什么让这段代码变得糟糕

What really makes this code bad

c++

design-patterns

class

c++11