-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ref#000 Data definitions #121
base: main
Are you sure you want to change the base?
Conversation
Create example use of interfaces
Create someFunction Intorudce Ideas collector
Define expectation from Room to Warehouse
1. Filled enumeration clases 2. Filled Interfaces. 3. Filled Constructors and class members
Remove ctors from structs
Create a GetGender function determining Gender based on PESEL
{ | ||
public: | ||
virtual ~iDoctor() = 0; | ||
virtual PersonalData& GetPersonalData() = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should stick to our convention of naming functions starting from the lowercase letter and All types names starting from uppercase.
This creates some problems for naming interfaces. I don't know what to do with iDoctorForExample. Maybe BaseDoctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my proposition, more information here (hope link works as intened):
https://github.com/coders-school/desktop-business-app/pull/121/files#diff-aee7177b08a803cf66c7e5c35b817aea2232da4c6c81cfda2160f7b9a02e0249
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am used to 'i' prefix for the interfaces. We could go with something different.
I think it will be missinterpreted. If we will have a collection of BaseDoctors someone might want to create another collection for specialized type of doctors.
PersonalData& GetPersonalData() override | ||
{ | ||
return personal_data_; | ||
} | ||
Specialization GetSpecialization() const override | ||
{ | ||
return specialization_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as long as getPersonalData() and getSpecialization() are simple getters, I think it is best to keep their implementation in base class. They might be virtual or not. Even if it makes us resing from abstractness of base class. Until we want special implementation in some derived class it is maybe better to keep them non virtual even.
I think that Doctor should then implement funtions from other derived interfaces (if they cannot be implemented in base classes as well) or it's specific functions.
As long as no other interfaces are inherited or specific implementation appears, Doctor might be itself a base or final class.
modules/backend/clinic/clinic.hpp
Outdated
|
||
class Clinic | ||
{ | ||
std::unique_ptr<iStaff> staff_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I wonder if having iStaff interface makes sense.
If Staff will be a class containing collections of doctors, receptionists etc., and we don't plan to pass it anywhere else and use pointer to iStaff, it would be better to keep it as a concrete class.
This leads me to conclusion that maybe we actually don't need interfaces to every class, only those passed around in many places and in different context. -
I think it would be better to store Staff as oridinary member than.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we can provide different types of staff for the customer.
Dentist clinic will have staff which is present now (doc, receptionist, manager)
Other clinic might want to add or use different staff. In such case we will simply inherit from iStaff and provide class with desired objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, makes sense. Provided that we can extract common and universal interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working on it and I rethink my idea. By now it is not necessary to have staff for different customers then we should not provide it this way. Removed an iFace
* Create Specialization tests Update namespaces * Add Allergen tests * Introduce treatment tests * Create treatementstate tests * Update names and types * Update EoF
* Small CMakeLists.txt improvements and fixes * Addec cmake-policy documentation * Added docs: building and running tests instructions * Attempt to improve md files readability * Second readability improvement * resolving remarks * Added options for memory checking in tests. I found out our script for valgrind does not do what I thought. This is part of correction. * corrected script for running memory checks on tests. It will now print general information and redirect for details to ctest logfiles. It will also return 1 if errors detected, so the script has chance to be utilised on CI * updated cmake policy documentation * Updated docs: building_and_running_tests.md
Update forward declaration type.
* Remove serde interface and add protos * Add readme with protobuf guidelines * Add protobuf generation Add protobuf data files * Remove commented code * Adjust proto files Remove Serialization interface * Add serialization functions for Staff * Add serializer functions for Staff * Remove comments * Add file manager and staff deserialization * Update CMakeLists for file manager * Change serialization method, introduce save data
Thanks @BoskiJanusz for Code Review. |
This pull request redefines data definitions.
General approach will more or less follow below diagram.