Ovary module modification and review #30
Conversation
jilei-hao
left a comment
jilei-hao
left a comment
There was a problem hiding this comment.
@RohanN15 Great work! It's a monumental task to modularizing a complicated logic like the ovary generator, but you did well. The class is well structured, and I like your effort trying to clean up the main function, which makes it looks tidy and elegant.
| add_library(OvaryModelGenerator OvaryModelGenerator.cxx) | ||
| add_executable(Main Main.cxx PelvisModelGenerator.cxx) | ||
| target_link_libraries(Main PRIVATE ${VTK_LIBRARIES} OvaryModelGenerator) | ||
| add_executable(ModelGenerator Main.cxx PelvisModelGenerator.cxx) |
There was a problem hiding this comment.
Main is normally not used for binary name. Because user calling it should know some thing more specific, although brief, about what this binary is about. Changing to ModelGenerator here to reflect our planning to adding more types of models into this program.
| delete pvsGen; | ||
|
|
||
| OvaryModelGenerator myOvary(nslices, nrot, d, h, w, outdirstr); | ||
| OvaryModelGenerator myOvary(nslices, nrot, d, h, w); |
There was a problem hiding this comment.
A lot of changes in this file related to the changes in the OvaryModelGenerator source and header files. Go to those files for specific comments.
| OvaryModelGenerator myOvary(nslices, nrot, d, h, w, outdirstr); | ||
| OvaryModelGenerator myOvary(nslices, nrot, d, h, w); | ||
| myOvary.Generate(); | ||
| auto ovarySections = myOvary.GetOutput(); |
There was a problem hiding this comment.
Moved scene export file to main. In future changes, it should also be modularized into some sort of exporter classes. For detailed reasons for this change, please go to the generator class for specific comments.
| */ | ||
|
|
||
| OvaryModelGenerator::OvaryModelGenerator(int slice, int rot, double de, double he, double wi, std::string outdir) | ||
| OvaryModelGenerator::OvaryModelGenerator(int slice, int rot, double de, double he, double wi) |
There was a problem hiding this comment.
Go to the header file to see my comment for this change.
| this->SetOutDir(outdir); | ||
| } | ||
| OvaryModelGenerator::~OvaryModelGenerator(){}; | ||
| OvaryModelGenerator::OvaryModelGenerator(const OvaryModelGenerator &other) |
There was a problem hiding this comment.
Go to the header file for my comment about copy constructor deletion.
| void GetOutput(std::vector<vtkSmartPointer<vtkPolyData>> vec, int imax); | ||
| std::vector<vtkSmartPointer<vtkPolyData>> &GetOutput(); | ||
|
|
||
| private: |
There was a problem hiding this comment.
Internal data should be private. If public access is needed, add Getters and Setters for indirect access.
| void SetDimensions(double de, double he, double wi); | ||
| void Generate(); | ||
| void GetOutput(std::vector<vtkSmartPointer<vtkPolyData>> vec, int imax); | ||
| std::vector<vtkSmartPointer<vtkPolyData>> &GetOutput(); |
There was a problem hiding this comment.
This is a design change. To follow the "Single Responsibility Principle" in SOLID principles, we don't want a class to more things than they should. Here, generating a model is one thing, exporting them to files is another, and they should be designed into separate modules. This allows a better cohesion within the module and decoupled it from the exporting logic.
In this change, instead of passing a vector to getouput for exporting, we simply just save it as internal data. And GetOutput() now just returns a reference to this data.
| OvaryModelGenerator(int slice, int rot, double de, double he, double wi); | ||
| ~OvaryModelGenerator(); | ||
| OvaryModelGenerator(const OvaryModelGenerator &other); | ||
| OvaryModelGenerator(const OvaryModelGenerator &other) = delete; |
There was a problem hiding this comment.
It's good practice to always explicitly define copy and assignment constructors. But it's not always necessary to enable them. We can also define them as "deleted". A general safe practice is always delete them unless they are absolutely necessary. Here, for a filter like generator class, we don't need them to be copied or assigned. So we delete them for now.
No description provided.