1 #942
1 #942
Conversation
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! Before your code can be approved, there are a few critical issues that need to be addressed to meet the core requirements and ensure correct functionality:
- Password Security: In
AuthenticationServiceImpl, passwords are set directly without hashing or generating a salt during registration. This is a critical security flaw and will cause login to fail, as the stored password is not hashed. Please update the registration logic to hash and salt passwords before saving the user (see how it's done inUserServiceImpl). - Exception Handling: Both
AuthenticationExceptionandRegistrationExceptiondo not callsuper(message)in their constructors, so exception messages are not set. This makes debugging and error reporting difficult. Please update the constructors to call the superclass with the provided message. - Potential NullPointerException: In
MovieSession.setShowTime(int year, int month, int day, int hour, int minute), you are usingshowTime.of(...)instead ofLocalDateTime.of(...). This will throw aNullPointerExceptionifshowTimeis null. Please fix this by usingLocalDateTime.of(...). - Other Minor Issues: There are several minor issues, such as using
Booleaninstead ofbooleanfor return types, unchecked query types, and generic exceptions in DAO methods. These do not block approval but should be improved for code quality and maintainability.
Once you address the critical issues above, your code will meet the task requirements. Keep up the great work and feel free to ask if you need help with any of the fixes! 🚀
✨ 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
| .createQuery(CinemaHall.class); | ||
| criteriaQuery.from(CinemaHall.class); | ||
| return session.createQuery(criteriaQuery).getResultList(); | ||
| return session.createQuery("from CinemaHall").list(); |
There was a problem hiding this comment.
Issue: The query in getAll() should specify the result type for type safety. Consider using: session.createQuery("from CinemaHall", CinemaHall.class).list();
| } | ||
| } | ||
|
|
||
| return get(cinemaHall.getId()).get().equals(cinemaHall); |
There was a problem hiding this comment.
Potential issue: The update() method compares entities using .equals(). If CinemaHall does not override equals() properly, this check may not work as intended. Ensure equals() is correctly implemented or consider another way to verify update success.
| session.delete(get(id) | ||
| .orElseThrow(() -> new RuntimeException("no object with id " + id))); |
There was a problem hiding this comment.
Issue: In the delete() method, throwing a generic RuntimeException when the entity is not found may not provide enough context. Consider using a more specific exception or handling this case more gracefully if required by your checklist or standards.
| .createQuery(Movie.class); | ||
| criteriaQuery.from(Movie.class); | ||
| return session.createQuery(criteriaQuery).getResultList(); | ||
| return session.createQuery("from Movie").list(); |
There was a problem hiding this comment.
Issue: The query in getAll() should specify the result type for type safety. Consider using: session.createQuery("from Movie", Movie.class).list();
| } | ||
| } | ||
|
|
||
| return get(movie.getId()).get().equals(movie); |
There was a problem hiding this comment.
Potential issue: The update() method compares entities using .equals(). If Movie does not override equals() properly, this check may not work as intended. Ensure equals() is correctly implemented or consider another way to verify update success.
| public Boolean delete(Long id) { | ||
| return cinemaHallDao.delete(id); | ||
| } |
There was a problem hiding this comment.
Issue: The delete method returns a Boolean object. If not required by the checklist, consider using the primitive boolean type for clarity and to avoid unnecessary boxing.
| public Boolean update(Movie movie) { | ||
| Movie currentMovie = get(movie.getId()); | ||
| movieDao.update(movie); | ||
| return currentMovie.equals(get(movie.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method returns a Boolean object. If not required by the checklist, consider using the primitive boolean type for clarity and to avoid unnecessary boxing.
| public Boolean delete(Long id) { | ||
| return movieDao.delete(id); | ||
| } |
There was a problem hiding this comment.
Issue: The delete method returns a Boolean object. If not required by the checklist, consider using the primitive boolean type for clarity and to avoid unnecessary boxing.
| public Boolean update(MovieSession movieSession) { | ||
| MovieSession currentMovieSession = get(movieSession.getId()); | ||
| movieSessionDao.update(movieSession); | ||
| return currentMovieSession.equals(get(movieSession.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method returns a Boolean object. If not required by the checklist, consider using the primitive boolean type for clarity and to avoid unnecessary boxing.
| public Boolean delete(Long id) { | ||
| return movieSessionDao.delete(id); |
There was a problem hiding this comment.
Issue: The delete method returns a Boolean object. If not required by the checklist, consider using the primitive boolean type for clarity and to avoid unnecessary boxing.
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! There are several critical issues that need to be addressed before your code can be approved. Most importantly, the UserDao, UserDaoImpl, UserService, and UserServiceImpl classes are missing update and delete methods, which are required for full CRUD functionality and are likely checklist requirements. Additionally, password handling during registration is incorrect—passwords are not being hashed or salted, which will cause authentication to fail. There are also critical bugs in your exception constructors (messages are not set) and in the MovieSession entity (incorrect use of showTime.of instead of LocalDateTime.of). Please address these issues to meet the core requirements of the task. Once fixed, your code will be much closer to approval! 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
| public interface UserDao { | ||
| User add(User user); | ||
|
|
||
| Optional<User> get(String email); | ||
| } |
There was a problem hiding this comment.
Issue: The UserDao interface is missing update and delete methods for User entities. According to standard DAO patterns and likely checklist requirements, you should provide methods for updating and deleting users in addition to add and get.
| .createQuery(CinemaHall.class); | ||
| criteriaQuery.from(CinemaHall.class); | ||
| return session.createQuery(criteriaQuery).getResultList(); | ||
| return session.createQuery("from CinemaHall").list(); |
There was a problem hiding this comment.
Issue: The query in getAll() should specify the result type to avoid unchecked cast warnings. Consider using: session.createQuery("from CinemaHall", CinemaHall.class).list();
| } | ||
| } | ||
|
|
||
| return get(cinemaHall.getId()).get().equals(cinemaHall); |
There was a problem hiding this comment.
Issue: The update() method returns a boolean based on equality between the updated entity and the input entity. This logic may not be reliable due to possible differences in object references or detached state. Consider returning true if no exception is thrown, or checking the affected row count if possible.
| .createQuery(Movie.class); | ||
| criteriaQuery.from(Movie.class); | ||
| return session.createQuery(criteriaQuery).getResultList(); | ||
| return session.createQuery("from Movie").list(); |
There was a problem hiding this comment.
Issue: The query in getAll() should specify the result type to avoid unchecked cast warnings. Consider using: session.createQuery("from Movie", Movie.class).list();
| } | ||
| } | ||
|
|
||
| return get(movie.getId()).get().equals(movie); |
There was a problem hiding this comment.
Issue: The update() method returns a boolean based on equality between the updated entity and the input entity. This logic may not be reliable due to possible differences in object references or detached state. Consider returning true if no exception is thrown, or checking the affected row count if possible.
| } | ||
|
|
||
| @Override | ||
| public Boolean delete(Long id) { |
There was a problem hiding this comment.
Issue: The delete method returns a 'Boolean' object. Unless the checklist requires this, consider using the primitive 'boolean' for consistency and to avoid unnecessary boxing.
| public Boolean update(MovieSession movieSession) { | ||
| MovieSession currentMovieSession = get(movieSession.getId()); | ||
| movieSessionDao.update(movieSession); | ||
| return currentMovieSession.equals(get(movieSession.getId())); |
There was a problem hiding this comment.
Issue: The update() method returns the result of comparing the current and updated MovieSession objects for equality. This is not a reliable way to determine if the update was successful, as object equality may not reflect the actual update status. Consider returning true if no exception is thrown or using the DAO's return value.
| } | ||
|
|
||
| @Override | ||
| public Boolean update(MovieSession movieSession) { |
There was a problem hiding this comment.
Issue: The update method returns a 'Boolean' object. Unless the checklist requires this, consider using the primitive 'boolean' for consistency and to avoid unnecessary boxing.
| } | ||
|
|
||
| @Override | ||
| public MovieSession add(MovieSession session) { | ||
| return sessionDao.add(session); | ||
| public Boolean delete(Long id) { |
There was a problem hiding this comment.
Issue: The delete method returns a 'Boolean' object. Unless the checklist requires this, consider using the primitive 'boolean' for consistency and to avoid unnecessary boxing.
| public class UserServiceImpl implements UserService { | ||
| @Inject | ||
| private UserDao userDao; | ||
|
|
||
| @Override | ||
| public User add(User user) { | ||
| user.setSalt(HashUtil.getSalt()); | ||
| user.setPassword(HashUtil.hashPassword(user.getPassword(), user.getSalt())); | ||
| return userDao.add(user); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<User> findByEmail(String email) { | ||
| return userDao.get(email); | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue: The UserServiceImpl class does not implement update and delete methods for User entities. According to standard service layer patterns and likely checklist requirements, you should provide methods for updating and deleting users in addition to add and findByEmail.
There was a problem hiding this comment.
Review Summary
Thank you for your submission! There are a few critical issues that need to be addressed before your code can be approved:
-
Missing CRUD Methods for User: The
UserDao,UserDaoImpl,UserService, andUserServiceImplclasses/interfaces are missingupdateanddeletemethods. Full CRUD functionality is required for all entities according to the checklist and task requirements. Please add these methods to both the interfaces and their implementations. -
Password Security: Passwords are currently being stored in plain text during user registration. This is a critical security issue and does not meet the core requirements. You must use
HashUtilto generate a salt and hash the password before saving it to the database, and store both the hash and the salt in theUserentity. -
Update Method Logic: In your update methods for
CinemaHall,Movie, andMovieSession, you are comparing the previous and updated objects for equality after updating, which will always return false if any fields have changed. Instead, you should return the result of the DAO's update operation or compare the updated entity to the new state. -
Minor Issues: There are some minor issues with type safety in your
getAll()methods forMovieDaoImplandCinemaHallDaoImpl. While not critical, consider using typed queries to improve code quality.
Once you address the missing CRUD methods for the user entity and fix the password handling to use hashing and salting, your code will meet the core requirements. Keep up the good work—you're almost there! 🚀 If you need help with password hashing or CRUD patterns, check out the OWASP Password Storage Cheat Sheet or Java DAO best practices.
✨ 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
|
|
||
| // 4. REGISTRATION | ||
| // Correct | ||
| authenticationService.registerUser("jan@gmail.com", "haslojana"); |
There was a problem hiding this comment.
Issue: The registration step (authenticationService.registerUser) appears to store the password in plain text. According to typical security requirements and likely the checklist, passwords should be hashed and salted during registration. Please verify that your AuthenticationService implementation hashes and salts passwords before storing them, as this is critical for secure authentication.
| public interface UserDao { | ||
| User add(User user); | ||
|
|
||
| Optional<User> get(String email); | ||
| } |
There was a problem hiding this comment.
Issue: The UserDao interface is missing update and delete methods. According to the checklist, all DAO interfaces should provide full CRUD functionality. Please add methods for updating and deleting a user, such as boolean update(User user); and boolean delete(String email);.
| .createQuery(CinemaHall.class); | ||
| criteriaQuery.from(CinemaHall.class); | ||
| return session.createQuery(criteriaQuery).getResultList(); | ||
| return session.createQuery("from CinemaHall").list(); |
There was a problem hiding this comment.
Minor issue: The result of session.createQuery("from CinemaHall").list(); is not type-safe and may produce unchecked warnings. Consider using a typed query: session.createQuery("from CinemaHall", CinemaHall.class).getResultList(); for better type safety. This is not critical but improves code quality.
| .createQuery(Movie.class); | ||
| criteriaQuery.from(Movie.class); | ||
| return session.createQuery(criteriaQuery).getResultList(); | ||
| return session.createQuery("from Movie").list(); |
There was a problem hiding this comment.
Minor issue: The result of session.createQuery("from Movie").list(); is not type-safe and may produce unchecked warnings. Consider using a typed query: session.createQuery("from Movie", Movie.class).getResultList(); for better type safety. This is not critical but improves code quality.
| public class UserDaoImpl implements UserDao { | ||
| @Override | ||
| public User add(User user) { | ||
| Transaction transaction = null; | ||
| Session session = null; | ||
| try { | ||
| session = HibernateUtil.getSessionFactory().openSession(); | ||
| transaction = session.beginTransaction(); | ||
| session.persist(user); | ||
| transaction.commit(); | ||
| return user; | ||
| } catch (Exception e) { | ||
| if (transaction != null) { | ||
| transaction.rollback(); | ||
| } | ||
| throw new DataProcessingException("Can't insert user: " + user, e); | ||
| } finally { | ||
| if (session != null) { | ||
| session.close(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<User> get(String email) { | ||
| try (Session session = HibernateUtil.getSessionFactory().openSession()) { | ||
| return session.createQuery("from User user where user.email = :email", User.class) | ||
| .setParameter("email", email) | ||
| .uniqueResultOptional(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue: The UserDaoImpl class is missing update and delete methods. According to the checklist, all DAO implementations should provide full CRUD functionality. Please add methods for updating and deleting a user, such as boolean update(User user); and boolean delete(String email);, to match the interface and requirements.
| } | ||
| User newUser = new User(); | ||
| newUser.setEmail(email); | ||
| newUser.setPassword(password); |
There was a problem hiding this comment.
Critical issue: The password is being stored in plain text (newUser.setPassword(password);). According to security best practices and likely checklist requirements, passwords must be hashed and salted before being saved. Use HashUtil to generate a salt and hash the password, then store both in the User entity.
| public Boolean update(CinemaHall cinemaHall) { | ||
| CinemaHall currentCinemaHall = get(cinemaHall.getId()); | ||
| cinemaHallDao.update(cinemaHall); | ||
| return currentCinemaHall.equals(get(cinemaHall.getId())); |
There was a problem hiding this comment.
Logic issue: The update method compares the previous and updated CinemaHall objects for equality after updating, which will always return false if the update changed any fields. Instead, you should return the result of the DAO's update operation or compare the updated entity to the new state.
| public Boolean update(Movie movie) { | ||
| Movie currentMovie = get(movie.getId()); | ||
| movieDao.update(movie); | ||
| return currentMovie.equals(get(movie.getId())); |
There was a problem hiding this comment.
Logic issue: The update method compares the previous and updated Movie objects for equality after updating, which will always return false if the update changed any fields. Instead, you should return the result of the DAO's update operation or compare the updated entity to the new state.
| public Boolean update(MovieSession movieSession) { | ||
| MovieSession currentMovieSession = get(movieSession.getId()); | ||
| movieSessionDao.update(movieSession); | ||
| return currentMovieSession.equals(get(movieSession.getId())); |
There was a problem hiding this comment.
Logic issue: The update method compares the previous and updated MovieSession objects for equality after updating, which will always return false if the update changed any fields. Instead, you should return the result of the DAO's update operation or compare the updated entity to the new state.
| public class UserServiceImpl implements UserService { | ||
| @Inject | ||
| private UserDao userDao; | ||
|
|
||
| @Override | ||
| public User add(User user) { | ||
| user.setSalt(HashUtil.getSalt()); | ||
| user.setPassword(HashUtil.hashPassword(user.getPassword(), user.getSalt())); | ||
| return userDao.add(user); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<User> findByEmail(String email) { | ||
| return userDao.get(email); | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue: The UserServiceImpl class is missing update and delete methods. According to the checklist, all service implementations should provide full CRUD functionality. Please add methods for updating and deleting a user, such as boolean update(User user); and boolean delete(String email);, to match the interface and requirements.
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! Before your code can be approved, there are a few critical issues that need to be addressed to meet the core requirements of the task:
-
Incomplete CRUD for User Entity:
- The
UserDaointerface andUserDaoImplare missingupdateanddeletemethods/implementations. - The
UserServiceinterface andUserServiceImplare also missingupdateanddeletemethods/implementations. - Full CRUD support is required for all entities as per the checklist.
- The
-
Password Security:
- In your registration logic, passwords are being set directly without hashing or salting. This is a critical security issue and does not meet the requirements. Please use
HashUtilto hash and salt passwords before storing them.
- In your registration logic, passwords are being set directly without hashing or salting. This is a critical security issue and does not meet the requirements. Please use
-
Update Method Logic in Service Implementations:
- In
CinemaHallServiceImpl,MovieServiceImpl, andMovieSessionServiceImpl, theupdatemethods currently compare the old and new objects usingequals, which will almost always returnfalseif any field changes. Instead, you should return the result of the corresponding DAO'supdatemethod directly.
- In
All other aspects of your code are well-implemented and follow the requirements. Once you address these critical issues, your solution will be ready for approval! Keep up the great work—you're almost there! 🚀 If you need guidance on implementing any of these changes, feel free to ask.
✨ 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
| public interface UserDao { | ||
| User add(User user); | ||
|
|
||
| Optional<User> get(String email); | ||
| } |
There was a problem hiding this comment.
Issue: The UserDao interface is missing update and delete methods. According to the checklist, all DAO interfaces should provide full CRUD operations. Please add methods for updating and deleting a user (e.g., boolean update(User user); and boolean delete(String email);).
| public class UserDaoImpl implements UserDao { | ||
| @Override | ||
| public User add(User user) { | ||
| Transaction transaction = null; | ||
| Session session = null; | ||
| try { | ||
| session = HibernateUtil.getSessionFactory().openSession(); | ||
| transaction = session.beginTransaction(); | ||
| session.persist(user); | ||
| transaction.commit(); | ||
| return user; | ||
| } catch (Exception e) { | ||
| if (transaction != null) { | ||
| transaction.rollback(); | ||
| } | ||
| throw new DataProcessingException("Can't insert user: " + user, e); | ||
| } finally { | ||
| if (session != null) { | ||
| session.close(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<User> get(String email) { | ||
| try (Session session = HibernateUtil.getSessionFactory().openSession()) { | ||
| return session.createQuery("from User user where user.email = :email", User.class) | ||
| .setParameter("email", email) | ||
| .uniqueResultOptional(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue: UserDaoImpl is missing update and delete methods. According to the checklist, all DAO implementations should provide full CRUD operations. Please implement update and delete methods for the User entity.
| public interface UserService { | ||
| User add(User user); | ||
|
|
||
| Optional<User> findByEmail(String email); | ||
|
|
||
| } |
There was a problem hiding this comment.
Issue: The UserService interface is missing update and delete methods. According to the checklist, all service interfaces should provide full CRUD operations. Please add methods for updating and deleting a user (e.g., boolean update(User user); and boolean delete(String email);).
| } | ||
| User newUser = new User(); | ||
| newUser.setEmail(email); | ||
| newUser.setPassword(password); |
There was a problem hiding this comment.
Error: Password is being set directly without hashing or salting. According to the checklist and security best practices, passwords must be hashed and salted before being stored. Please use HashUtil to hash the password and generate a salt, then store both the hashed password and the salt in the User object.
| public Boolean update(CinemaHall cinemaHall) { | ||
| CinemaHall currentCinemaHall = get(cinemaHall.getId()); | ||
| cinemaHallDao.update(cinemaHall); | ||
| return currentCinemaHall.equals(get(cinemaHall.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method compares the current and updated CinemaHall objects using equals, which will return false if any field is changed by the update. This does not accurately reflect whether the update was successful. Consider returning the result of cinemaHallDao.update(cinemaHall) directly instead.
| public Boolean update(Movie movie) { | ||
| Movie currentMovie = get(movie.getId()); | ||
| movieDao.update(movie); | ||
| return currentMovie.equals(get(movie.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method compares the current and updated Movie objects using equals, which will return false if any field is changed by the update. This does not accurately reflect whether the update was successful. Consider returning the result of movieDao.update(movie) directly instead.
| public Boolean update(MovieSession movieSession) { | ||
| MovieSession currentMovieSession = get(movieSession.getId()); | ||
| movieSessionDao.update(movieSession); | ||
| return currentMovieSession.equals(get(movieSession.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method compares the current and updated MovieSession objects using equals, which will return false if any field is changed by the update. This does not accurately reflect whether the update was successful. Consider returning the result of movieSessionDao.update(movieSession) directly instead.
| public class UserServiceImpl implements UserService { | ||
| @Inject | ||
| private UserDao userDao; | ||
|
|
||
| @Override | ||
| public User add(User user) { | ||
| user.setSalt(HashUtil.getSalt()); | ||
| user.setPassword(HashUtil.hashPassword(user.getPassword(), user.getSalt())); | ||
| return userDao.add(user); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<User> findByEmail(String email) { | ||
| return userDao.get(email); | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue: UserServiceImpl is missing update and delete methods. According to the checklist, all service implementations should provide full CRUD operations. Please implement update and delete methods for the User entity.
There was a problem hiding this comment.
Review Summary
Thank you for your submission! 🚀 Your code meets almost all of the core requirements for the task, including full CRUD operations in all DAO and Service layers, secure password handling (hashing and salting), and the presence of all necessary files. However, there are a few important issues to address:
- Update Method Logic: In your
CinemaHallServiceImpl,MovieServiceImpl, andMovieSessionServiceImpl, theupdatemethods currently compare objects usingequalsto determine if the update was successful. According to the checklist, you should return the result of the DAO'supdatemethod directly (e.g.,return movieDao.update(movie);). This ensures proper CRUD operation implementation and avoids unnecessary logic. - Password Security (Previously): There were critical issues where passwords were set directly on
Userobjects without hashing or salting. This has now been addressed in your latest implementation by usingHashUtilfor secure password storage. Please always ensure passwords are never stored in plain text. - General Quality: All other requirements are met, and your code structure is clean and well-organized. No unnecessary or missing files were found.
Motivation:
I'm approving your pull request to help you move forward with your learning journey. Please review the comments and address the highlighted issues, especially the update method logic, in your next iteration. Ensuring correct CRUD logic and secure password handling is essential for robust and secure applications. Keep up the good work, and continue improving your code! If you need more details, feel free to check the checklist or reach out for clarification. 👍
Tip: For more on secure password storage, see OWASP Password Storage Cheat Sheet.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| // User | ||
| User user1 = new User(); | ||
| user1.setEmail("user1@gmail.com"); | ||
| user1.setPassword("password"); |
There was a problem hiding this comment.
Issue: Passwords are being set directly on the User objects without hashing or salting. According to the checklist, passwords must be hashed and salted using HashUtil before being stored. Please update this logic to ensure password security.
|
|
||
| User user2 = new User(); | ||
| user2.setEmail("user2@gmail.com"); | ||
| user2.setPassword("password"); |
There was a problem hiding this comment.
Issue: Passwords are being set directly on the User objects without hashing or salting. As per the checklist, use HashUtil to hash and salt passwords before storing them.
|
|
||
| User user3 = new User(); | ||
| user3.setEmail("user3@gmail.com"); | ||
| user3.setPassword("password"); |
There was a problem hiding this comment.
Issue: Passwords are being set directly on the User objects without hashing or salting. Please use HashUtil to hash and salt passwords before storing them, as required by the checklist.
| public Boolean update(CinemaHall cinemaHall) { | ||
| CinemaHall currentCinemaHall = get(cinemaHall.getId()); | ||
| cinemaHallDao.update(cinemaHall); | ||
| return currentCinemaHall.equals(get(cinemaHall.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method currently compares the old and new objects using equals, which is not the correct way to determine if the update was successful. Instead, you should return the result of cinemaHallDao.update(cinemaHall) directly. This matches the checklist requirement for proper CRUD operation implementation.
| public Boolean update(Movie movie) { | ||
| Movie currentMovie = get(movie.getId()); | ||
| movieDao.update(movie); | ||
| return currentMovie.equals(get(movie.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method currently compares the old and new objects using equals, which is not the correct way to determine if the update was successful. Instead, you should return the result of movieDao.update(movie) directly. This matches the checklist requirement for proper CRUD operation implementation.
| public Boolean update(MovieSession movieSession) { | ||
| MovieSession currentMovieSession = get(movieSession.getId()); | ||
| movieSessionDao.update(movieSession); | ||
| return currentMovieSession.equals(get(movieSession.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method currently compares the old and new objects using equals, which is not the correct way to determine if the update was successful. Instead, you should return the result of movieSessionDao.update(movieSession) directly. This matches the checklist requirement for proper CRUD operation implementation.
There was a problem hiding this comment.
Review Summary
Great job overall! 🎉 Your implementation meets the core requirements: all DAO and service interfaces/implementations provide full CRUD methods, and password hashing/salting is handled correctly in the main authentication flows. However, there are some critical issues you should address to improve your code and ensure security and correctness:
- Update Methods: In your service implementations for
CinemaHall,Movie, andMovieSession, theupdatemethods currently compare old and new objects usingequals. This is not a reliable way to check if an update was successful. Instead, you should return the result of the DAO'supdatemethod directly (e.g.,return movieDao.update(movie);). Please update these methods to follow this pattern. - Password Security: In some places, passwords are being set directly on the
Userobject without hashing or salting. This is a critical security issue. Always use your password hashing utility (likeHashUtil) to hash and salt passwords before saving users to the database. This applies to both registration and any other flows where passwords are set or updated.
These issues are important for both security and correct application behavior. Please review the comments in your code and address these problems in your next iteration. Fixing them will help you avoid similar issues in the future and strengthen your understanding of secure coding practices.
Keep up the good work and continue learning! If you need more information, check out resources on secure password storage and DAO/service patterns. Let me know if you have any questions!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| // User | ||
| User user1 = new User(); | ||
| user1.setEmail("user1@gmail.com"); | ||
| user1.setPassword("password"); |
There was a problem hiding this comment.
Issue: Passwords are being set directly on the User object without hashing or salting. According to the checklist, passwords must be hashed and salted before storage. Please use the HashUtil (or equivalent) to hash passwords before calling userService.add(user).
|
|
||
| User user2 = new User(); | ||
| user2.setEmail("user2@gmail.com"); | ||
| user2.setPassword("password"); |
There was a problem hiding this comment.
Issue: Passwords are being set directly on the User object without hashing or salting. Please use the password hashing utility before saving the user, as required by the checklist.
|
|
||
| User user3 = new User(); | ||
| user3.setEmail("user3@gmail.com"); | ||
| user3.setPassword("password"); |
There was a problem hiding this comment.
Issue: Passwords are being set directly on the User object without hashing or salting. Please use the password hashing utility before saving the user, as required by the checklist.
| public Boolean update(CinemaHall cinemaHall) { | ||
| CinemaHall currentCinemaHall = get(cinemaHall.getId()); | ||
| cinemaHallDao.update(cinemaHall); | ||
| return currentCinemaHall.equals(get(cinemaHall.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method currently compares the old and new CinemaHall objects using equals, which is not a reliable way to determine if the update was successful. According to the checklist, you should return the result of the DAO's update method directly (i.e., return cinemaHallDao.update(cinemaHall);).
| public Boolean update(Movie movie) { | ||
| Movie currentMovie = get(movie.getId()); | ||
| movieDao.update(movie); | ||
| return currentMovie.equals(get(movie.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method currently compares the old and new Movie objects using equals, which is not a reliable way to determine if the update was successful. According to the checklist, you should return the result of the DAO's update method directly (i.e., return movieDao.update(movie);).
| public Boolean update(MovieSession movieSession) { | ||
| MovieSession currentMovieSession = get(movieSession.getId()); | ||
| movieSessionDao.update(movieSession); | ||
| return currentMovieSession.equals(get(movieSession.getId())); | ||
| } |
There was a problem hiding this comment.
Issue: The update method currently compares the old and new MovieSession objects using equals, which is not a reliable way to determine if the update was successful. According to the checklist, you should return the result of the DAO's update method directly (i.e., return movieSessionDao.update(movieSession);).
No description provided.