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

FINERACT-2081: introduce factory pattern to Note entity instantiation. #4020

Closed

Conversation

Zeyad2003
Copy link
Contributor

  • Removed all redundant static instantiation methods and constructors from Note.java.
  • Implemented NoteFactory class in the domain package.
  • Used Builder pattern to make it easy to instantiate the Note object.
  • Changed all places to use the new Factor implementation.

Description

Describe the changes made and why they were made.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per https://github.com/apache/fineract/#pull-requests

  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.

  • Create/update unit or integration tests for verifying the changes made.

  • Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.

  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes

  • Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

import org.apache.fineract.portfolio.savings.domain.SavingsAccountTransaction;
import org.apache.fineract.portfolio.shareaccounts.domain.ShareAccount;

public final class NoteFactory {
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @Zeyad2003, I came across your PR today. I would suggest either using the builder, or using the factory, but putting a factory layer facading a builder layer seems to be an overkill to me.

Please give it a try to refactor going with either of the approaches and see if the code gets simpler and more straightforward.. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kjozsa, Thanks for your feedback.

I understand your point, and I'd agree but in different scenarios that don't involve JPA entities.

Using public @Builder may cause some violations like bypassing some validation checks, or improperly updating fields that should remain immutable after instantiation.

That's why I limited the access to AccessLevel.PACKAGE

Relying solely on Factory would push us towards less flexible solutions like:

  • Gigantic constructor with +30 parameter or multiple parameterized constructors definition (not flexible).

