[audio] Refactored use of filename, filename extension and separator #3837
Conversation
|
Pointing to this discussion. I think we schould discuss the naming of the util class and wether we consider to move the declarations of the |
| @@ -0,0 +1,75 @@ | |||
| /** | |||
There was a problem hiding this comment.
Whats is the reason for this class? We have org.apache.commons.io already in the classpath which provides FilenameUtils. The methods are identical to these implemented here.
There was a problem hiding this comment.
@htreu thanks for your review. Have a look at this discussion. I suggested to use that util class, but we decided to be less dependant. That is the reason for my own implementation.
| case M3U_EXTENSION: | ||
| InputStream isM3U = new URL(url).openStream(); | ||
| String urlsM3U = IOUtils.toString(isM3U); | ||
| for (String line : urlsM3U.split("\n")) { |
There was a problem hiding this comment.
Line 65 & 66 might also be a call to IOUtils.readLines(is) which does provide a List<String> ready to use.
There was a problem hiding this comment.
Fair point. The use of IOUtils.toString(InputStream input) is deprecated since org.apache.commons.io v2.5, so we should adopt that anyway.
| case PLS_EXTENSION: | ||
| InputStream isPLS = new URL(url).openStream(); | ||
| String urlsPLS = IOUtils.toString(isPLS); | ||
| for (String line : urlsPLS.split("\n")) { |
There was a problem hiding this comment.
same here to read lines by IOUtils.readLines(is).
|
@htreu I applied the changes. |
| final String extension = AudioStreamUtils.getExtension(filename); | ||
| switch (extension) { | ||
| case M3U_EXTENSION: | ||
| final InputStream isM3U = new URL(url).openStream(); |
There was a problem hiding this comment.
what about closing the stream?
There was a problem hiding this comment.
Good point. I changed it.
There was a problem hiding this comment.
java.io.InputStream implements Closeable and so AutoCloseable.
Why not using try-with-resources?
There was a problem hiding this comment.
This design pattern is new to me, thanks for pointing to it. It looks like the best solution for this.
| } | ||
| break; | ||
| case PLS_EXTENSION: | ||
| final InputStream isPLS = new URL(url).openStream(); |
There was a problem hiding this comment.
what about closing the stream?
There was a problem hiding this comment.
Good point. I changed it. Maybe the try {} catch {} block is placed incorrect now and should be refactored / split up as well.
Signed-off-by: Christoph Weitkamp <[email protected]>
Signed-off-by: Christoph Weitkamp <[email protected]>
Signed-off-by: Christoph Weitkamp <[email protected]>
Signed-off-by: Christoph Weitkamp <[email protected]>
|
@maggu2810 thanks for review, I refactored the code to use the try-with-resources statement. |
| if (filename == null) { | ||
| return false; | ||
| } | ||
| if ((extension == null) || extension.isEmpty()) { |
There was a problem hiding this comment.
You could remove the braces around the first check.
Signed-off-by: Christoph Weitkamp <[email protected]>
|
@maggu2810 done. If you want me to squash my commits, please let me know. |
We can do it on merging. Thank you |
Signed-off-by: Christoph Weitkamp [email protected]