Features/loan#12
Conversation
…loans Co-authored-by: Dennis Seldén <111012436+dennsel@users.noreply.github.com> Co-authored-by: Linda Eskilsson <eskilsson.linda@hotmail.se> Co-authored-by: Erika Falk Svendsen <alicia-fs@outlook.com>
📝 WalkthroughWalkthroughThis PR implements a loan management feature for a library system. It updates JPA entity mappings to make Loan the owning side of the Book relationship, refactors the LoanServices API to accept entity objects instead of IDs, adds an "Mina lån" (My Loans) menu option to view and return active loans, and enhances the book search UI to display loan availability and enable book loaning. Changes
Sequence DiagramsequenceDiagram
actor User
participant App
participant LoanServices
participant Database
User->>App: Select option 4 "Mina lån"
App->>LoanServices: activeLoans(currentUser)
LoanServices->>Database: Query loans WHERE user_id = ? AND returnDate IS NOT NULL
Database-->>LoanServices: Return list of active loans
LoanServices-->>App: List<Loan>
App->>User: Display active loans with return dates
User->>App: Select loan to return
App->>LoanServices: returnBook(user, book)
LoanServices->>Database: Fetch loan & detach book
LoanServices->>Database: Remove loan record
Database-->>LoanServices: Loan deleted
LoanServices-->>App: Success
App->>User: Confirmation & updated loan list
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
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/LoanServices.java (2)
16-30: Inverted logic inisBookLoanedquery.The query
WHERE l.returnDate IS NOT NULLreturns loans that have a return date set. Based onloanBook()(line 47),returnDateis set to the expected return date at loan creation. This meansisBookLoanedwould returntruefor all loans (sincereturnDateis always set), making the check ineffective.If
returnDaterepresents the "expected return date" and loans are deleted when returned, the query should check for the existence of a loan record instead:🐛 Proposed fix
public boolean isBookLoaned(Long bookId) { List<Loan> loans = em.createQuery( - "SELECT l FROM Loan l WHERE l.book.bookId = :bookId AND l.returnDate IS NOT NULL", + "SELECT l FROM Loan l WHERE l.book.bookId = :bookId", Loan.class ) .setParameter("bookId", bookId) .getResultList(); - if (loans.isEmpty()) { - return false; - } else { - return true; - } + return !loans.isEmpty(); }
60-68: Same query logic issue inreturnBook.The query
WHERE l.returnDate IS NOT NULLhas the same issue asisBookLoaned. Since loans are deleted upon return (line 78), simply find the loan by user and book:🐛 Proposed fix
Loan loan = em.createQuery( - "SELECT l FROM Loan l WHERE l.user = :user AND l.book = :book AND l.returnDate IS NOT NULL", + "SELECT l FROM Loan l WHERE l.user = :user AND l.book = :book", Loan.class )
🤖 Fix all issues with AI agents
In @src/main/java/org/example/App.java:
- Line 43: The code instantiates LoanServices inside the main loop causing
repeated creation of LoanServices and its EntityManager; move the instantiation
of LoanServices loanServices = new LoanServices(EMFactory.getEntityManager())
out of the loop and create it once before entering the loop alongside searchCli
and userCli so the same LoanServices/EntityManager is reused for each iteration.
- Around line 58-92: This code path lacks a login guard and doesn't handle
non-numeric input: check SessionManager.getCurrentUser() for null before calling
loanServices.activeLoans() (if null, print a message asking the user to log in
and break/return to the menu), and wrap the Integer.parseInt(inputString) call
in a try-catch for NumberFormatException (on catch, print an "invalid input"
message and continue the while loop without crashing); keep using
loans.remove(...) and the existing logic when valid.
🧹 Nitpick comments (5)
src/main/java/org/example/Loan.java (1)
58-60: JPA mapping looks correct, but consider cascade and orphanRemoval.The
@OneToOnewith@JoinColumncorrectly establishes Loan as the owning side. However, there's no cascade or orphanRemoval configured. If a Loan is removed, the Book reference remains intact (which is likely desired), but ensure this aligns with your persistence strategy inLoanServices.Also, note the inconsistent column naming convention:
bookIDvsloan_user(line 55). Consider standardizing to snake_case (e.g.,book_id).src/main/java/org/example/SearchCli.java (2)
115-115: Consider injectingLoanServicesinstead of creating new instances.Creating a new
LoanServicesinstance on every call toprintBookDetailsis inefficient. Consider injectingLoanServicesvia the constructor or as a class field, similar to howemandsearchServiceare handled.♻️ Suggested refactor
public class SearchCli { static EntityManager em; static BookSearch searchService = new BookSearch(); + static LoanServices loanServices; static Scanner sc; // Constructor? public SearchCli(Scanner sc, EntityManager em) { this.sc = sc; this.em = em; + this.loanServices = new LoanServices(em); }Then use the field in
printBookDetailsinstead of instantiating a new one.
156-162: Clean up commented-out code and TODO comments.These lines contain dead code and incomplete TODO items. The TODO mentions redirecting non-logged-in users to the login page, but the current implementation simply doesn't show the loan option to guests. Either implement the TODO or remove these comments if the current behavior is intentional.
Would you like me to help implement the redirect-to-login flow, or should these comments be removed?
src/main/java/org/example/LoanServices.java (2)
46-47: Consider renamingreturnDatetodueDatefor clarity.Setting
returnDateat loan creation (line 47) suggests this is the due date, not the actual return date. This naming confusion affects the entire codebase. Consider renaming todueDateorexpectedReturnDateto better reflect its purpose.
39-52: Add exception handling with transaction rollback.If an exception occurs after
begin()but beforecommit(), the transaction remains open. Consider wrapping in try-catch with rollback in the catch/finally block.♻️ Suggested pattern
try { em.getTransaction().begin(); // ... operations ... em.getTransaction().commit(); } catch (Exception e) { if (em.getTransaction().isActive()) { em.getTransaction().rollback(); } throw e; // or handle appropriately }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/example/App.javasrc/main/java/org/example/Book.javasrc/main/java/org/example/Loan.javasrc/main/java/org/example/LoanServices.javasrc/main/java/org/example/SearchCli.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/org/example/SearchCli.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/EMFactory.java (1)
EMFactory(7-30)src/main/java/org/example/user/SessionManager.java (1)
SessionManager(5-33)
🔇 Additional comments (4)
src/main/java/org/example/SearchCli.java (1)
127-154: LGTM!The loan status display and interactive loan flow are well-structured. The login check before presenting the loan option is appropriate.
src/main/java/org/example/Book.java (1)
84-85: LGTM!The
mappedBy = "book"correctly establishes Book as the inverse side of the bidirectional relationship with Loan. This is consistent with Loan.java owning the relationship via@JoinColumn.src/main/java/org/example/App.java (1)
23-24: LGTM!The
DateTimeFormatteris appropriately initialized for displaying loan return dates.src/main/java/org/example/LoanServices.java (1)
83-91: LGTM!Given that loans are deleted upon return, this query correctly returns all active loans for the user.
|
|
||
| System.out.print("Menyval: "); | ||
| String choice = scanner.nextLine(); | ||
| LoanServices loanServices = new LoanServices(EMFactory.getEntityManager()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move LoanServices instantiation outside the loop.
Creating a new LoanServices (and thus potentially a new EntityManager reference) on every iteration of the main loop is inefficient. This should be instantiated once before the loop, similar to searchCli and userCli.
♻️ Suggested refactor
SearchCli searchCli = new SearchCli(scanner, EMFactory.getEntityManager());
+ LoanServices loanServices = new LoanServices(EMFactory.getEntityManager());
DateTimeFormatter formatter =
DateTimeFormatter.ofPattern("yyyy-MM-dd");
// ...
switch (choice) {
- LoanServices loanServices = new LoanServices(EMFactory.getEntityManager());🤖 Prompt for AI Agents
In @src/main/java/org/example/App.java at line 43, The code instantiates
LoanServices inside the main loop causing repeated creation of LoanServices and
its EntityManager; move the instantiation of LoanServices loanServices = new
LoanServices(EMFactory.getEntityManager()) out of the loop and create it once
before entering the loop alongside searchCli and userCli so the same
LoanServices/EntityManager is reused for each iteration.
| case "4" -> { | ||
| System.out.println("Dina aktiva lån:"); | ||
| List<Loan> loans = loanServices.activeLoans(SessionManager.getCurrentUser()); | ||
|
|
||
| boolean running = true; | ||
| while (running) { | ||
| if (!loans.isEmpty()) { | ||
| int counter = 0; | ||
| for (Loan loan : loans) { | ||
| counter += 1; | ||
| System.out.println(counter + ". Titel: " + loan.getBook().getTitle() + " - Åter: " + loan.getReturnDate() | ||
| .format(formatter)); | ||
| } | ||
| System.out.println("Välj nummer för den bok du vill lämna tillbaka (0 = tillbaka): "); | ||
| String inputString = scanner.nextLine(); | ||
| Integer input = Integer.parseInt(inputString); | ||
| if (input >= 1 && input <= loans.size()) { | ||
| loanServices.returnBook( | ||
| SessionManager.getCurrentUser(), | ||
| loans.get(input - 1).getBook() | ||
| ); | ||
| loans.remove(input - 1); | ||
| System.out.println("Du har lämnat tillbaka din bok!"); | ||
| } else if (input == 0) { | ||
| running = false; | ||
| } else { | ||
| System.out.println("ogiltigt val."); | ||
| } | ||
|
|
||
| } else { | ||
| System.out.println("Inga aktiva lån."); | ||
| running = false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add login check and handle NumberFormatException.
Two issues:
-
Missing login guard: While menu option "4" is only displayed when logged in, a user can still type "4" when logged out.
SessionManager.getCurrentUser()would returnnull, causing issues inactiveLoans(). -
Unhandled
NumberFormatException: Line 73 usesInteger.parseInt()without try-catch. Non-numeric input will crash the application.
🐛 Proposed fix
case "4" -> {
+ if (!SessionManager.isLoggedIn()) {
+ System.out.println("Du måste logga in först.");
+ break;
+ }
System.out.println("Dina aktiva lån:");
List<Loan> loans = loanServices.activeLoans(SessionManager.getCurrentUser());
boolean running = true;
while (running) {
if (!loans.isEmpty()) {
int counter = 0;
for (Loan loan : loans) {
counter += 1;
System.out.println(counter + ". Titel: " + loan.getBook().getTitle() + " - Åter: " + loan.getReturnDate()
.format(formatter));
}
System.out.println("Välj nummer för den bok du vill lämna tillbaka (0 = tillbaka): ");
String inputString = scanner.nextLine();
- Integer input = Integer.parseInt(inputString);
+ int input;
+ try {
+ input = Integer.parseInt(inputString);
+ } catch (NumberFormatException e) {
+ System.out.println("Skriv en siffra.");
+ continue;
+ }
if (input >= 1 && input <= loans.size()) {🤖 Prompt for AI Agents
In @src/main/java/org/example/App.java around lines 58 - 92, This code path
lacks a login guard and doesn't handle non-numeric input: check
SessionManager.getCurrentUser() for null before calling
loanServices.activeLoans() (if null, print a message asking the user to log in
and break/return to the menu), and wrap the Integer.parseInt(inputString) call
in a try-catch for NumberFormatException (on catch, print an "invalid input"
message and continue the while loop without crashing); keep using
loans.remove(...) and the existing logic when valid.
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.