是什么让这段代码变坏了？

Question

在我的公司里，我偶然发现了以下两个代码片段，我一见钟情就感到非常不愉快，但本着向编写这段代码的工程师提供建设性反馈的精神，我正在试图找出为什么这段代码不好的技术论据：

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承诺适用于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函数有用，应该对传入的索引添加一个边界检查，以便尽早捕获错误。