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

added all entities as per issues and design specification #8

Merged
merged 2 commits into from Oct 30, 2023

Conversation

awesominat
Copy link
Owner

Implemented Buy/Sell/TopupTransaction, CommonUser, CommonUserFactory, CompanyInformation, CompanyNews, Pricepoint, Stock, Transaction, TransactionHistory, TransactionType, User, and UserFactory entities.

As per the issue, TopupTransaction was added, implementation of certain entities shifted to fit UseCases. For example, User was switched to have a Portfolio attribute to more closely match the view portfolio use case.

…/Sell/TopupTransaction, CommonUser, CommonUserFactory, CompanyInformation, CompanyNews, Pricepoint, Stock, Transaction, TransactionHistory, TransactionType, User, and UserFactory entities
@awesominat awesominat linked an issue Oct 30, 2023 that may be closed by this pull request
Copy link
Collaborator

@rickygrosvenor-pramanick rickygrosvenor-pramanick left a comment

Choose a reason for hiding this comment

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

These are very fundamental building blocks for our project as these are the high-level classes on which the rest of dependencies would point inwards towards. With that in mind, I believe that you have implemented the entities in a suitable manner such that we are able to expand (add other methods if needed in the projects) from this, if needed. This is a good foundation to ensure Clean Architecture and SOLID principle adherence, especially as you have maintained the practice of ensuring that there is no logic within the entities.

I think that these entities are sufficient for our proposed use cases however, I do wonder if we could include TransactionType as an attribute within the Transaction class? I was just wondering why you implemented it this way.

@quiz3
Copy link
Collaborator

quiz3 commented Oct 30, 2023

This is a good addition. The entities are necessary for the upcoming implementation of the use_cases. There isn't much to say, since we are just restarting the project on a new repo, but for what it's worth, I have already reviewed the entities here on the old repo (which are the same), and fixed issues on the previous versions.

Copy link
Collaborator

@quiz3 quiz3 left a comment

Choose a reason for hiding this comment

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

These entities are solid and well-formed. Having reviewed and edited some of the on the old repo, I can say with confidence they will make an excellent addition to the new repo.

The toString() methods are a very useful feature for debugging purposes.

Copy link
Collaborator

@gursi26 gursi26 left a comment

Choose a reason for hiding this comment

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

I think that the logic should belong to the use cases. According to the clean architecture this layer should only be comprised of classes with getter and setter functions.

src/entities/CommonUser.java Outdated Show resolved Hide resolved
@awesominat awesominat merged commit c2fe4f9 into main Oct 30, 2023
@awesominat awesominat deleted the add-entities branch October 30, 2023 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the entities
4 participants