[PB-5020]: Fix sonar issues v2 #1671
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Deploying drive-web with
|
| Latest commit: |
7c56fb0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1f65548d.drive-web.pages.dev |
| Branch Preview URL: | https://fix-sonar-issues-v2.drive-web.pages.dev |
terrerox
force-pushed
the
fix/sonar-issues-v2
branch
from
f4d9c8d to
da3eff8
Compare
terrerox
changed the title
src/app/network/DownloadManager.ts
Outdated
| private static readonly handleConnectionLostError = (err: unknown, taskId: string) => { | ||
| tasksService.updateTask({ | ||
| taskId, | ||
| merge: { status: TaskStatus.Error, subtitle: t('error.connectionLostError') as string }, |
There was a problem hiding this comment.
I think that the cast to string is not necessary, could remove it :)
src/app/network/DownloadManager.ts
Outdated
| throw err; | ||
| }; | ||
|
|
||
| private static readonly handleServerOrCancelledError = ( |
There was a problem hiding this comment.
this function name coud lead to confusion, the function returns boolean value, dependeing of certain conditions, maybe shouldContinueAfterError or something similar fits better with the purpose of the function
src/app/network/DownloadManager.ts
Outdated
| this.handleConnectionLostError(err, taskId); | ||
| } | ||
|
|
||
| const updateTaskWithErrorStatus = this.handleServerOrCancelledError( |
There was a problem hiding this comment.
same as the function name, this not fits boolean naming convention, seems more like a function name because it has an action on the name, something as shouldContinueProcessing fits better I think
- Rename handleServerOrCancelledError to shouldUpdateTaskWithError for clearer intent - Replace variable updateTaskWithErrorStatus with shouldContinueProcessing - Remove unnecessary type casting in handleConnectionLostError
CandelR
left a comment
CandelR
left a comment
There was a problem hiding this comment.
Remember to check the sonar cloud new issues
src/app/network/DownloadManager.ts
Outdated
| this.handleConnectionLostError(err, taskId); | ||
| } | ||
|
|
||
| const shouldContinueProcessing = this.shouldUpdateTaskWithError(downloadTask, isServerError, filePickerCancelled); |
There was a problem hiding this comment.
To me, the name of the function and what it does still doesn't make sense; it's not clear.
It's called shouldUpdateTaskWithError, and it returns true if it doesn't update it and false if it does.
I recommend adding jsdoc to the functions of this class, because I find it a bit confusing. It's not the best solution, but I don't think we need to refactor this in this task either
There was a problem hiding this comment.
Done! Do I need to wait for @internxt/cryptography ’s approval before sending this task to QA?
There was a problem hiding this comment.
Not necessary in this one, pass it to QA
Extracted task status updates and retry item removal logic into separate methods to improve code clarity and maintainability. The shouldContinueAfterError function now focuses solely on decision logic.
|
Description
Removed redundant cycles, reduced cognitive complexity, deleted an unused component and simplified nested code.
Related Issues
Related Pull Requests
Checklist
Testing Process
Additional Notes