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

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
}

我的主要论点是:

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

是否还有我会遗漏的不写这样的代码的其他论点？

Answer 1

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

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

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

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

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

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