java-stream-player icon indicating copy to clipboard operation
java-stream-player copied to clipboard

Remove dead code implementing awaitTermination()

Open HelgeStenstrom opened this issue 6 years ago • 10 comments

As it was, the field future was always null, and therefore the method awaitTermination() didn't do anything. So I removed awaitTermination() and the future field.

They were not used, therefore they were not needed.

If something else is needed (if there is still a need, even if it wasn't fulfilled by the removed method), it's best to write a JUnit testcase that shows how the need is fulfilled by new code.

HelgeStenstrom avatar Sep 07 '19 10:09 HelgeStenstrom

If merged, this closes issue #2.

HelgeStenstrom avatar Sep 07 '19 10:09 HelgeStenstrom

Wait wait, are you sure about this one, when i wrote Future i used it because it was useful for a multithreading issue. Can you apply the change to your repo and check if the library works correctly with XR3Player, we might have issues in production after this.

goxr3plus avatar Sep 08 '19 20:09 goxr3plus

I specifically remember writing and using this method in XR3Player because i had issues

goxr3plus avatar Sep 08 '19 20:09 goxr3plus

But you might be right after all, let me think.

goxr3plus avatar Sep 08 '19 20:09 goxr3plus

O my gosh i am writing REACT js everyday and i have 1 million thinks in my mind, thanks for detecting important issues, i appreciate you very much ♥

goxr3plus avatar Sep 08 '19 20:09 goxr3plus

Rebased, to remove merge conflict.

HelgeStenstrom avatar Sep 08 '19 20:09 HelgeStenstrom

See for yourself.

Current master branch, line 657: https://github.com/goxr3plus/java-stream-player/blob/e50245f11518a91309ccefbcdc2a7d977a21b529/src/main/java/com/goxr3plus/streamplayer/stream/StreamPlayer.java#L657

But there is no place where future is assigned a value. The default value set on line 145 is null. https://github.com/goxr3plus/java-stream-player/blob/e50245f11518a91309ccefbcdc2a7d977a21b529/src/main/java/com/goxr3plus/streamplayer/stream/StreamPlayer.java#L145

So the code in the if-clause will not be executed, and there is nothing else in awaitTermination(). Since awaitTermination() does nothing, the call can be removed, as well as the method itself.

HelgeStenstrom avatar Sep 08 '19 20:09 HelgeStenstrom

I'm not the first one to see this. https://github.com/goxr3plus/java-stream-player/issues/2#issuecomment-471306539

HelgeStenstrom avatar Sep 08 '19 20:09 HelgeStenstrom

Yes you are right , i had a careful look now , it seems i added it because i wanted to implement something . I need to think , await termination has a role to play , i will remember .

goxr3plus avatar Sep 09 '19 12:09 goxr3plus

I suggest that you remove the dead code from the master branch now (merge the pull request), and then think about what it was that you wanted to accomplish, do some unit test for it, and then write the code to make them pass.

HelgeStenstrom avatar Sep 09 '19 16:09 HelgeStenstrom