Skip to main content
Commonmark migration
Source Link

A few small-ish things the I noticed (I should stop starting my answers with some sentence like this ...):

  • You're working on bare threads. That's usually considered a bad idea, because they don't clean up so easily. It's preferred to use ExecutorService and related classes. Interestingly you already do that for the prompt ...

  • You're always starting the thread when runFuturesConsumer and runKeyPressDaemon are called. This is bad, because it's not documented anywhere and I'd expect the method to keep track of whether what you try to run is already running and not start another instance when it's not needed.

  • You didn't post the code for cancelServerFuture but the naming suggests something unsettling. Cancelling usually means that there's no result, but your code implies there should be? Maybe?
    I don't see much benefit in cancelling the server computation and then transferring something to the server (short-circuit does that). Or maybe I'm just missing something ...

  • You're handling a user-cancel as a failure. I think that's misleading. A user cancellation is never a failure. Yet after the user cancelled the computation you display the message "failure caused by: stopped before the completion". This is not really the expected behaviour. Instead I'd expect either no message or something to the effect of "Successfully cancelled execution".

  • There's no code showing what exactly the Locks are that you're using, but judging by how they are used in the code, I'm not sure whether you actually need them in the first place and if you need them, whether you should lock that way ... From what I can make of the code, the only actually necessary lock should be the outputLock (which should also be used for the prompt)

  • The interruption process you use is something that's usually better known as POISON_PILL. At least that's the conventional name for it as I heard it.

    While we're on interruption: it's interesting that you compare INTERRUPTION_VALUE through Object#equals and the other cases through a reference equals (==). I'm not quite sure why that is, but alas ...

