Upload log improvements #707
Conversation
| @@ -277,10 +291,11 @@ private void uploadFileFromURL(String fileUrl) throws ExceptionImport, Exception | |||
| READ_TIMEOUT); | |||
| InputStream targetStream = new FileInputStream(testData); | |||
|
|
|||
| media = new MediaImpl(testData.getName(), targetStream, Charset.forName("UTF-8")); | |||
| media = new MediaImpl(testData.getName(), targetStream, Charset.forName("UTF-8"), | |||
There was a problem hiding this comment.
[Minor] Though existing code, in all files you changed in this commit, suggest utilising StandardCharsets.UTF_8.toString() constant rather than string literal "UTF-8". If the string literal is utilised in multiple places within the one file, can create local class constant e.g. private static final String UTF8_CHARSET = StandardCharsets.UTF_8.toString(); and reference that in the multiple local places. String literals/any type literal (which is magic) anywhere should be discouraged moving forwards (it's a bad code smell from existing codebase).
| Executions.getCurrent().sendRedirect("import-csv/csvimporter.zul", "_blank"); | ||
| break; | ||
|
|
||
| case MODAL: |
There was a problem hiding this comment.
[Suggestion & Minor] Is this an intentional fall-through to 'default' branch of switch statement? If so, and in any case, suggest adding a code comment under case MODAL mentioning this/reasoning on your switch MODAL with empty block conditional processing, as it aids in understanding the empty block which late at night may be counter-intuitive/hard to decipher if someone picks up this code, to refactor/understand. Or, just remove as it's a bit weird looking and in this case confusing (if you want to leave it there, please have a comment why you're leaving it in as mentioned first) Ref: https://rules.sonarsource.com/cpp/RSPEC-3458
There was a problem hiding this comment.
This part is legacy code, and Nolan asked the same question. I guess the intention was to create a view either in the same page or in a modal popup depends on case values. Probably it's safe to remove "case MODAL" to avoid confusion.
There was a problem hiding this comment.
Thanks, Frank. Agree - less confusing code is good code - regardless if there is history behind it, someone picking up the code shouldn't need to know that history ( and, it is definately safe to remove case MODAL ).
There was a problem hiding this comment.
Had a quick look - good functional changes, added a couple of code improvement suggestions, for maintenance/quality. Well done @nolantellis
| Executions.getCurrent().sendRedirect("import-csv/csvimporter.zul", "_blank"); | ||
| break; | ||
|
|
||
| case MODAL: |
There was a problem hiding this comment.
This part is legacy code, and Nolan asked the same question. I guess the intention was to create a view either in the same page or in a modal popup depends on case values. Probably it's safe to remove "case MODAL" to avoid confusion.
No description provided.