Sloppy Coding II
Introduction
In the prequel to this post, “Sloppy Coding“, I showed some rather trivial, small-scoped, examples of antiquated C++ coding style currently present in a hugely successful, widely deployed, open source code base. In this follow up post, I’ll do the same, but at the higher, class level scope.
Hey, I troll often, and this is what trollers do. 🙂
The Class Definition Under Assault
If you’re a young, advancing, C++ programmer, you may want to think about avoiding the bad design decisions that I rag about in the following class definition:
Where Are The Private Members?
After scanning through the class definition for the first time, the most blatantly obvious faux pax I noticed right off the bat was the “everything is public!” violation of the golden rule of encapsulation. If the intent was for CBlockHeader to be a user-defined type with no need to establish and preserve any invariant conditions, then it should have been declared as a less verbose struct .
I know, I know, that’s another nitpick. But there’s more to it than meets the eye. Notice the last three CBlockHeader class member functions are declared const. Since users can directly reach into a skinless CBlockHeader object to mutate its internal state at will, nice users can be unsafely stomped on by cheaters (Jihan Wu? 🙂 ) in between calls.
The cheater gets the 50, and the nice guy gets the goose egg.
Where Are The Initializers?
Notice that the CBlockHeader class constructor does not contain an initialization list – all initialization is performed in the constructor body via a call to SetNull().
An initialization explicitly states that initialization, rather than assignment, is done and can be more elegant and efficient (CppCoreGuidelines). It also prevents “use before set” errors. For a slight performance bump upon each CBlockHeader object instantiation and good C++ style, the struct implementation of the CBlockHeader constructor should look like this, no?
Nasty Pre-processor Hackros, Turtles All The Way Down
Ugh, I don’t even know what to say about the dangerous 1980’s C pre-processor macro bombs planted in the class definition: ADD_SERIAL_METHODS, NCONST_PTR (nested within ADD_SERIAL_METHODS) , and READWRITE. Compared to the design flaws pointed out previously, these abominations make the class uncomfortably uninhabitable for a large population of otherwise competent C++ programmers.
Bjarne Stroustrup is the creator of C++ and a long time hero of mine. He’s been passionately evolving the language (C With Classes, C++, C++98, C++11, C++14, C++17) and a leading force in the community since the 80s. Mr. Stroustrup has this to say about Hacros:
A safer, more maintainable, alternative to the current Hack-Horiffic design would be to wrap the required inline functionality in class templates and have CBlockHeader publicly extend those classes. The details are left to the aspiring student.
Fini
Ok, ok. EVERYONE on social media knows that trolls beget trolls. So, troll away at this post. Rip me to shreds. I deserve payback.
I agree with everything you wrote in Sloppy Code I and II, therefore I can’t really troll at you back. I don’t know who coded this, but… it’s just plain sad.
Thanks! I actually asked the coder why the data members were public and he said: “because I use them outside of the class”. Yikes!
I would say that if oyu have no invariant to protec, having public members instead of horde of set/get is maybe better.
I agree. Don’t waste time typing in getters and setters that can be bypassed and making the interface wider than necessary. The GetHash() function is the only useful function member.