Testing on the Toilet: Avoid Hardcoding Values for Better Libraries
Wednesday, August 19, 2020
This article was adapted from a Google Testing on the Toilet (TotT) episode. You can download a printer-friendly version of this TotT episode and post it in your office.
By Adel Saoud
You may have been in a situation where you're using a value that always remains the same, so you define a constant. This can be a good practice as it removes magic values and improves code readability. But be mindful that hardcoding values can make usability and potential refactoring significantly harder.
Consider the following function that relies on hardcoded values:
// Declared in the module.
constexpr int kThumbnailSizes[] = {480, 576, 720};
// Returns thumbnails of various sizes for the given image.
std::vector<Image> GetThumbnails(const Image& image) {
std::vector<Image> thumbnails;
for (const int size : kThumbnailSizes) {
thumbnails.push_back(ResizeImage(image, size));
}
return thumbnails;
}
|
Using hardcoded values can make your code:
- Less predictable: The caller might not expect the function to be relying on hardcoded values outside its parameters; a user of the function shouldn’t need to read the function’s code to know that. Also, it is difficult to predict the product/resource/performance implications of changing these hardcoded values.
- Less reusable: The caller is not able to call the function with different values and is stuck with the hardcoded values. If the caller doesn’t need all these sizes or needs a different size, the function has to be forked or refactored to avoid aforementioned complications with existing callers.
When designing a library, prefer to pass required values, such as through a function call or a constructor. The code above can be improved as follows:
std::vector<Image> GetThumbnails(const Image& image, absl::Span<const int> sizes) {
std::vector<Image> thumbnails;
for (const int size : sizes) {
thumbnails.push_back(ResizeImage(image, size));
}
return thumbnails;
}
|
If most of the callers use the same value for a certain parameter, make your code configurable so that this value doesn't need to be duplicated by each caller. For example, you can define a public constant that contains a commonly used value, or use default arguments in languages that support this feature (e.g. C++ or Python).
// Declared in the public header.
inline constexpr int kDefaultThumbnailSizes[] = {480, 576, 720};
// Default argument allows the function to be used without specifying a size.
std::vector<Image> GetThumbnails(const Image& image,
absl::Span<const int> sizes = kDefaultThumbnailSizes);
|
Ironically, you've omitted their value for testing :P. Default arguments also make it super easy to test typical usage, as well as all the weird edge cases.
ReplyDeleteThanks for your message! I'm not sure I understand what you mean by "omitted their value for testing", so feel free to elaborate!
DeleteYes exactly! Default arguments make it easy for the function callers to use your function, but they also allow for the use & unit testing of a wide range of values & edge cases.
He meant that this refactor enables for easy testing, compared to having hardcoded value inside the class. (this is a test blog after all:)
DeleteHi Adel, thank you for this post.
ReplyDeleteI know that this is an example, but that size value should be checked in function body. This is usual pattern in introducing security bugs in C/C++, when we allow user of our function to set size of something.
Regards, Karlo.
Hi Karlo, you're absolutely right! Validating user input is a very important part of any code, but we've omitted it in our examples above to keep the code slim & focused on our discussed topic.
DeleteWhen is it a good idea to use constant fields in a class, then?
ReplyDeleteGood question, Igor! We're actually not advocating against constants in classes, those have their use cases for sure.
DeleteFor example, the const & function in the example in red are quite OK if they're defined in your own class that is only used by your project where the logic is specific & never/rarely changes. That code becomes an issue though, for the reasons discussed in the post, if it's part of a public code library accessible to many other engineers & projects where it should cater for a wide variety of use cases & values.
I hope that clarifies things!