Skip to content
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

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open

Ref#000 Data definitions #121

wants to merge 53 commits into from

Conversation

fbtom
Copy link
Contributor

@fbtom fbtom commented May 24, 2024

This pull request redefines data definitions.
General approach will more or less follow below diagram.
general_approach

  1. Introducing data types
  2. Adding logic to validate, collect and return data in data types
  3. Define and add communication between modules
  4. Adjust CMakeFiles, Docker
  5. Redefine github actions

@fbtom fbtom self-assigned this May 24, 2024
@ziobron ziobron added the Standard The user has Standard course option label May 24, 2024
fbtom added 3 commits May 25, 2024 01:06
Create example use of interfaces
Create someFunction
Intorudce Ideas collector
{
public:
virtual ~iDoctor() = 0;
virtual PersonalData& GetPersonalData() = 0;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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.

Comment on lines 14 to 20
PersonalData& GetPersonalData() override
{
return personal_data_;
}
Specialization GetSpecialization() const override
{
return specialization_;
Copy link
Contributor

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.


class Clinic
{
std::unique_ptr<iStaff> staff_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

  2. I think it would be better to store Staff as oridinary member than.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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
@ziobron ziobron closed this Jun 18, 2024
@fbtom fbtom reopened this Jun 18, 2024
* 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
@fbtom fbtom removed the Auto-closed label Jun 18, 2024
@ziobron ziobron closed this Jun 26, 2024
@fbtom fbtom reopened this Jun 26, 2024
Update forward declaration type.
@ziobron ziobron closed this Sep 4, 2024
@fbtom fbtom changed the title Data definitions Ref#000 Data definitions Oct 1, 2024
@fbtom fbtom reopened this Oct 1, 2024
fbtom and others added 2 commits October 1, 2024 20:52
* 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
@fbtom fbtom marked this pull request as ready for review October 7, 2024 19:17
@ziobron
Copy link
Contributor

ziobron commented Oct 7, 2024

Thanks @BoskiJanusz for Code Review.
🏅 1 XP granted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-closed Standard The user has Standard course option
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arch #9 PeselValidation Arch #7 Person
4 participants