Skip to main content
Code Review

Return to Answer

replaced http://security.stackexchange.com/ with https://security.stackexchange.com/
Source Link
  • In general just using printStackTrace to handle exceptions is not good practice. If you don't care about handling it, just don't catch it.
  • return at the end of a method is unnecessary.
  • For string data using plain getBytes/the plain String constructor instead of specifying e.g. UTF-8 as the encoding seems more risky for no benefit. I guess the platform default is useful in some circumstances, but sending data over the network is not one of them.
  • The port number should be a constant.
  • The default empty constructor that doesn't set the port number seems not useful.
  • Streams' close methods should be called no matter what, that is, use try/finally to ensure closing resources.

Sending the data as a sequence terminated by -1 will break immediately once you send a general purpose file, instead send the length first or escape the offending pattern in all sent data.

Also it would be good to use a shared class for the shared methods.

Unfortunately without a deep look into the documentation I can't say anything about the crypto part. You might want to ask on the Information Security Information Security site as well. In any case, if you really want to implement this yourself maybe try to run each of the client and server against a second implementation using an established library and compare.

  • In general just using printStackTrace to handle exceptions is not good practice. If you don't care about handling it, just don't catch it.
  • return at the end of a method is unnecessary.
  • For string data using plain getBytes/the plain String constructor instead of specifying e.g. UTF-8 as the encoding seems more risky for no benefit. I guess the platform default is useful in some circumstances, but sending data over the network is not one of them.
  • The port number should be a constant.
  • The default empty constructor that doesn't set the port number seems not useful.
  • Streams' close methods should be called no matter what, that is, use try/finally to ensure closing resources.

Sending the data as a sequence terminated by -1 will break immediately once you send a general purpose file, instead send the length first or escape the offending pattern in all sent data.

Also it would be good to use a shared class for the shared methods.

Unfortunately without a deep look into the documentation I can't say anything about the crypto part. You might want to ask on the Information Security site as well. In any case, if you really want to implement this yourself maybe try to run each of the client and server against a second implementation using an established library and compare.

  • In general just using printStackTrace to handle exceptions is not good practice. If you don't care about handling it, just don't catch it.
  • return at the end of a method is unnecessary.
  • For string data using plain getBytes/the plain String constructor instead of specifying e.g. UTF-8 as the encoding seems more risky for no benefit. I guess the platform default is useful in some circumstances, but sending data over the network is not one of them.
  • The port number should be a constant.
  • The default empty constructor that doesn't set the port number seems not useful.
  • Streams' close methods should be called no matter what, that is, use try/finally to ensure closing resources.

Sending the data as a sequence terminated by -1 will break immediately once you send a general purpose file, instead send the length first or escape the offending pattern in all sent data.

Also it would be good to use a shared class for the shared methods.

Unfortunately without a deep look into the documentation I can't say anything about the crypto part. You might want to ask on the Information Security site as well. In any case, if you really want to implement this yourself maybe try to run each of the client and server against a second implementation using an established library and compare.

Source Link
ferada
  • 11.4k
  • 25
  • 65
  • In general just using printStackTrace to handle exceptions is not good practice. If you don't care about handling it, just don't catch it.
  • return at the end of a method is unnecessary.
  • For string data using plain getBytes/the plain String constructor instead of specifying e.g. UTF-8 as the encoding seems more risky for no benefit. I guess the platform default is useful in some circumstances, but sending data over the network is not one of them.
  • The port number should be a constant.
  • The default empty constructor that doesn't set the port number seems not useful.
  • Streams' close methods should be called no matter what, that is, use try/finally to ensure closing resources.

Sending the data as a sequence terminated by -1 will break immediately once you send a general purpose file, instead send the length first or escape the offending pattern in all sent data.

Also it would be good to use a shared class for the shared methods.

Unfortunately without a deep look into the documentation I can't say anything about the crypto part. You might want to ask on the Information Security site as well. In any case, if you really want to implement this yourself maybe try to run each of the client and server against a second implementation using an established library and compare.

lang-java

AltStyle によって変換されたページ (->オリジナル) /