Conversation
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
WalkthroughThe changes introduce a repository pattern architecture replacing direct JDBC calls with a DataSource abstraction. Two new repositories for accounts and missions are added alongside corresponding DTOs. The Main class is substantially refactored to support interactive login and menu-driven operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant Scanner
participant AccountRepository
participant MoonMissionRepository
User->>Main: java Main
Main->>Scanner: initialize Scanner
Main->>Main: initialize repositories
rect rgb(200, 220, 255)
note over Main,AccountRepository: Login Phase
Main->>User: prompt username
User->>Scanner: enter username
Scanner-->>Main: username
Main->>User: prompt password
User->>Scanner: enter password
Scanner-->>Main: password
Main->>AccountRepository: matchCredentials(username, password)
alt Credentials Valid
AccountRepository-->>Main: true
Main->>Main: login successful
else Credentials Invalid
AccountRepository-->>Main: false
Main->>User: login failed, retry
end
end
rect rgb(220, 240, 220)
note over Main,MoonMissionRepository: Menu Operations
loop until exit
Main->>User: display menu options
User->>Scanner: select option
Scanner-->>Main: menu choice
alt List Missions
Main->>MoonMissionRepository: getAllMissions()
MoonMissionRepository-->>Main: List<MissionDTO>
Main->>User: display missions
else Get Mission Details
Main->>User: prompt mission ID
User->>Scanner: enter ID
Scanner-->>Main: mission ID
Main->>MoonMissionRepository: getMissionById(id)
MoonMissionRepository-->>Main: Optional<MissionDTO>
Main->>User: display mission details
else Missions by Year
Main->>User: prompt year
User->>Scanner: enter year
Scanner-->>Main: year
Main->>MoonMissionRepository: missionCount(year)
MoonMissionRepository-->>Main: count
Main->>User: display count
else Account Operations
Main->>AccountRepository: CRUD operation
AccountRepository-->>Main: result
Main->>User: confirm operation
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (5)
src/main/java/com/repo/MoonMissionRepository.java (1)
63-66: Use an explicit column alias for portability.
rs.getInt("count(*)")relies on the database returning the expression as the column name, which is non-portable across databases. Use an explicit alias for reliability.PreparedStatement ps = c.prepareStatement( - "select count(*) from moon_mission where year(launch_date) = ?")) { + "select count(*) as cnt from moon_mission where year(launch_date) = ?")) { ps.setInt(1, year); ResultSet rs = ps.executeQuery(); if (rs.next()) { - return rs.getInt("count(*)"); + return rs.getInt("cnt"); }src/main/java/com/repo/AccountRepository.java (1)
72-87: Verify INSERT returns generated key if needed.
createAccountdoesn't retrieve the auto-generateduser_id. If the caller needs the new ID (e.g., for confirmation or further operations), consider usingStatement.RETURN_GENERATED_KEYSand returning the created account with its ID.src/main/java/com/example/SimpleDriverManagerDataSource.java (1)
59-67:unwrapandisWrapperFordon't follow JDBC contract.Per JDBC spec,
unwrap(Class<T>)should throwSQLExceptionif the class is not a wrapper for the requested interface, not returnnull. Similarly,isWrapperForshould check if this class implements the interface.@Override public <T> T unwrap(Class<T> iface) throws SQLException { - return null; + if (isWrapperFor(iface)) { + return iface.cast(this); + } + throw new SQLException("Not a wrapper for " + iface.getName()); } @Override public boolean isWrapperFor(Class<?> iface) throws SQLException { - return false; + return iface != null && iface.isAssignableFrom(getClass()); }src/main/java/com/example/Main.java (2)
95-102: Prefer interface type for better abstraction.The method parameter uses concrete
AccountRepositoryinstead of theAccountRepointerface. Using the interface would improve testability and follow dependency inversion.- private boolean login(AccountRepository ac) { + private boolean login(AccountRepo ac) {This applies to other methods as well:
createAccount,updatePassword,deleteAccount, and similarlyMoonMissionRepofor mission methods.
206-208: Address TODO: Help method for account name.There's a TODO for adding a help method for account name generation. Consider implementing this or creating a tracking issue.
Do you want me to open an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)src/main/java/com/example/Main.java(2 hunks)src/main/java/com/example/SimpleDriverManagerDataSource.java(1 hunks)src/main/java/com/repo/AccountRepo.java(1 hunks)src/main/java/com/repo/AccountRepository.java(1 hunks)src/main/java/com/repo/DTO/AccountDTO.java(1 hunks)src/main/java/com/repo/DTO/MissionDTO.java(1 hunks)src/main/java/com/repo/MoonMissionRepo.java(1 hunks)src/main/java/com/repo/MoonMissionRepository.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/example/Main.java (2)
src/main/java/com/repo/AccountRepository.java (1)
AccountRepository(14-145)src/main/java/com/repo/MoonMissionRepository.java (1)
MoonMissionRepository(14-84)
🔇 Additional comments (6)
README.md (1)
1-1: Nice addition—badge provides quick deadline reference.The GitHub Classroom assignment deadline badge is a standard and helpful addition to assignment repositories. It gives students at-a-glance access to deadline and assignment details.
src/main/java/com/repo/DTO/AccountDTO.java (1)
3-10: Consider security implications for sensitive fields.The record stores
passwordandssnas plain strings. While this is acceptable for a DTO, ensure:
- Passwords are hashed before storage in the database (not stored plain-text)
- This DTO is not inadvertently serialized to logs or API responses, as it contains PII (SSN) and credentials
The record structure itself is clean and appropriate for data transfer.
src/main/java/com/repo/DTO/MissionDTO.java (1)
5-13: LGTM!Well-structured record with appropriate types. Using
LocalDateforlaunchDateis the correct choice for date-only values.src/main/java/com/repo/MoonMissionRepo.java (1)
9-16: LGTM!Clean interface definition that properly abstracts the data access layer. The method signatures are appropriate and use
Optionalcorrectly for potentially absent results.src/main/java/com/example/SimpleDriverManagerDataSource.java (1)
11-32: Acceptable for development, but consider connection pooling for production.This simple DataSource creates a new connection on each
getConnection()call. For production use, consider HikariCP or similar connection pool to avoid connection overhead and manage resources properly.src/main/java/com/repo/AccountRepo.java (1)
9-28: LGTM!Well-designed repository interface with clear method signatures. Good use of
Optionalfor potentially absent results and appropriate separation of concerns.
| DataSource ds = new SimpleDriverManagerDataSource( | ||
| System.getProperty("APP_JDBC_URL"), | ||
| System.getProperty("APP_DB_USER"), | ||
| System.getProperty("APP_DB_PASS") | ||
| ); |
There was a problem hiding this comment.
Resolved configuration values are ignored - environment variables won't work.
The code resolves config with fallback to environment variables (lines 29-31) but then directly uses System.getProperty() instead of the resolved values. This breaks the intended fallback behavior.
DataSource ds = new SimpleDriverManagerDataSource(
- System.getProperty("APP_JDBC_URL"),
- System.getProperty("APP_DB_USER"),
- System.getProperty("APP_DB_PASS")
+ jdbcUrl,
+ dbUser,
+ dbPass
);📝 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.
| DataSource ds = new SimpleDriverManagerDataSource( | |
| System.getProperty("APP_JDBC_URL"), | |
| System.getProperty("APP_DB_USER"), | |
| System.getProperty("APP_DB_PASS") | |
| ); | |
| DataSource ds = new SimpleDriverManagerDataSource( | |
| jdbcUrl, | |
| dbUser, | |
| dbPass | |
| ); |
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 39 to 43, the DataSource is
constructed using System.getProperty(...) directly which ignores the previously
resolved configuration values (with environment-variable fallbacks on lines
29-31); replace the direct System.getProperty calls with the resolved variables
(e.g., jdbcUrl, dbUser, dbPass or whatever local names were computed earlier)
when constructing the SimpleDriverManagerDataSource so the fallback logic is
honored; ensure those resolved variables are final/visible in this scope and
used instead of calling System.getProperty again.
| String accName; //Default accountname is first three letters of first and last name | ||
| if(fn.length() <= 3 && ln.length() <=3){ //if both first and last name are 3 or fewer letters | ||
| accName = fn + ln;} //accountname is both full combined | ||
|
|
||
| else if (fn.length() <= 3){ //if only first name is 3 or fewer letters | ||
| accName = fn + ln.substring(0, 2); //accountname is full first name and first 3 from last name | ||
| } | ||
| else if(ln.length() <= 3){ //if only last name is 3 or fewer letters | ||
| accName = fn.substring(0, 2) + ln; //accountname is first 3 from first name and full last name | ||
| } | ||
| else{ //if both are longer than 3 accountname follows default pattern | ||
| accName = fn.substring(0, 2) + ln.substring(0, 2); | ||
| } |
There was a problem hiding this comment.
Account name generation uses wrong substring length.
Comments say "first 3 letters" but substring(0, 2) returns only 2 characters. If the intent is 3 characters, use substring(0, 3).
- if(fn.length() <= 3 && ln.length() <=3){ //if both first and last name are 3 or fewer letters
+ if(fn.length() <= 3 && ln.length() <= 3){ //if both first and last name are 3 or fewer letters
accName = fn + ln;} //accountname is both full combined
else if (fn.length() <= 3){ //if only first name is 3 or fewer letters
- accName = fn + ln.substring(0, 2); //accountname is full first name and first 3 from last name
+ accName = fn + ln.substring(0, 3); //accountname is full first name and first 3 from last name
}
else if(ln.length() <= 3){ //if only last name is 3 or fewer letters
- accName = fn.substring(0, 2) + ln; //accountname is first 3 from first name and full last name
+ accName = fn.substring(0, 3) + ln; //accountname is first 3 from first name and full last name
}
else{ //if both are longer than 3 accountname follows default pattern
- accName = fn.substring(0, 2) + ln.substring(0, 2);
+ accName = fn.substring(0, 3) + ln.substring(0, 3);
}📝 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.
| String accName; //Default accountname is first three letters of first and last name | |
| if(fn.length() <= 3 && ln.length() <=3){ //if both first and last name are 3 or fewer letters | |
| accName = fn + ln;} //accountname is both full combined | |
| else if (fn.length() <= 3){ //if only first name is 3 or fewer letters | |
| accName = fn + ln.substring(0, 2); //accountname is full first name and first 3 from last name | |
| } | |
| else if(ln.length() <= 3){ //if only last name is 3 or fewer letters | |
| accName = fn.substring(0, 2) + ln; //accountname is first 3 from first name and full last name | |
| } | |
| else{ //if both are longer than 3 accountname follows default pattern | |
| accName = fn.substring(0, 2) + ln.substring(0, 2); | |
| } | |
| String accName; //Default accountname is first three letters of first and last name | |
| if(fn.length() <= 3 && ln.length() <= 3){ //if both first and last name are 3 or fewer letters | |
| accName = fn + ln;} //accountname is both full combined | |
| else if (fn.length() <= 3){ //if only first name is 3 or fewer letters | |
| accName = fn + ln.substring(0, 3); //accountname is full first name and first 3 from last name | |
| } | |
| else if(ln.length() <= 3){ //if only last name is 3 or fewer letters | |
| accName = fn.substring(0, 3) + ln; //accountname is first 3 from first name and full last name | |
| } | |
| else{ //if both are longer than 3 accountname follows default pattern | |
| accName = fn.substring(0, 3) + ln.substring(0, 3); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 182 to 194, the code and
comments intend to use the first three characters of first and/or last name but
call substring(0, 2) which returns only two characters; change all substring(0,
2) calls to substring(0, 3) so you extract three characters when the name length
is > 3, and keep the existing checks (fn.length() <= 3 / ln.length() <= 3) to
ensure substring(0, 3) is only called on sufficiently long strings.
| private String getValidName(String prompt){ | ||
| while(true){ | ||
| System.out.println("\n" + prompt); | ||
| String name = scanner.nextLine().trim(); | ||
|
|
||
| if (name.isBlank()) { | ||
| System.out.println("\nCannot be blank"); | ||
| } | ||
| else if(!Pattern.matches("^[a-zA-Z]+$", name)){ | ||
| System.out.println("\nMust only contain letters"); | ||
| } | ||
| else{ | ||
| return name.substring(0, 1).toUpperCase() + name.substring(1).toLowerCase(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Edge case: Single-character name produces empty suffix.
If name is exactly 1 character, name.substring(1) returns an empty string, resulting in just the uppercase first character. Consider if this is acceptable or if minimum length validation is needed.
else{
- return name.substring(0, 1).toUpperCase() + name.substring(1).toLowerCase();
+ String capitalized = name.substring(0, 1).toUpperCase();
+ if (name.length() > 1) {
+ capitalized += name.substring(1).toLowerCase();
+ }
+ return capitalized;
}📝 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.
| private String getValidName(String prompt){ | |
| while(true){ | |
| System.out.println("\n" + prompt); | |
| String name = scanner.nextLine().trim(); | |
| if (name.isBlank()) { | |
| System.out.println("\nCannot be blank"); | |
| } | |
| else if(!Pattern.matches("^[a-zA-Z]+$", name)){ | |
| System.out.println("\nMust only contain letters"); | |
| } | |
| else{ | |
| return name.substring(0, 1).toUpperCase() + name.substring(1).toLowerCase(); | |
| } | |
| } | |
| } | |
| private String getValidName(String prompt){ | |
| while(true){ | |
| System.out.println("\n" + prompt); | |
| String name = scanner.nextLine().trim(); | |
| if (name.isBlank()) { | |
| System.out.println("\nCannot be blank"); | |
| } | |
| else if(!Pattern.matches("^[a-zA-Z]+$", name)){ | |
| System.out.println("\nMust only contain letters"); | |
| } | |
| else{ | |
| String capitalized = name.substring(0, 1).toUpperCase(); | |
| if (name.length() > 1) { | |
| capitalized += name.substring(1).toLowerCase(); | |
| } | |
| return capitalized; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 302 to 317, the current
capitalization logic uses name.substring(1) which yields an empty string for
single-character names; change the method to explicitly handle single-character
names by checking if name.length() == 1 and returning name.toUpperCase() in that
case, otherwise continue to return name.substring(0,1).toUpperCase() +
name.substring(1).toLowerCase(); keep existing blank/letters validation messages
intact.
| @Override | ||
| public boolean matchCredentials(String name, String password) { | ||
| Optional<AccountDTO> account = getAccountByName(name); | ||
|
|
||
| return account.isPresent() && account.get().password().equals(password); | ||
| } |
There was a problem hiding this comment.
Plain-text password comparison is a security risk.
matchCredentials compares passwords using String.equals(), which implies passwords are stored in plain text. This is a significant security vulnerability. Passwords should be hashed (e.g., bcrypt, Argon2) and compared using a constant-time comparison to prevent timing attacks.
Consider using a password hashing library:
// Example with BCrypt (add dependency: org.mindrot:jbcrypt)
return account.isPresent() &&
BCrypt.checkpw(password, account.get().password());🤖 Prompt for AI Agents
In src/main/java/com/repo/AccountRepository.java around lines 65 to 70, the
method matchCredentials currently compares plaintext passwords with
String.equals which implies plaintext storage and is vulnerable to timing
attacks; update authentication to verify a stored hashed password instead:
ensure account passwords are stored as a secure hash (e.g., bcrypt or Argon2)
when accounts are created/updated, add a vetted hashing library dependency (for
example org.mindrot:jbcrypt or an Argon2 library), and replace the equals check
with the library's verify function (e.g., BCrypt.checkpw or Argon2 verify) which
performs safe comparison; also remove any code paths that store raw passwords
and add unit tests for both successful and failed authentication cases.
| try(Connection c = dataSource.getConnection(); | ||
| PreparedStatement ps = c.prepareStatement("select * from moon_mission where missionid = ?")){ | ||
| ps.setInt(1, missionid); |
There was a problem hiding this comment.
Column name mismatch will cause query to fail.
The query uses missionid but mapMission() at line 75 reads mission_id. This inconsistency will cause the query to return no results or fail, depending on the actual database schema.
- PreparedStatement ps = c.prepareStatement("select * from moon_mission where missionid = ?")){
+ PreparedStatement ps = c.prepareStatement("select * from moon_mission where mission_id = ?")){📝 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.
| try(Connection c = dataSource.getConnection(); | |
| PreparedStatement ps = c.prepareStatement("select * from moon_mission where missionid = ?")){ | |
| ps.setInt(1, missionid); | |
| try(Connection c = dataSource.getConnection(); | |
| PreparedStatement ps = c.prepareStatement("select * from moon_mission where mission_id = ?")){ | |
| ps.setInt(1, missionid); |
🤖 Prompt for AI Agents
In src/main/java/com/repo/MoonMissionRepository.java around lines 41 to 43, the
SQL uses "missionid" while mapMission() expects "mission_id", so update the code
to use the correct column name to match the mapper; change the prepared
statement SQL to use "where mission_id = ?" (or alternatively update mapMission
to read "missionid" if the DB actually uses that name), and keep the parameter
binding as ps.setInt(1, missionid) so the query and mapper use the same column
name.
| private MissionDTO mapMission(ResultSet rs) throws SQLException { | ||
| return new MissionDTO( | ||
| rs.getInt("mission_id"), | ||
| rs.getString("spacecraft"), | ||
| rs.getDate("launch_date").toLocalDate(), | ||
| rs.getString("carrier_rocket"), | ||
| rs.getString("operator"), | ||
| rs.getString("mission_type"), | ||
| rs.getString("outcome") | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException if launch_date is null.
If launch_date is NULL in the database, rs.getDate("launch_date") returns null, and calling .toLocalDate() on it will throw NPE.
private MissionDTO mapMission(ResultSet rs) throws SQLException {
+ java.sql.Date launchDate = rs.getDate("launch_date");
return new MissionDTO(
rs.getInt("mission_id"),
rs.getString("spacecraft"),
- rs.getDate("launch_date").toLocalDate(),
+ launchDate != null ? launchDate.toLocalDate() : null,
rs.getString("carrier_rocket"),
rs.getString("operator"),
rs.getString("mission_type"),
rs.getString("outcome")
);
}📝 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.
| private MissionDTO mapMission(ResultSet rs) throws SQLException { | |
| return new MissionDTO( | |
| rs.getInt("mission_id"), | |
| rs.getString("spacecraft"), | |
| rs.getDate("launch_date").toLocalDate(), | |
| rs.getString("carrier_rocket"), | |
| rs.getString("operator"), | |
| rs.getString("mission_type"), | |
| rs.getString("outcome") | |
| ); | |
| } | |
| private MissionDTO mapMission(ResultSet rs) throws SQLException { | |
| java.sql.Date launchDate = rs.getDate("launch_date"); | |
| return new MissionDTO( | |
| rs.getInt("mission_id"), | |
| rs.getString("spacecraft"), | |
| launchDate != null ? launchDate.toLocalDate() : null, | |
| rs.getString("carrier_rocket"), | |
| rs.getString("operator"), | |
| rs.getString("mission_type"), | |
| rs.getString("outcome") | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/repo/MoonMissionRepository.java around lines 73 to 83, the
code calls rs.getDate("launch_date").toLocalDate() which will throw a
NullPointerException if the DB value is NULL; change the mapping to retrieve the
Date into a local variable, check for null, and pass either null or an
Optional/localDate to the MissionDTO (e.g., Date d = rs.getDate("launch_date");
LocalDate launchDate = (d == null) ? null : d.toLocalDate()), and ensure
MissionDTO's launchDate field/signature accepts null (or an Optional)
accordingly.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.