What Really Makes This Code Bad
At my company, I came across the following 2 pieces of code that I found at first glance completely unpleasant, but in the spirit of offering constructive feedback from the engineer who wrote this, I am trying to find technical arguments why this code is bad:
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
}
My main arguments:
- This code is very indirect and misleading, it shouldn't use getter to initiliaze (part of FileManager), but at least setter: more direct code makes the code clearer.
- The getter completely loses the internal state of the FileManager, so encapsulation means nothing to the FileManager at this point. Worse, getter promises is applicable to const objects, but it is simply used to change the internal state of the FileManager. Breaking encapsulation is a surefire path to more efficient refactoring.
Are there any other arguments for not writing code like this that I would not notice?
source to share
Another argument against this code is that the signature GetFileTableEntry
returns a pointer in situations where the object is always present. This calls for a link, not a pointer:
FileTableEntry& FilerManager::GetFileTableEntry(uint32_t i) const {
return GetFileTable()[i];
}
This should touch your first point. Your second point can be solved by making a link const
:
const FileTableEntry& FilerManager::GetFileTableEntry(uint32_t i) const {
return GetFileTable()[i];
}
This prevents callers from modifying the internal state returned GetFileTableEntry
.
Note. To make this feature GetFileTableEntry
useful, you should add indexed index bounds checking for early error detection.
source to share