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
847 #1517
847 #1517
Conversation
public class App { | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(App.class); | ||
|
||
@Setter |
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.
You can use @Data
instead of adding getter and setter to each field
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.
Yes, I can, but it seemed to me that the annotation @Data
is not quite suitable for the class App
. Usually, the classes called App
are needed to run an application. When I look at this class, I get the feeling that there is another class called Kingdom
. I propose to move the fields king
, castle
, army
and static class FactoryMaker
from class App
to a new class Kingdom
(where I can use the annotation @Data
) and in App
to use an object of class Kingdom
. This option seems cleaner to me. What do you think about it?
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.
Yeah you can do that as well
abstract-factory/src/main/java/com/iluwatar/abstractfactory/App.java
Outdated
Show resolved
Hide resolved
abstract-factory/src/main/java/com/iluwatar/abstractfactory/Kingdom.java
Outdated
Show resolved
Hide resolved
abstract-factory/src/test/java/com/iluwatar/abstractfactory/AbstractFactoryTest.java
Outdated
Show resolved
Hide resolved
Please let me know when its ready for another review |
I made the request for review via github, a request did not reach you? |
Need to be reviewed |
Do you want till #1503 gets completed else the code will have boilerplate code for getters and setters |
I removed lombok from the commit 2e36a11. |
private King king; | ||
private Castle castle; | ||
private Army army; | ||
private final Kingdom kingdom = new Kingdom(); |
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.
You can refactor it to
Supplier<Kingdom> kingdomSupplier = Kingdom::new
...
public Kingdom getKingdom() {
return kingdomSupplier.get();
}
but its upto you
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 don't quite understand why. Can you explain it in more detail?
Many thanks for this improvement! @all-contributors please add @vdlald for code |
I've put up a pull request to add @vdlald! 🎉 |
Thanks! |
Relates to #847
Pull request description