  • Using some static methods that have their own problems but also violate the Single Responsibility Principle (SRP) and at the end of the day we might face the same constructor problem as well.

You can review the earlier discussion on using @Builder with JPA entities here.

Please let me know your thoughts.

Thank you!
Zeyad

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I agree with your reasons, but in that case, I would enforce everyone to use the dedicated Factory for Note instantiation, so we'd need to get rid of the public constructors.

I would also consider moving the Note#update(JsonCommand) function's functionality away from the entity, as this makes a direct dependency between the entity layer and the web layer..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
It has been replaced by a private update() method within the NoteWritePlatformService.

@Zeyad2003 Zeyad2003 requested a review from kjozsa August 12, 2024 09:48
@adamsaghy adamsaghy requested review from vidakovic and removed request for kjozsa August 14, 2024 08:58
@@ -37,23 +42,27 @@
import org.apache.fineract.portfolio.shareaccounts.domain.ShareAccount;

@Entity
@Getter
@NoArgsConstructor
@AllArgsConstructor
Copy link
Contributor

Choose a reason for hiding this comment

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

AllArgsConstructor does not make sense for this entity, because from the several entity-type references only one (or in some special cases maybe more but never all) can be set.
The Builder will generate a private AllArgsConstructor anyway. Could you please remove this annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the @Builder annotation requires a proper constructor for initializing the entity, so I've limited the access to private instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

If a class is annotated with @builder, then a package-private constructor is generated with all fields as arguments (as if @AllArgsConstructor(access = AccessLevel.PACKAGE) is present on the class), and it is as if this constructor has been annotated with @builder instead. Note that this constructor is only generated if you haven't written any constructors and also haven't added any explicit @XArgsConstructor annotations. In those cases, lombok will assume an all-args constructor is present and generate code that uses it; this means you'd get a compiler error if this constructor is not present.
So yes, you need to add the @AllArgsConstructor, just because @NoArgsConstructor is there. Private is fine.

@adamsaghy adamsaghy requested a review from kjozsa August 15, 2024 12:03
@@ -63,107 +72,17 @@ public class Note extends AbstractAuditableWithUTCDateTimeCustom<Long> {
private Integer noteTypeId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the enum itself in the entity and map it as @Enumerated(ORDINAL)? Wouldn't it achieve the same results, but allow to use the enum from the Java code side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it would be better, and also I would recommend using EnumType.STRING instead to avoid any conflict if we change the order or something.

But I think it's out of the scope of this PR for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we cant change this right now, because this Integer is not the ordinal of the enum, but a custom number associated with each enum type. This topic would worth a discussion and then a refactor of the more hundred enum-like properties not using real enum types, but this is really not the scope of this PR.

@@ -48,6 +50,8 @@
@Slf4j
public class NoteWritePlatformServiceJpaRepositoryImpl implements NoteWritePlatformService {

private static final String NOTE = "note";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very much against this, in such trivial cases. Especially if you are writing "note" just in the same file at line 84.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. It has been removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've found several (> 10) static (or not static :) ) "note" parameter definition, everywhere in Fineract. Something like this:
public static final String NOTE_PARAM = "note";
If you refactor the notes anyway, it would be nice to add this to a central place only once and everywhere (all the account types) could use this.
Do you think it fits this PR?

Copy link
Contributor Author

@Zeyad2003 Zeyad2003 Aug 21, 2024

Choose a reason for hiding this comment

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

Actually, the definitions of "note" appear in many places across the codebase. You can use the following command to locate them:

grep -rnw --include="*.java" '.' -e '[A-Za-z_][A-Za-z0-9_]*\s*=\s*"note"'

However, addressing this is something that can be tackled later. For now, I need to focus on other tasks related to implementing a type-safe API.

@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2081/enhance_note_entity branch from eeddc2c to 6d34e49 Compare August 21, 2024 14:39
@kjozsa
Copy link
Contributor

kjozsa commented Aug 21, 2024

LGTM

@vidakovic
Copy link
Contributor

Why do we need a "NoteFactory" in the first place? This is just hiding the builder pattern for instantiating "Note" classes behind these static functions with endless parameters that are really not a good developer experience (especially when reading/reviewing code). I see no improvement over the original implementation... why not just use the builder pattern when instantiating the Note? If we just use the builders then everything is just perfectly clear to the reader and we don't have to carry around yet more boilerplate code... my 2 cents.

@Zeyad2003
Copy link
Contributor Author

@vidakovic

Please refer to this reply for an explanation of why I chose to use the Factory pattern as a facade for the Builder pattern.

We also had an earlier discussion here about using @Builder with JPA entities, where @marta-jankovics shared valuable insights on the appropriate access level for employing the builder approach.

Looking forward to your thoughts.

Best regards,
Zeyad

@marta-jankovics
Copy link
Contributor

LGTM
Added some suggestions that would be nice to implement, but not blocking the PR to be merged.

@vidakovic
Copy link
Contributor

We are mixing unrelated things... JPA and entity classes are a whole different thing... but there too I would qualify the static functions as an anti pattern....

@Zeyad2003
Copy link
Contributor Author

@vidakovic

I understand your point, but I believe relying solely on the builder pattern can introduce challenges.

For example, consider this method:

public static Note createLoanTransactionNote(Loan loan, LoanTransaction loanTransaction, String note) {
    return Note.builder()
            .loan(loan)
            .client(loan.client())
            .loanTransaction(loanTransaction)
            .noteTypeId(NoteType.LOAN_TRANSACTION.getValue())
            .note(note)
            .build();
}

This method is used in 14 places. Without a factory, each instance would need to know the full structure of the Note entity and the necessary attributes to set, (e.g. when creating LoanTransactionNote we should set client attribute as well).

Additionally, a factory method allows us to encapsulate extra logic, such as fetching required data, applying business rules, or performing validation ...etc
BTW, I understand your concerns about validation at the domain level, and your suggestion to enforce it at the database schema. However, as @marta-jankovics pointed out earlier "it's too late, and the error message is too ugly".

Regarding static methods, perhaps we could establish guidelines to ensure they’re only created when necessary and in an acceptable form.

I'd like to know your thoughts on how we might balance these concerns while keeping the codebase as maintainable and clear as possible.

Best regards!

@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2081/enhance_note_entity branch from 6d34e49 to ca9a20c Compare August 22, 2024 01:58
- Removed all redundant static instantiation methods and constructors from Note.java.
- Implemented `NoteFactory` class in the domain package.
- Used Builder pattern to make it easy to instantiate the Note object.
- Changed all places to use the new Factor implementation.
@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2081/enhance_note_entity branch from ca9a20c to 868be50 Compare August 22, 2024 02:06
@vidakovic
Copy link
Contributor

I don't agree. And I can show why, but this here is not the main task. So, let's move on with the write requests, make them work and document everything for the gsoc project.

This here doesn't contribute and is a distraction.

@kjozsa
Copy link
Contributor

kjozsa commented Aug 22, 2024

Why do we need a "NoteFactory" in the first place? This is just hiding the builder pattern for instantiating "Note" classes behind these static functions with endless parameters that are really not a good developer experience (especially when reading/reviewing code). I see no improvement over the original implementation... why not just use the builder pattern when instantiating the Note? If we just use the builders then everything is just perfectly clear to the reader and we don't have to carry around yet more boilerplate code... my 2 cents.

That was my very first thought as well 🙂 The slight adventage with having both might be that the client doesn't need to know the mandatory fields for each "note type", instead the factory forces it to do the right thing...

I'm still not sold it's completely worth the additional LOC though.

@vidakovic
Copy link
Contributor

Let's close this one for now. I don't think the factory pattern introduces any improvements here. And Zeyad is occupied finishing his GSoC project anyway... which has higher priority.

@vidakovic vidakovic closed this Sep 5, 2024
@Zeyad2003
Copy link
Contributor Author

I've opened another PR (#4071) for using builder pattern only and removed all other static instantiation methods.
If you have time, please take a look.

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.

4 participants