I just happened to be looking at /src/Base/Builder3D.h and its mates and started to play. Nothing here requires a detailed understanding of the intricacies of what the code does, beyond refactoring it to do the same.
Note: There is nothing here that has not been thoroughly documented in every programming textbook for the last 20+ years. The principles don't apply to just C++.
Many clang-tidy warnings. Code smell: Unresolved issues. Might be hidden issues. Looks like old C programmers have been here.
Often no curly braces. Code smell: Apple goto fail (2014?) freaked out many.
Many format inconsistencies. Code smell: Difficult to maintain.
There are several classes. We select Builder3D and InventorBuilder. There is much shared code. Code smell: Classes are very similar.
Both classes make items, generate stuff and send to a designated target.
Both classes have a lot of methods. Code smell: Maybe too many responsibilities.
Both classes share a number (but not all) item types. Code smell: This class concerning itself with the intricacies of generating these items and bloating itself with a bunch of very specific methods to do so? If you want to change items the class must be refactored. Bad.
Every item becomes a string and appended to the output. And that pretty much defines the class. The class should care not what exactly it is sending.
Looking at the methods there is an immediate code smell: Too many parameters. Either the method is too complex or it has too many responsibilities. e.g.:
Code: Select all
void addText(float pos_x, float pos_y , float pos_z,const char * text, float color_r=1.0,float color_g=1.0,float color_b=1.0)Looking at this example we see that the first three are position, the next is the item content, and then follows three more for color, sometimes four.
Code: Select all
void addText(Position position, std::string text, Color color)Position and color are used often (the long way) so would benefit immeasureably from being defined as such. They become simple to define and pass around.
Color is immediately recognisable, whereas a list of strings or numbers is indecipherable (could be anything, never mind what they are called).
When a color or position is passed in this long horrible way, who's to say it is valid? You get the idea.
The attached file is where I got to. Remember I am still coming up to speed with C++ so there are likely major fundamental mistakes. It is untested. It is complete only so far as to show some ideas.
Firstly we make a base class and subclass off it.
We establish a color class, RGB and RGBA. With these focused, the content can be checked and passed around easily.
Ditto position.
In each case with sufficient constructors to provide minimum flexibility.
Make a couple of interfaces to ensure classes adhere to requirements.
To add a new item type is as simple as defining a class for it and providing the two versions of output.
Make a class for each item. Include two methods: one for each class.
The code is now simpler, cleaner and easier to read AND TEST ( a bit of a novel concept around here - and far too difficult with the current structure). Taking it one small step at a time yields results.
Further refactoring would likely yield many more improvements.
So please don't shoot the messenger.
Suggested reading:
C++ Coding Standard: Sutter & Alexandrescu
Refactoring: Fowler
A Philosophy of Software Design: Ousterhout
Effective Modern C++: Meyers
Effective C++ 3rd Edition: Meyers
Modern C++ Design: Stroustrup
Object Oriented Software Construction: Meyer (Many really old ideas. Beware)
Your Code as a Crime Scene: Tornhill
Large Scale C++ Vol 1: Lakos
The C++ Programming Language: Stroustrup
Working Effectively with Legacy Code: Feathers
Test-Driven Development: Beck
Design Patterns: Gamma,Helm, Johnson, Vlissides
