Features/test/loan#13
Conversation
Changed to instead of constructor inject EntityManager as method parameters. Added exception handling for transaction rollback and streamlined query logic
…return. Added a check to block guests from accessing loans.
and extracted functionalities into helper methods for improved readability. Also added more robust error handling.
📝 WalkthroughWalkthroughThis PR refactors the CLI application's architecture, shifting EntityManager lifecycle management from constructor injection to method-scoped handling. SearchCli transitions from static to instance-based methods, LoanServices becomes stateless by accepting EntityManager parameters, and App.java is reorganized with helper methods for menu rendering and business logic encapsulation. A CLI bootstrap test class is removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/user/UserCLI.java (1)
79-82: RedundantSessionManager.login(user)call.Since
userService.login()now internally callsSessionManager.login(user)(seeUserService.javaline 33), this call at line 82 is redundant. The session is already set whenlogin()returns.Suggested fix
// Check credentials with database User user = userService.login(username, password); - // Tell SessionManager to remember user - SessionManager.login(user); - if (SessionManager.isLoggedIn()){ System.out.println("Inloggningen lyckades. Välkommen " + user.getFirstName() + "!"); }src/main/java/org/example/App.java (1)
15-15: Missingpublicmodifier onmainmethod.The entry point
main(String[] args)must be declaredpublicfor the JVM to invoke it. Without this modifier, the application will fail to start with an error like "Main method not found".Proposed fix
- static void main(String[] args) { + public static void main(String[] args) {
🤖 Fix all issues with AI agents
In @src/main/java/org/example/App.java:
- Around line 46-54: The case for option "3" incorrectly calls userMenu(userCli)
which ends up invoking UserCLI.userMenu() that bails out when already logged in;
change the call to userCli.manageUserMenu() (or delegate to
userCli.manageUserMenu via the same wrapper method) so that when
SessionManager.isLoggedIn() is true the code invokes manageUserMenu() on userCli
instead of userMenu(), ensuring the logged-in user sees the manage-user flow.
In @src/main/java/org/example/SearchCli.java:
- Around line 138-142: The call to
loanServices.loanBook(SessionManager.getCurrentUser(), b, em) returns a boolean
but its value is ignored, so the success message is always shown; change the
code to capture the boolean result (e.g., boolean success =
loanServices.loanBook(...)) and only print "Du har nu lånat boken!" when success
is true, otherwise print a failure message (e.g., book already loaned or
transaction failed) and do not set isRunning = false on failure so the user can
retry or be informed; use the existing symbols loanServices.loanBook,
SessionManager.getCurrentUser(), b, em, isRunning and adjust the control flow
accordingly.
In @src/main/java/org/example/user/UserService.java:
- Around line 33-34: There is a duplicate SessionManager.login(user) call: keep
session management in the service layer (UserService.login) and remove the
redundant call from the CLI layer (UserCLI.loginMenu); update UserCLI.loginMenu
to stop calling SessionManager.login(user) after userService.login() returns so
only the SessionManager.login(user) invocation in UserService.login remains, and
run tests to ensure no other code expects CLI to manage sessions.
🧹 Nitpick comments (1)
src/main/java/org/example/SearchCli.java (1)
111-112: Consider movingLoanServicesinstantiation to a field.
LoanServicesis instantiated on everyprintBookDetailscall. Since it's now stateless, it could be a single instance field likeBookSearch.Suggested refactor
public class SearchCli { private final BookSearch searchService = new BookSearch(); + private final LoanServices loanServices = new LoanServices(); private final Scanner sc;Then use
this.loanServicesinprintBookDetailsinstead of creating a new instance.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/example/App.javasrc/main/java/org/example/LoanServices.javasrc/main/java/org/example/SearchCli.javasrc/main/java/org/example/user/UserCLI.javasrc/main/java/org/example/user/UserService.javasrc/main/java/org/example/user/testUserCli.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/user/testUserCli.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/org/example/user/UserService.java (1)
src/main/java/org/example/user/SessionManager.java (1)
SessionManager(5-33)
src/main/java/org/example/App.java (2)
src/main/java/org/example/user/SessionManager.java (1)
SessionManager(5-33)src/main/java/org/example/EMFactory.java (1)
EMFactory(7-30)
src/main/java/org/example/SearchCli.java (2)
src/main/java/org/example/EMFactory.java (1)
EMFactory(7-30)src/main/java/org/example/user/SessionManager.java (1)
SessionManager(5-33)
🔇 Additional comments (6)
src/main/java/org/example/user/UserCLI.java (1)
218-220: LGTM!The application name change to "CLIBRARY" is a straightforward cosmetic update.
src/main/java/org/example/App.java (1)
93-139: LGTM onloanMenustructure.Good use of try-with-resources for
EntityManagerlifecycle management. The input validation and error handling are appropriate.src/main/java/org/example/LoanServices.java (3)
22-52: LGTM onloanBooktransaction handling.Proper transaction demarcation with begin/commit and rollback on failure. Using
merge()to ensure entities are managed before association is correct for detached entities.
56-88: LGTM onreturnBookimplementation.Transaction handling with rollback on failure is properly implemented. The query correctly finds the active loan and removes it.
10-18: No issue found:Book.getId()correctly returns thebookIdfield.The query
l.book.bookId = :bookIdand the method callbook.getId()are consistent.Book.getId()(lines 64-66 in Book.java) returns thebookIdfield, which matches the query's reference tol.book.bookId. No correction needed.Likely an incorrect or invalid review comment.
src/main/java/org/example/SearchCli.java (1)
19-57: LGTM on EntityManager lifecycle management.Good use of try-with-resources to ensure the
EntityManageris properly closed. The refactoring to instance-based design with explicit EM propagation is clean and improves testability.
| case "1" -> { | ||
| loanServices.loanBook(SessionManager.getCurrentUser(), b); | ||
| loanServices.loanBook(SessionManager.getCurrentUser(), b, em); | ||
| isRunning = false; | ||
| System.out.println("Du har nu lånat boken!"); | ||
| } |
There was a problem hiding this comment.
Unchecked loanBook result leads to incorrect success message.
loanBook() returns boolean indicating success/failure, but the return value is ignored. If the loan fails (e.g., book already loaned, transaction error), the user will still see "Du har nu lånat boken!" which is misleading.
Proposed fix
case "1" -> {
- loanServices.loanBook(SessionManager.getCurrentUser(), b, em);
- isRunning = false;
- System.out.println("Du har nu lånat boken!");
+ boolean success = loanServices.loanBook(SessionManager.getCurrentUser(), b, em);
+ isRunning = false;
+ if (success) {
+ System.out.println("Du har nu lånat boken!");
+ } else {
+ System.out.println("Kunde inte låna boken.");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "1" -> { | |
| loanServices.loanBook(SessionManager.getCurrentUser(), b); | |
| loanServices.loanBook(SessionManager.getCurrentUser(), b, em); | |
| isRunning = false; | |
| System.out.println("Du har nu lånat boken!"); | |
| } | |
| case "1" -> { | |
| boolean success = loanServices.loanBook(SessionManager.getCurrentUser(), b, em); | |
| isRunning = false; | |
| if (success) { | |
| System.out.println("Du har nu lånat boken!"); | |
| } else { | |
| System.out.println("Kunde inte låna boken."); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/main/java/org/example/SearchCli.java around lines 138 - 142, The call to
loanServices.loanBook(SessionManager.getCurrentUser(), b, em) returns a boolean
but its value is ignored, so the success message is always shown; change the
code to capture the boolean result (e.g., boolean success =
loanServices.loanBook(...)) and only print "Du har nu lånat boken!" when success
is true, otherwise print a failure message (e.g., book already loaned or
transaction failed) and do not set isRunning = false on failure so the user can
retry or be informed; use the existing symbols loanServices.loanBook,
SessionManager.getCurrentUser(), b, em, isRunning and adjust the control flow
accordingly.
| SessionManager.login(user); | ||
| return user; |
There was a problem hiding this comment.
Duplicate SessionManager.login(user) call detected.
This line sets the session, but UserCLI.loginMenu() (lines 81-82) also calls SessionManager.login(user) after userService.login() returns. The duplicate call is redundant and could cause confusion during maintenance.
Either remove this line and keep session management in the CLI layer, or remove the call in UserCLI.loginMenu() to centralize session management here. Centralizing it in the service layer is cleaner, so the CLI call should be removed.
🤖 Prompt for AI Agents
In @src/main/java/org/example/user/UserService.java around lines 33 - 34, There
is a duplicate SessionManager.login(user) call: keep session management in the
service layer (UserService.login) and remove the redundant call from the CLI
layer (UserCLI.loginMenu); update UserCLI.loginMenu to stop calling
SessionManager.login(user) after userService.login() returns so only the
SessionManager.login(user) invocation in UserService.login remains, and run
tests to ensure no other code expects CLI to manage sessions.
…ssary `SessionManager.login` call.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/App.java (1)
15-15: Missingpublicmodifier onmainmethod.The JVM requires the entry point to be
public static void main(String[] args). Withoutpublic, the application will fail to start with a "Main method not found" error.🐛 Proposed fix
- static void main(String[] args) { + public static void main(String[] args) {
🧹 Nitpick comments (3)
src/main/java/org/example/App.java (3)
89-96: Dead code: else branch is unreachable.The only call site (line 57) already guards with
SessionManager.isLoggedIn(), so theelsebranch at lines 93-95 will never execute.♻️ Suggested simplification
private static void logOut(UserCLI userCli) { - if(SessionManager.isLoggedIn()) { - userCli.logout(); - } - else { - System.out.println("Inte inloggad."); - } + userCli.logout(); }
98-102: Redundant login check.The caller at line 37 already ensures
SessionManager.isLoggedIn()is true before callingloanMenu(). This check creates unreachable code.♻️ Suggested simplification
private static void loanMenu(Scanner sc) { - if (!SessionManager.isLoggedIn()) { - System.out.println("Logga in för att se dina lån."); - return; - } - LoanServices loanServices = new LoanServices();
107-143: Handle input value 8 and provide feedback for invalid selections.The prompt indicates "8 = tillbaka" (8 = back), but the current code only checks
if (input > 0 && input <= loans.size()). This means:
- Input
8silently does nothing despite being mentioned in the prompt- Any out-of-range numeric input (e.g.,
0,5when there are 3 loans) also silently fails with no user feedback♻️ Suggested improvement for input handling
System.out.print("Välj nummer att lämna tillbaka (8 = tillbaka): "); try { int input = Integer.parseInt(sc.nextLine()); + if (input == 8) { + return; // Go back + } else if (input > 0 && input <= loans.size()) { boolean success = loanServices.returnBook( SessionManager.getCurrentUser(), loans.get(input - 1).getBook(), em ); if (success) { System.out.println("Boken har lämnats tillbaka!"); } else { System.out.println("Kunde inte lämna tillbaka boken."); } + } else { + System.out.println("Ogiltigt val."); } }LoanServices.returnBook() correctly manages transactions within the passed EntityManager using explicit
begin()andcommit()with proper rollback handling.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/example/App.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/example/App.java (2)
src/main/java/org/example/user/SessionManager.java (1)
SessionManager(5-33)src/main/java/org/example/EMFactory.java (1)
EMFactory(7-30)
🔇 Additional comments (3)
src/main/java/org/example/App.java (3)
20-22: LGTM!Good refactoring to share a single
Scannerinstance across CLI components. This avoids resource management issues with multiple Scanner instances onSystem.in.
68-78: LGTM!Clean extraction of menu rendering logic into a dedicated helper method, improving readability.
146-159: LGTM!Initialization and termination logic properly manages the
EMFactorylifecycle.
1 participant
Summary by CodeRabbit
UI/UX Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.