While we're on interruption: it's interesting that you compare INTERRUPTION_VALUE through Object#equals and the other cases through a reference equals (==). I'm not quite sure why that is, but alas ...

  • You're overcomplicating your handling of the prompt. Instead of calling handlePrompt (and effectively nesting a switch block there) you could just "inline" the execution there. Additional note there: "incorrect" user input isn't handled there. Passing in "asd" just blows up the code with an InputMismatchException :(

    You're overcomplicating your handling of the prompt. Instead of calling handlePrompt (and effectively nesting a switch block there) you could just "inline" the execution there. Additional note there: "incorrect" user input isn't handled there. Passing in "asd" just blows up the code with an InputMismatchException :(

Well this should cover most of it, I strongly recommend a follow-up review though :)

A few small-ish things the I noticed (I should stop starting my answers with some sentence like this ...):

  • You're working on bare threads. That's usually considered a bad idea, because they don't clean up so easily. It's preferred to use ExecutorService and related classes. Interestingly you already do that for the prompt ...

  • You're always starting the thread when runFuturesConsumer and runKeyPressDaemon are called. This is bad, because it's not documented anywhere and I'd expect the method to keep track of whether what you try to run is already running and not start another instance when it's not needed.

  • You didn't post the code for cancelServerFuture but the naming suggests something unsettling. Cancelling usually means that there's no result, but your code implies there should be? Maybe?
    I don't see much benefit in cancelling the server computation and then transferring something to the server (short-circuit does that). Or maybe I'm just missing something ...

  • You're handling a user-cancel as a failure. I think that's misleading. A user cancellation is never a failure. Yet after the user cancelled the computation you display the message "failure caused by: stopped before the completion". This is not really the expected behaviour. Instead I'd expect either no message or something to the effect of "Successfully cancelled execution".

  • There's no code showing what exactly the Locks are that you're using, but judging by how they are used in the code, I'm not sure whether you actually need them in the first place and if you need them, whether you should lock that way ... From what I can make of the code, the only actually necessary lock should be the outputLock (which should also be used for the prompt)

  • The interruption process you use is something that's usually better known as POISON_PILL. At least that's the conventional name for it as I heard it.

While we're on interruption: it's interesting that you compare INTERRUPTION_VALUE through Object#equals and the other cases through a reference equals (==). I'm not quite sure why that is, but alas ...

  • You're overcomplicating your handling of the prompt. Instead of calling handlePrompt (and effectively nesting a switch block there) you could just "inline" the execution there. Additional note there: "incorrect" user input isn't handled there. Passing in "asd" just blows up the code with an InputMismatchException :(

Well this should cover most of it, I strongly recommend a follow-up review though :)

A few small-ish things the I noticed (I should stop starting my answers with some sentence like this ...):

  • You're working on bare threads. That's usually considered a bad idea, because they don't clean up so easily. It's preferred to use ExecutorService and related classes. Interestingly you already do that for the prompt ...

  • You're always starting the thread when runFuturesConsumer and runKeyPressDaemon are called. This is bad, because it's not documented anywhere and I'd expect the method to keep track of whether what you try to run is already running and not start another instance when it's not needed.

  • You didn't post the code for cancelServerFuture but the naming suggests something unsettling. Cancelling usually means that there's no result, but your code implies there should be? Maybe?
    I don't see much benefit in cancelling the server computation and then transferring something to the server (short-circuit does that). Or maybe I'm just missing something ...

  • You're handling a user-cancel as a failure. I think that's misleading. A user cancellation is never a failure. Yet after the user cancelled the computation you display the message "failure caused by: stopped before the completion". This is not really the expected behaviour. Instead I'd expect either no message or something to the effect of "Successfully cancelled execution".

  • There's no code showing what exactly the Locks are that you're using, but judging by how they are used in the code, I'm not sure whether you actually need them in the first place and if you need them, whether you should lock that way ... From what I can make of the code, the only actually necessary lock should be the outputLock (which should also be used for the prompt)

  • The interruption process you use is something that's usually better known as POISON_PILL. At least that's the conventional name for it as I heard it.

    While we're on interruption: it's interesting that you compare INTERRUPTION_VALUE through Object#equals and the other cases through a reference equals (==). I'm not quite sure why that is, but alas ...

  • You're overcomplicating your handling of the prompt. Instead of calling handlePrompt (and effectively nesting a switch block there) you could just "inline" the execution there. Additional note there: "incorrect" user input isn't handled there. Passing in "asd" just blows up the code with an InputMismatchException :(

Well this should cover most of it, I strongly recommend a follow-up review though :)

Source Link
Vogel612
  • 25.3k
  • 7
  • 58
  • 141

A few small-ish things the I noticed (I should stop starting my answers with some sentence like this ...):

  • You're working on bare threads. That's usually considered a bad idea, because they don't clean up so easily. It's preferred to use ExecutorService and related classes. Interestingly you already do that for the prompt ...

  • You're always starting the thread when runFuturesConsumer and runKeyPressDaemon are called. This is bad, because it's not documented anywhere and I'd expect the method to keep track of whether what you try to run is already running and not start another instance when it's not needed.

  • You didn't post the code for cancelServerFuture but the naming suggests something unsettling. Cancelling usually means that there's no result, but your code implies there should be? Maybe?
    I don't see much benefit in cancelling the server computation and then transferring something to the server (short-circuit does that). Or maybe I'm just missing something ...

  • You're handling a user-cancel as a failure. I think that's misleading. A user cancellation is never a failure. Yet after the user cancelled the computation you display the message "failure caused by: stopped before the completion". This is not really the expected behaviour. Instead I'd expect either no message or something to the effect of "Successfully cancelled execution".

  • There's no code showing what exactly the Locks are that you're using, but judging by how they are used in the code, I'm not sure whether you actually need them in the first place and if you need them, whether you should lock that way ... From what I can make of the code, the only actually necessary lock should be the outputLock (which should also be used for the prompt)

  • The interruption process you use is something that's usually better known as POISON_PILL. At least that's the conventional name for it as I heard it.

While we're on interruption: it's interesting that you compare INTERRUPTION_VALUE through Object#equals and the other cases through a reference equals (==). I'm not quite sure why that is, but alas ...

  • You're overcomplicating your handling of the prompt. Instead of calling handlePrompt (and effectively nesting a switch block there) you could just "inline" the execution there. Additional note there: "incorrect" user input isn't handled there. Passing in "asd" just blows up the code with an InputMismatchException :(

Well this should cover most of it, I strongly recommend a follow-up review though :)