-
Notifications
You must be signed in to change notification settings - Fork 1k
Solution #1093
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
base: master
Are you sure you want to change the base?
Solution #1093
Conversation
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.
Review Summary
Thank you for your hard work! Before your PR can be approved, there are a few critical issues that need to be addressed. The main problem is that your service implementation classes (CountryServiceImpl
, ActorServiceImpl
, and MovieServiceImpl
) are directly using HibernateUtil.getSessionFactory()
instead of accepting a SessionFactory
via their constructors, as required by the checklist. Additionally, in your Main.java
, you are not passing the SessionFactory
to the service implementations when creating their instances. These issues impact the core requirements and functionality of your code. Please refactor your service classes to accept a SessionFactory
in their constructors and update Main.java
to pass the SessionFactory
accordingly. Once these changes are made, your solution should meet all the core requirements. Keep going—you’re almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review
button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor
tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
import mate.academy.hibernate.relations.service.impl.ActorServiceImpl; | ||
import mate.academy.hibernate.relations.service.impl.CountryServiceImpl; | ||
import mate.academy.hibernate.relations.service.impl.MovieServiceImpl; |
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.
Issue: The service implementation instances (CountryServiceImpl
, ActorServiceImpl
, and MovieServiceImpl
) are initialized without passing the required SessionFactory
parameter. According to the checklist, you must use the provided SessionFactory
when initializing service implementations. Please update these lines to pass sessionFactory
to the constructors.
|
||
public class ActorServiceImpl implements ActorService { | ||
private final ActorDao actorDao = new ActorDaoImpl(HibernateUtil.getSessionFactory()); |
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.
Issue: The ActorDao is initialized with HibernateUtil.getSessionFactory()
directly in the field declaration. According to the checklist, service implementations must be initialized with the provided SessionFactory
, not by directly accessing HibernateUtil
. Please refactor this class to accept a SessionFactory
in the constructor and use it to initialize ActorDaoImpl
.
|
||
public class CountryServiceImpl implements CountryService { | ||
private final CountryDao countryDao = new CountryDaoImpl(HibernateUtil.getSessionFactory()); |
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.
Issue: The CountryDao is initialized with HibernateUtil.getSessionFactory()
directly in the field declaration. According to the checklist, service implementations must be initialized with the provided SessionFactory
, not by directly accessing HibernateUtil
. Please refactor this class to accept a SessionFactory
in the constructor and use it to initialize CountryDaoImpl
.
|
||
public class MovieServiceImpl implements MovieService { | ||
private final MovieDao movieDao = new MovieDaoImpl(HibernateUtil.getSessionFactory()); |
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.
Issue: The MovieDao is initialized with HibernateUtil.getSessionFactory()
directly in the field declaration. According to the checklist, service implementations must be initialized with the provided SessionFactory
, not by directly accessing HibernateUtil
. Please refactor this class to accept a SessionFactory
in the constructor and use it to initialize MovieDaoImpl
.
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.
Review Summary
Thank you for your hard work! However, there are critical issues that need to be addressed before your PR can be approved. The service implementation classes (ActorServiceImpl
, CountryServiceImpl
, and MovieServiceImpl
) must accept both their respective DAO and a SessionFactory
in their constructors, as required by the checklist. Additionally, in Main.java
, the services should be initialized with both the DAO and the SessionFactory
. Please update your constructors and initialization logic accordingly. Once these changes are made, your code will meet the core requirements—keep going, you're almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review
button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor
tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
Country usa = new Country("USA"); | ||
CountryService countryService = null; // TODO: initialize this instance | ||
CountryService countryService = new CountryServiceImpl(countryDao); | ||
// TODO: initialize this instance | ||
countryService.add(usa); |
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.
Issue: The CountryServiceImpl
should be initialized with both the countryDao
and the sessionFactory
, not just the DAO. This does not meet the checklist requirement that service implementations must accept a SessionFactory
in their constructors.
Actor vinDiesel = new Actor("Vin Diesel"); | ||
vinDiesel.setCountry(usa); | ||
ActorService actorService = null; // TODO: initialize this instance | ||
ActorService actorService = new ActorServiceImpl(actorDao); |
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.
Issue: The ActorServiceImpl
should be initialized with both the actorDao
and the sessionFactory
, not just the DAO. Please update this to match the checklist requirement.
actorService.add(vinDiesel); | ||
|
||
var movieDao = new MovieDaoImpl(sessionFactory); | ||
Movie fastAndFurious = new Movie("Fast and Furious"); |
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.
Issue: The MovieServiceImpl
should be initialized with both the movieDao
and the sessionFactory
, not just the DAO. Please update this to match the checklist requirement.
public ActorServiceImpl(ActorDao actorDao) { | ||
this.actorDao = actorDao; | ||
} |
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.
Error: The constructor should accept both an ActorDao
and a SessionFactory
, not just the DAO. This is required by the checklist item: 'Service implementation classes must accept a SessionFactory in their constructors.' Please update the constructor accordingly.
} | ||
|
||
@Override |
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.
Error: The constructor should accept both a CountryDao
and a SessionFactory
, not just the DAO. This is required by the checklist item: 'Service implementation classes must accept a SessionFactory in their constructors.' Please update the constructor accordingly.
public MovieServiceImpl(MovieDao movieDao) { | ||
this.movieDao = movieDao; | ||
} |
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.
Error: The constructor should accept both a MovieDao
and a SessionFactory
, not just the DAO. This is required by the checklist item: 'Service implementation classes must accept a SessionFactory in their constructors.' Please update the constructor accordingly.
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.
Review Summary
Thank you for your submission! 🎉 Your code now meets the core requirements for the task, and all critical issues from previous reviews have been addressed. The constructors for ActorServiceImpl
, CountryServiceImpl
, and MovieServiceImpl
now correctly accept both their respective DAO and the SessionFactory
, and the services are properly initialized in Main.java
. All model and DAO classes are implemented as required, and exception handling is in place.
However, please note that there were several critical issues initially—specifically, the service implementation constructors and their initialization in Main.java
did not follow the checklist (they only accepted the SessionFactory
and created new DAO instances internally). This is not a best practice and can lead to tight coupling and reduced flexibility. In the future, always ensure that dependencies are injected via constructors as required, and avoid creating them internally within service classes. This approach will help you write more maintainable and testable code.
I am approving your PR to help you move forward with your learning journey. Please review the comments and feedback carefully, and make sure to apply these best practices in your future work to avoid similar issues. Keep up the good work and continue improving your code! 🚀 If you need more information, consider reading about Dependency Injection and best practices for service and DAO layers in Java applications.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
Country usa = new Country("USA"); | ||
CountryService countryService = null; // TODO: initialize this instance | ||
CountryService countryService = new CountryServiceImpl(sessionFactory); |
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.
Issue: The CountryServiceImpl
constructor is called with only the SessionFactory
, but according to the checklist, it should be initialized with both the CountryDaoImpl
instance and the SessionFactory
. Please update this line to pass both arguments.
Actor vinDiesel = new Actor("Vin Diesel"); | ||
vinDiesel.setCountry(usa); | ||
ActorService actorService = null; // TODO: initialize this instance | ||
ActorService actorService = new ActorServiceImpl(sessionFactory); |
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.
Issue: The ActorServiceImpl
constructor is called with only the SessionFactory
, but it should be initialized with both the ActorDaoImpl
instance and the SessionFactory
as per the checklist. Please update this line accordingly.
Movie fastAndFurious = new Movie("Fast and Furious"); | ||
fastAndFurious.setActors(List.of(vinDiesel)); | ||
MovieService movieService = null; // TODO: initialize this instance | ||
MovieService movieService = new MovieServiceImpl(sessionFactory); |
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.
Issue: The MovieServiceImpl
constructor is called with only the SessionFactory
, but it should be initialized with both the MovieDaoImpl
instance and the SessionFactory
as required by the checklist. Please update this line to pass both arguments.
public ActorServiceImpl(SessionFactory sessionFactory) { | ||
this.actorDao = new ActorDaoImpl(sessionFactory); | ||
} |
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.
Error: The constructor only accepts a SessionFactory
and creates a new ActorDaoImpl
internally. According to the checklist, the constructor should accept both an ActorDao
and a SessionFactory
as parameters, and assign the provided ActorDao
to the field. Please update the constructor accordingly.
public CountryServiceImpl(SessionFactory sessionFactory) { | ||
this.countryDao = new CountryDaoImpl(sessionFactory); | ||
} |
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.
Error: The constructor only accepts a SessionFactory
and creates a new CountryDaoImpl
internally. According to the checklist, the constructor should accept both a CountryDao
and a SessionFactory
as parameters, and assign the provided CountryDao
to the field. Please update the constructor accordingly.
public MovieServiceImpl(SessionFactory sessionFactory) { | ||
this.movieDao = new MovieDaoImpl(sessionFactory); | ||
} |
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.
Error: The constructor only accepts a SessionFactory
and creates a new MovieDaoImpl
internally. According to the checklist, the constructor should accept both a MovieDao
and a SessionFactory
as parameters, and assign the provided MovieDao
to the field. Please update the constructor accordingly.
No description provided.