July 2, 2021
Developer Traps: Needless Layering
—A detailed analysis of how I code provided insight on poor practice.
I started to perform a deeper analysis of my approach to coding. It’s proven insightful.
My approach to studying how I code was motivated by a talk by Kent Beck on Responsive Design. This talk is insightful overall, but what motivated my analysis was Kent’s discussion on how he prepared for his Smalltalk patterns book.
To develop the patterns in the book he spent time writing down what he was doing when he coded and providing explanations for why he was doing those things. Basically, he was trying to capture the thought process of an expert Smalltalk developer in a way that could be easily explained to others.
What struck me in this discussion, what that anyone could repeat this exercise and benefit. So I wrote down what I did when I coded with the objective of eliminating poor practice.
Here is an example.
I’m in the process of writing an application using Qt that relies upon an SQLite database. One of the challenges was that I wanted to create test cases that automatically create and remove database files.
Seems simple enough. My first attempt looks like this. (I’m using Qt and Google Test.)
class FileTemplateManager
{
public:
FileTemplateManager(const char * const t): file_name_template(t) {
if(!file_name_template)
throw "file template error";
}
~FileTemplateManager() {
}
const char *get_template() const {
return file_name_template;
}
private:
const char * const file_name_template;
};
const bool fileExists(const QString& path) {
QFileInfo check_file(path);
if (check_file.exists() && check_file.isFile()) {
return true;
}
return false;
}
/* Check container state following initialization using default constructor.
*/
TEST(TestDatabase, Create) {
FileTemplateManager test_file_path("/tmp/database.XXXXXX");;
QString database(test_file_path.get_template());
create(database);
ASSERT_TRUE(fileExists(database));
}
The point of this attempt was to ensure that I could create a database with a fixed path.
That database is located at /tmp/database.XXXXXX.
The format of the database path is motivated by my desire to use man 3 mktemp to generate a unique file name.
I’m planning on using mktemp because I couldn’t find a generic way to generate a random file name that I could provide SQLite to create the database.
(Qt has a function for this but the name isn’t generated until you open the file.)
The entire focus of the abstraction around temporary files is the management of the template string.
Initially, I liked this code because it worked, it was clean and the FileTemplateManager did only one thing.
The second attempt looks like this.
class FileTemplateManager
{
public:
FileTemplateManager(const char * const t): file_name_template(t) {
if(!file_name_template)
throw "file template error";
}
~FileTemplateManager() {
}
const char *get_template() const {
return file_name_template;
}
private:
const char * const file_name_template;
};
class FileManager
{
public:
FileManager(const FileTemplateManager& t): file_name_template(t) {
file_name = new char[strlen(file_name_template.get_template()) + 1];
strcpy(file_name, file_name_template.get_template());
mktemp(file_name);
}
const char * const get_name() const {
return file_name;
}
~FileManager() {
if(std::remove(file_name) != 0)
std::perror( "Error deleting file" );
delete file_name;
}
private:
FileTemplateManager file_name_template;
char *file_name;
};
const bool fileExists(const QString& path) {
QFileInfo check_file(path);
if (check_file.exists() && check_file.isFile()) {
return true;
}
return false;
}
/* Check container state following initialization using default constructor.
*/
TEST(TestDatabase, Create) {
FileManager test_file_path(FileTemplateManager("/tmp/database.XXXXXX"));
QString database(test_file_path.get_name());
create(database);
ASSERT_TRUE(fileExists(database));
}
Introduced the FileManager abstraction to manage the creation and deletion of the file using RAII.
There is some nice stuff here: FileManager is blissfully ignorant of the details of FileTemplateManager and the two play nicely together.
mktemp gets introduced and the whole thing works very nicely.
The fourth attempt looks like this.
class FileManager
{
public:
FileManager(const char * const t) {
file_name = new char[strlen(t) + 1];
strcpy(file_name, t);
if(0 == strlen(mktemp(file_name))) {
throw "cannot find a unique file name";
}
}
const char * const get_name() const {
return file_name;
}
~FileManager() {
if(std::remove(file_name) != 0)
std::perror( "Error deleting file" );
delete file_name;
}
private:
char *file_name;
};
const bool fileExists(const QString& path) {
QFileInfo check_file(path);
if (check_file.exists() && check_file.isFile()) {
return true;
}
return false;
}
/* Check container state following initialization using default constructor.
*/
TEST(TestDatabase, Create) {
FileManager test_file_path("/tmp/database.XXXXXX");
QString database(test_file_path.get_name());
create(database);
ASSERT_TRUE(fileExists(database));
}
FileTemplateManager turned out to be complete nonsense.
It’s not needed because the string it managed was corrupt or invalid only if there was programmer error.
Managing this at runtime was a waste.
FileTemplateManager is also the wrong abstraction.
The goal was a unique filename to use in the test so that I could ensure that the database file name was unique.
It did nothing to support that.
Elminating FileTemplateManager is best because the errors that mktemp returns occur when there are no ‘X’s in the file name and when it can’t create a unique file name.
The first is a programming error and the second is a runtime error.
The programming error is handled by ignoring the NULL return from mktemp whenever there are no X’s in the file name.
This will be caught when you send NULL to strlen().
The second requires a check for a zero-length (empty) string.
Whenever this occurs, FileManager cannot fulfill it’s contract so it throws an exception.
Nice.
A better path to this implementation would have used FileManager to return the provided file name in the first iteration and deal with mktemp in the second.
A closer look at the nature of the errors returned by mktemp and their implications on the context mktemp was going to be used in might have paved the way for this.
