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 lombok dependency #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nirvana124
Copy link

Added lombok dependency to reduce verbosity of code.

Copy link

@mvoronov mvoronov left a comment

Choose a reason for hiding this comment

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

Hi @nirvana124 !
Nice improvement! I agree that adding lombok will decrease boilerplate code.
But, I believe you are messed one thing here:

  1. MatchInput its just a POJO, it shouldn't know anything about conversion one entity to another
  2. You are misusing @builder(toBuilder) annotation in Team.java
    Best regards,
    Maks

@nirvana124
Copy link
Author

Hi @nirvana124 !
Nice improvement! I agree that adding lombok will decrease boilerplate code.
But, I believe you are messed one thing here:

  1. MatchInput its just a POJO, it shouldn't know anything about conversion one entity to another
  2. You are misusing @builder(toBuilder) annotation in Team.java
    Best regards,
    Maks

@mvoronov Thanks for review.

If we are adding any method about conversion in POJO we can consider it as a class and not a POJO :)
It increases the code readability but i think in this project we already have MatchDataProcessor for conversion.

Can you please elaborate on point 2 ?

@mvoronov
Copy link

Hi @nirvana124 !
Nice improvement! I agree that adding lombok will decrease boilerplate code.
But, I believe you are messed one thing here:

  1. MatchInput its just a POJO, it shouldn't know anything about conversion one entity to another
  2. You are misusing @builder(toBuilder) annotation in Team.java
    Best regards,
    Maks

@mvoronov Thanks for review.

If we are adding any method about conversion in POJO we can consider it as a class and not a POJO :)
It increases the code readability but i think in this project we already have MatchDataProcessor for conversion.

Can you please elaborate on point 2 ?

Hi @nirvana124 !
I mean why don't simply do like it was originally? Could you explain what is the benefit of using toBuilder in this piece of code?

Team team = this.teamRepository.findByTeamName(teamName);
return team.toBuilder().matches(matchRepository.findLatestMatchesbyTeam(teamName,4)).build();

@anshulbharadwaj
Copy link

Hi @nirvana124 !
I agree with @mvoronov point here. For Team class creating a builder pattern is not adding any values. Normally if a constructor of a class becomes more complex (wherein it accepts more than 7 params), that is one of the scenario beneficial for Builder Pattern.

@mvoronov
Copy link

Hi @nirvana124 !
I agree with @mvoronov point here. For Team class creating a builder pattern is not adding any values. Normally if a constructor of a class becomes more complex (wherein it accepts more than 7 params), that is one of the scenario beneficial for Builder Pattern.

Well, it also might be a true, but my point mainly was about "toBuilder" option.

@nirvana124
Copy link
Author

Thanks for your feedback :)
Below are the reasons why I have used builder and toBuilder
Builder: It makes code more readable, It removes verbosity. You can see in one single line for what parameters you are setting the values.

toBuilder: You can see there is a team object which is returned by repository.
I used toBuilder on team object which is creating a new object and not changing existing team object.
Team team = this.teamRepository.findByTeamName(teamName); return team.toBuilder().matches(matchRepository.findLatestMatchesbyTeam(teamName,4)).build();


@Entity
@Builder(toBuilder = true)
@Data
Copy link

Choose a reason for hiding this comment

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

Why do we need @DaTa, isn't @getter enough

Choose a reason for hiding this comment

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

@rahilsh, @DaTa is a superset of annotations in Lombok that encompasess all your getter, setter, tostring, constructor etc., They help you reduce the verbosity even in the metadata for Annotations!

Copy link

Choose a reason for hiding this comment

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

@itsraghz I meant @DaTa adds other things like setters,tostring which we don't need here. We just need @getter

Copy link

Choose a reason for hiding this comment

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

Yeah, that is right @rahilsh!

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.

5 participants