refactor: replace boilerplate with Lombok annotations #1032
Conversation
|
@kuntal1461 is attempting to deploy a commit to the Saptarshi Sarkar's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
App 💻
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Yeah! You did it 🎉 Now, Relax 😉, Grab a drink ☕, and wait for the maintainers to check your contributions. Meanwhile, you can discuss on other issues and solve them 😀. Thank You 😃!
Meanwhile you can also discuss about the project in our Discord Server 😀
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Lombok as a static dependency and refactors several classes to use Lombok annotations for logging and boilerplate reduction. Updates control flow in FileDownloader for process handling (merged streams, timeout, interruption handling) and refines job list management with duplicate detection and targeted removals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as GUI.FileDownloader
participant PB as ProcessBuilder
participant Proc as Process
participant OS as OS
User->>UI: downloadYoutubeOrInstagram(...)
UI->>PB: configure command + redirectErrorStream(true)
PB-->>UI: built ProcessBuilder
UI->>PB: start()
alt start fails
PB--x UI: IOException
UI->>UI: handleStartFailure()\n(set exitCode=1, map error)
UI-->>User: failure message
else started
PB-->>Proc: Process
par read merged stdout
UI->>Proc: read output
opt thread interrupted
UI->>Proc: destroy()
end
and wait with timeout
UI->>Proc: waitFor(30 min)
alt timeout
UI->>Proc: destroyForcibly()
UI->>UI: exitCode=non-zero, timeout message
else completed
Proc-->>UI: exitValue()
end
end
alt exitCode == 0
UI->>UI: optional Spotify conversion
UI-->>User: success
else
UI-->>User: failure (exitCode != 0)
end
end
sequenceDiagram
autonumber
participant Client
participant Jobs as support.Jobs
participant Job as support.Job
Client->>Jobs: add(Job)
alt duplicate (matches existing)
Jobs->>Jobs: log debug "Duplicate job ignored"
else new
Jobs->>Jobs: add to list
end
Client->>Jobs: remove(link)
Jobs->>Jobs: find by matchesLink(link)
alt found
Jobs->>Jobs: remove found job
Jobs->>Jobs: log "Removed job"
else not found
Jobs->>Jobs: log "No matching job..."
end
Client->>Jobs: getList()
Jobs-->>Client: new ConcurrentLinkedDeque(copy)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
kuntal1461
force-pushed
the
feat/lombok-refactor-1027
branch
from
8e58bc6 to
6aab835
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Core/src/main/java/preferences/Clear.java (1)
19-22: Consider protected access to enable GUI subclass logging.The private helper method successfully centralizes preference removal and adds debug logging. However,
GUI/src/main/java/gui/preferences/Clear.javaextends this class and directly callspreferences.remove()in its own methods (lines 11, 15, 19), bypassing this logging. Makingremove(String key)protected would allow the GUI subclass to leverage the same logging behavior.Apply this diff to change the access level:
- private void remove(String key) { + protected void remove(String key) { preferences.remove(key); log.debug("Cleared preference: {}", key); }Then update
GUI/src/main/java/gui/preferences/Clear.javato use the parent method:public void folders() { remove(FOLDERS.toString()); } public void mainAutoPaste() { remove(MAIN_AUTO_PASTE.toString()); } public void mainTheme() { remove(MAIN_THEME.toString()); }Core/src/main/java/support/Job.java (1)
10-10: Consider the necessity of bothgetLink()andgetSourceLink().With
@Getterat the class level, Lombok generatesgetLink()for thelinkfield, but the explicitgetSourceLink()method also returnslink. This creates two methods accessing the same field.This might be intentional for backward compatibility. If so, consider:
- Documenting that
getSourceLink()is the preferred public API- Or deprecating
getLink()by using@Getter(onMethod_ = @Deprecated)on the field- Or renaming the field to
sourceLinkso Lombok generatesgetSourceLink()directlyIf backward compatibility is the goal, you can prevent Lombok from generating
getLink():+ @Getter(AccessLevel.NONE) + @EqualsAndHashCode.Include private final String link;This keeps only
getSourceLink()in the public API.Also applies to: 37-39
GUI/src/main/java/backend/FileDownloader.java (2)
153-160: Extract timeout duration as a named constant.The 30-minute timeout is hardcoded with a "tune as needed" comment, suggesting it may require adjustment. Extract it as a class-level constant for better maintainability and discoverability.
Apply this diff to extract the timeout as a constant:
+ private static final int DOWNLOAD_TIMEOUT_MINUTES = 30; + private void downloadYoutubeOrInstagram(boolean isSpotifySong){ String[] fullCommand = new String[]{YT_DLP, "--quiet", "--progress", "-P", dir, downloadLink, "-o", filename, "-f", (isSpotifySong ? "bestaudio" : "mp4")}; ProcessBuilder processBuilder = new ProcessBuilder(fullCommand) .redirectErrorStream(true);//merge stderr -> stdout to avoid deadlocks sendInfoMessage(String.format(DOWNLOADING_F, filename)); final Process process ; try { process = processBuilder.start(); } catch (IOException e) { M.msgDownloadError("Failed to start download process for \"" + filename + "\""); exitCode = 1; return; } catch (Exception e) { handleStartFailure(e); exitCode = 1; return; } try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) { String line; while ((line = reader.readLine()) != null) { if (Thread.currentThread().isInterrupted()) { process.destroyForcibly(); throw new InterruptedException("Interrupted while reading download progress"); } progressProperty.setValue(line); } // Wait with timeout to avoid hanging forever (tune as needed) - boolean finished = process.waitFor(30, java.util.concurrent.TimeUnit.MINUTES); + boolean finished = process.waitFor(DOWNLOAD_TIMEOUT_MINUTES, java.util.concurrent.TimeUnit.MINUTES); if (!finished) { process.destroyForcibly(); M.msgDownloadError("Download timed out for \"" + filename + "\""); exitCode = 1; return; }
184-199: Consider more robust error message parsing.The exception message parsing relies on specific string formats and uses
toLowerCase().trim().replace(" ", "")for normalization. While this is consistent with the CLI version's approach, it's fragile and could miss edge cases (e.g., tabs, multiple spaces, format changes).Consider using Pattern matching or more structured error detection:
private void handleStartFailure(Exception e) { - String msg = e.getMessage() != null ? e.getMessage() : ""; - String[] parts = msg.split(","); - String p0 = parts.length > 0 ? parts[0] : ""; - String p1 = parts.length > 1 ? parts[1] : ""; - - if (p0.toLowerCase().replace(" ", "").contains("cannotrunprogram")) { + String msg = e.getMessage(); + if (msg == null) { + M.msgDownloadError("An Unknown Error occurred! " + e.getClass().getSimpleName()); + return; + } + + String normalizedMsg = msg.toLowerCase().replaceAll("\\s+", ""); + + if (normalizedMsg.contains("cannotrunprogram")) { M.msgDownloadError(DRIFTY_COMPONENT_NOT_EXECUTABLE_ERROR); - } else if ("permissiondenied".equalsIgnoreCase(p1.trim().replace(" ", ""))) { + } else if (normalizedMsg.contains("permissiondenied")) { M.msgDownloadError(PERMISSION_DENIED_ERROR); - } else if ("videounavailable".equalsIgnoreCase(p0.trim().replace(" ", ""))) { + } else if (normalizedMsg.contains("videounavailable")) { M.msgDownloadError(VIDEO_UNAVAILABLE_ERROR); } else { M.msgDownloadError("An Unknown Error occurred! " + e.getMessage()); } }This approach:
- Uses
replaceAll("\\s+", "")to handle all whitespace consistently- Simplifies the logic by using
contains()instead of splitting and indexing- Provides a clearer fallback for null messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Core/pom.xmlis excluded by!**/*.xmlpom.xmlis excluded by!**/*.xml
📒 Files selected for processing (7)
Core/src/main/java/module-info.java(1 hunks)Core/src/main/java/preferences/Clear.java(1 hunks)Core/src/main/java/preferences/Get.java(4 hunks)Core/src/main/java/support/DownloadMetrics.java(1 hunks)Core/src/main/java/support/Job.java(3 hunks)Core/src/main/java/support/Jobs.java(1 hunks)GUI/src/main/java/backend/FileDownloader.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
Core/src/main/java/support/Jobs.java (2)
Core/src/main/java/preferences/Get.java (1)
Slf4j(22-134)Core/src/main/java/support/JobHistory.java (3)
clear(25-27)JobHistory(5-46)JobHistory(8-10)
Core/src/main/java/preferences/Get.java (2)
Core/src/main/java/preferences/Clear.java (1)
Slf4j(9-75)Core/src/main/java/support/Jobs.java (1)
Slf4j(8-54)
Core/src/main/java/preferences/Clear.java (3)
Core/src/main/java/preferences/Get.java (1)
Slf4j(22-134)Core/src/main/java/support/Jobs.java (1)
Slf4j(8-54)GUI/src/main/java/gui/preferences/Clear.java (2)
Clear(7-26)folders(15-17)
GUI/src/main/java/backend/FileDownloader.java (2)
Core/src/main/java/support/Constants.java (1)
Constants(10-48)CLI/src/main/java/backend/FileDownloader.java (2)
downloadYoutubeOrInstagram(143-211)FileDownloader(31-302)
Core/src/main/java/module-info.java (2)
CLI/src/main/java/module-info.java (1)
CLI(1-7)GUI/src/main/java/module-info.java (1)
GUI(1-20)
Core/src/main/java/support/DownloadMetrics.java (1)
CLI/src/main/java/backend/ProgressBarThread.java (1)
ProgressBarThread(17-230)
Core/src/main/java/support/Job.java (1)
Core/src/main/java/support/JobHistory.java (1)
JobHistory(5-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Build [macos-13, CLI]
- GitHub Check: Build [macos-13, GUI]
- GitHub Check: Build [windows-latest, CLI]
- GitHub Check: Build [macos-14, GUI]
- GitHub Check: Build [macos-14, CLI]
- GitHub Check: Build [ubuntu-latest, CLI]
- GitHub Check: Build [windows-latest, GUI]
- GitHub Check: Build [ubuntu-latest, GUI]
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Dependency Review
- GitHub Check: Lint Code Base
- GitHub Check: Build and Scan [drifty-gui, ubuntu-latest]
- GitHub Check: Build and Scan [drifty-gui, windows-latest]
- GitHub Check: Build and Scan [drifty-gui, macos-14]
- GitHub Check: Build and Scan [drifty-cli, macos-14]
- GitHub Check: Build and Scan [drifty-gui, macos-13]
- GitHub Check: Build and Scan [drifty-cli, windows-latest]
- GitHub Check: Build and Scan [drifty-cli, ubuntu-latest]
- GitHub Check: Build and Scan [drifty-cli, macos-13]
- GitHub Check: build (GUI, drifty-gui)
- GitHub Check: build (CLI, drifty-cli)
🔇 Additional comments (17)
Core/src/main/java/preferences/Clear.java (2)
5-5: LGTM! Consistent logging integration.The
@Slf4jannotation is correctly applied and aligns with the project-wide Lombok adoption seen inGet.javaandJobs.java.Also applies to: 9-9
24-74: LGTM! Effective DRY refactor with operational visibility.All public methods now delegate to the centralized
remove()helper, eliminating code duplication and providing consistent debug logging for preference clearing operations. The DEBUG log level is appropriate for this operational context.Core/src/main/java/module-info.java (1)
11-11: LGTM!Correctly declares Lombok as a static (compile-time only) dependency, following best practices. The module will require Lombok for annotation processing during compilation but not at runtime.
Core/src/main/java/preferences/Get.java (2)
4-4: LGTM!The
@Slf4jannotation is properly imported and applied, enabling Lombok-generated logging throughout the class.Also applies to: 22-22
50-50: LGTM!Error logging has been appropriately added to SQLException handlers. The log statements use parameterized logging (avoiding string concatenation) and preserve the exception stack trace by passing the exception as the second parameter.
Also applies to: 128-128
Core/src/main/java/support/Job.java (3)
10-12: LGTM! Verify equals/hashCode behavior change.The Lombok annotations are correctly applied. However, note that
@EqualsAndHashCode(onlyExplicitlyIncluded = true)with onlylink,dir, andfilenameincluded means thatdownloadLinkis now excluded from equality checks. This is a behavioral change:
- Before: All fields (including
downloadLink) likely participated inequals()andhashCode()- After: Only
link,dir, andfilenameare usedThis means two
Jobinstances with the same link/dir/filename but differentdownloadLinkvalues will now be considered equal. Verify this is the intended behavior, especially in collections likeJobs.jobListwhere duplicate detection relies onequals().Based on learnings: Lombok's
@EqualsAndHashCode(onlyExplicitlyIncluded = true)only uses explicitly marked fields, which is correct here. However, ensure this behavioral change is intentional and documented.Also applies to: 18-25
42-45: LGTM!The early-return style simplifies the conditional logic and improves readability. The behavior is preserved: returns
downloadLinkif non-null, falls back tolinkif non-null, otherwise throwsIllegalStateException.
33-35: Good addition!The
matchesLink(String link)overload provides a convenient way to check link equality without constructing aJobobject. This is used effectively inJobs.remove()andJobHistoryclasses.Core/src/main/java/support/Jobs.java (3)
3-4: LGTM!Lombok annotations are correctly applied:
@Slf4jprovides logging capability used throughout the class@NoArgsConstructorgenerates the no-args constructor, replacing the explicit oneBased on learnings: Lombok 1.18.42 allows configurable access levels for
@Logannotations, but the defaultprivatelogger is appropriate here.Also applies to: 8-9
18-26: LGTM! Duplicate detection now includes debug logging.The duplicate check now logs when a duplicate job is detected and ignored. This is helpful for debugging but doesn't change the core behavior—duplicates are still prevented.
Note: The duplicate detection relies on
Job.equals(), which (after the Lombok refactor in Job.java) only compareslink,dir, andfilenamefields, excludingdownloadLink. Ensure this is the intended comparison criteria.
28-37: LGTM! Improved logging and explicit removal.The method now:
- Finds the matching job using
matchesLink()- Actually removes it from the list (line 31)
- Logs the removal (line 32)
- Logs when no match is found (line 36)
The explicit
jobList.remove(job)on line 31 ensures the job is actually removed, and the logging provides good observability for debugging.Optional: Consider thread-safety of the find-and-remove pattern
While
ConcurrentLinkedDequeprovides thread-safe individual operations, the pattern of iterating to find a match and then removing it is not atomic. If another thread removes the same job between thematchesLink()check and theremove()call, theremove()may fail silently (if the deque'sremove()returns false when not found).This is likely not a new issue (existed before this refactor), but consider using
removeIf()for atomicity:public void remove(Job oldJob) { boolean removed = jobList.removeIf(job -> job.matchesLink(oldJob)); if (removed) { log.debug("Removed job: {}", oldJob); } else { log.debug("No matching job found to remove: {}", oldJob); } }However, this changes the logging slightly since you won't have the actual removed
jobinstance to log—onlyoldJob. Evaluate based on your concurrency requirements.GUI/src/main/java/backend/FileDownloader.java (6)
41-49: LGTM! Consistent use of centralized error constants.The static imports align with the project's pattern of using centralized constants from
support.Constants, promoting consistency across error messaging.
129-130: Good addition to prevent potential deadlocks.Merging stderr into stdout prevents blocking when the stderr buffer fills while the code only reads from stdout. This is the correct approach when programmatically consuming process output.
147-150: LGTM! Proper interruption handling.The interruption check enables responsive cancellation and prevents zombie processes by forcibly destroying the process before propagating the interruption.
165-171: LGTM! Correct interrupt handling pattern.Properly restores the interrupt status, destroys the process, and logs the error before returning. This follows Java concurrency best practices.
172-172: LGTM! Proper guard prevents conversion on failed downloads.The additional
exitCode == 0check ensures conversion is only attempted after successful downloads, avoiding unnecessary work and confusing error messages.
132-143: No changes required: the IOException catch handles checked failures and the Exception catch covers all runtime exceptions (SecurityException, NullPointerException, IndexOutOfBoundsException).
Signed-off-by: kuntal1461 <kuntal.1461@gmail.com>
Signed-off-by: kuntal1461 <kuntal.1461@gmail.com>
kuntal1461
force-pushed
the
feat/lombok-refactor-1027
branch
from
1f10f26 to
552e590
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Hi @kuntal1461, thanks for your contribution.
I've added review comments below. Some changes seem unrelated to the Lombok issue. Let's keep this PR focused.
| /* | ||
| * Last Modified : @kuntal1461 | ||
| */ |
There was a problem hiding this comment.
Git already tracks commit history, including timestamps and authorship. There's no need for manual Last modified: comments.
| /* | |
| * Last Modified : @kuntal1461 | |
| */ |
| /* | ||
| * last ModifiedBy : @kuntal1461 | ||
| */ |
There was a problem hiding this comment.
Git already tracks commit history, including timestamps and authorship. There's no need for manual Last modified: comments.
| /* | |
| * last ModifiedBy : @kuntal1461 | |
| */ |
| jobHistory.addJob(job, true); | ||
| } | ||
| } catch (SQLException e) { | ||
| log.error("Error fetching completed jobs", e); |
There was a problem hiding this comment.
Fix the indentation to solve linter warnings
| log.error("Error fetching completed jobs", e); | |
| log.error("Error fetching completed jobs", e); |
| jobs.add(job); | ||
| } | ||
| } catch (SQLException e) { | ||
| log.error("Error fetching queued jobs from the database", e); |
There was a problem hiding this comment.
Fix the indentation to solve linter warnings
| log.error("Error fetching queued jobs from the database", e); | |
| log.error("Error fetching queued jobs from the database", e); |
| if (downloadLink != null) return downloadLink; | ||
| if (link != null) return link; |
There was a problem hiding this comment.
Please consider using braces ({}) for these if-statements, to solve linter warnings.
| <maven.compiler.target>23</maven.compiler.target> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <project.shortVersionString>2.1.0</project.shortVersionString> <!-- for Windows MSI versioning --> | ||
| <lombok.version>1.18.40</lombok.version> |
There was a problem hiding this comment.
Please consider using the latest version of lombok: 1.18.42
| <lombok.version>1.18.40</lombok.version> | |
| <lombok.version>1.18.42</lombok.version> |
There was a problem hiding this comment.
@kuntal1461 The changes in this file seem unrelated to the issue at hand, which focuses specifically on replacing boilerplate code with Lombok. For clarity and maintainability, let's keep this PR scoped to that objective. Unrelated changes can be addressed in a separate PR if needed. Appreciate your understanding!
| @NoArgsConstructor | ||
| public class Jobs { | ||
| private ConcurrentLinkedDeque<Job> jobList; | ||
|
|
There was a problem hiding this comment.
Please consider removing this redundant newline.
| * last ModifiedBy : @kuntal1461 | ||
| */ | ||
| public class Job { | ||
|
|
There was a problem hiding this comment.
Please remove this redundant newline.
SaptarshiSarkar12
changed the title
|
Hi @SaptarshiSarkar12 Thanks for taking the time to review the code. I’ll incorporate your feedback and update the codebase accordingly. |
2 participants
Fixes issue
Fixes #1027
Changes proposed
@Getter,@Setter,@NoArgsConstructor,@AllArgsConstructor,@BuildertoString,equals, andhashCodeimplementations where appropriatepreferencesandsupportpackages to use LombokCheck List (Check all the applicable boxes)
Screenshots
(Not applicable for backend refactor)
Note to reviewers
This PR focuses purely on boilerplate reduction using Lombok annotations.
No business logic or functional behavior has been modified.
All changes compile and pass tests locally.