Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

I'm going to comment only on the coding aspects as I am not a cryptographer. The only thing I can say is that if you aren't one you shouldn't roll out your own algorithms.

/**
 * 
 */

What are these comments for? They don't convey any information, you should better delete them.

public static SHECipher getDefaultInstance(){

I'd suggest you to get rid of this factory method and replace it with dependency injection dependency injection. It might look innocuous, but with that method you're just introducing an hidden dependency in your system, which is going to cause you troubles if you ever want to swap it with another implementation or if you want to test your code, which you should.

public void init(Key key, IvParameterSpec iv) 

I'd get rid of the init method, and do all the initialisation in the constructor. Probably you want to decompose your class in two classes. A SHECiper, which represent cipher that is initialised and always in a valid state, and a SHECipherFactory that creates the instances you need and takes care of their initialisation. This would also allow you to make some of the fields of your class final, which is always a good thing.

public void reset()

Similary, you should get rid of reset too. If you don't need the cipher anymore just throw it away and create a new one instead.

Why do you keep code that is commented out? I'd delete all the zeroKey code. You're not using it anyway.

I'm going to comment only on the coding aspects as I am not a cryptographer. The only thing I can say is that if you aren't one you shouldn't roll out your own algorithms.

/**
 * 
 */

What are these comments for? They don't convey any information, you should better delete them.

public static SHECipher getDefaultInstance(){

I'd suggest you to get rid of this factory method and replace it with dependency injection. It might look innocuous, but with that method you're just introducing an hidden dependency in your system, which is going to cause you troubles if you ever want to swap it with another implementation or if you want to test your code, which you should.

public void init(Key key, IvParameterSpec iv) 

I'd get rid of the init method, and do all the initialisation in the constructor. Probably you want to decompose your class in two classes. A SHECiper, which represent cipher that is initialised and always in a valid state, and a SHECipherFactory that creates the instances you need and takes care of their initialisation. This would also allow you to make some of the fields of your class final, which is always a good thing.

public void reset()

Similary, you should get rid of reset too. If you don't need the cipher anymore just throw it away and create a new one instead.

Why do you keep code that is commented out? I'd delete all the zeroKey code. You're not using it anyway.

I'm going to comment only on the coding aspects as I am not a cryptographer. The only thing I can say is that if you aren't one you shouldn't roll out your own algorithms.

/**
 * 
 */

What are these comments for? They don't convey any information, you should better delete them.

public static SHECipher getDefaultInstance(){

I'd suggest you to get rid of this factory method and replace it with dependency injection. It might look innocuous, but with that method you're just introducing an hidden dependency in your system, which is going to cause you troubles if you ever want to swap it with another implementation or if you want to test your code, which you should.

public void init(Key key, IvParameterSpec iv) 

I'd get rid of the init method, and do all the initialisation in the constructor. Probably you want to decompose your class in two classes. A SHECiper, which represent cipher that is initialised and always in a valid state, and a SHECipherFactory that creates the instances you need and takes care of their initialisation. This would also allow you to make some of the fields of your class final, which is always a good thing.

public void reset()

Similary, you should get rid of reset too. If you don't need the cipher anymore just throw it away and create a new one instead.

Why do you keep code that is commented out? I'd delete all the zeroKey code. You're not using it anyway.

replaced http://security.stackexchange.com/ with https://security.stackexchange.com/
Source Link

I'm going to comment only on the coding aspects as I am not a cryptographer. The only thing I can say is that if you aren't one you shouldn't roll out your own algorithms you shouldn't roll out your own algorithms.

/**
 * 
 */

What are these comments for? They don't convey any information, you should better delete them.

public static SHECipher getDefaultInstance(){

I'd suggest you to get rid of this factory method and replace it with dependency injection. It might look innocuous, but with that method you're just introducing an hidden dependency in your system, which is going to cause you troubles if you ever want to swap it with another implementation or if you want to test your code, which you should.

public void init(Key key, IvParameterSpec iv) 

I'd get rid of the init method, and do all the initialisation in the constructor. Probably you want to decompose your class in two classes. A SHECiper, which represent cipher that is initialised and always in a valid state, and a SHECipherFactory that creates the instances you need and takes care of their initialisation. This would also allow you to make some of the fields of your class final, which is always a good thing.

public void reset()

Similary, you should get rid of reset too. If you don't need the cipher anymore just throw it away and create a new one instead.

Why do you keep code that is commented out? I'd delete all the zeroKey code. You're not using it anyway.

I'm going to comment only on the coding aspects as I am not a cryptographer. The only thing I can say is that if you aren't one you shouldn't roll out your own algorithms.

/**
 * 
 */

What are these comments for? They don't convey any information, you should better delete them.

public static SHECipher getDefaultInstance(){

I'd suggest you to get rid of this factory method and replace it with dependency injection. It might look innocuous, but with that method you're just introducing an hidden dependency in your system, which is going to cause you troubles if you ever want to swap it with another implementation or if you want to test your code, which you should.

public void init(Key key, IvParameterSpec iv) 

I'd get rid of the init method, and do all the initialisation in the constructor. Probably you want to decompose your class in two classes. A SHECiper, which represent cipher that is initialised and always in a valid state, and a SHECipherFactory that creates the instances you need and takes care of their initialisation. This would also allow you to make some of the fields of your class final, which is always a good thing.

public void reset()

Similary, you should get rid of reset too. If you don't need the cipher anymore just throw it away and create a new one instead.

Why do you keep code that is commented out? I'd delete all the zeroKey code. You're not using it anyway.

I'm going to comment only on the coding aspects as I am not a cryptographer. The only thing I can say is that if you aren't one you shouldn't roll out your own algorithms.

/**
 * 
 */

What are these comments for? They don't convey any information, you should better delete them.

public static SHECipher getDefaultInstance(){

I'd suggest you to get rid of this factory method and replace it with dependency injection. It might look innocuous, but with that method you're just introducing an hidden dependency in your system, which is going to cause you troubles if you ever want to swap it with another implementation or if you want to test your code, which you should.

public void init(Key key, IvParameterSpec iv) 

I'd get rid of the init method, and do all the initialisation in the constructor. Probably you want to decompose your class in two classes. A SHECiper, which represent cipher that is initialised and always in a valid state, and a SHECipherFactory that creates the instances you need and takes care of their initialisation. This would also allow you to make some of the fields of your class final, which is always a good thing.

public void reset()

Similary, you should get rid of reset too. If you don't need the cipher anymore just throw it away and create a new one instead.

Why do you keep code that is commented out? I'd delete all the zeroKey code. You're not using it anyway.

Source Link
mariosangiorgio
  • 2.7k
  • 1
  • 15
  • 19

I'm going to comment only on the coding aspects as I am not a cryptographer. The only thing I can say is that if you aren't one you shouldn't roll out your own algorithms.

/**
 * 
 */

What are these comments for? They don't convey any information, you should better delete them.

public static SHECipher getDefaultInstance(){

I'd suggest you to get rid of this factory method and replace it with dependency injection. It might look innocuous, but with that method you're just introducing an hidden dependency in your system, which is going to cause you troubles if you ever want to swap it with another implementation or if you want to test your code, which you should.

public void init(Key key, IvParameterSpec iv) 

I'd get rid of the init method, and do all the initialisation in the constructor. Probably you want to decompose your class in two classes. A SHECiper, which represent cipher that is initialised and always in a valid state, and a SHECipherFactory that creates the instances you need and takes care of their initialisation. This would also allow you to make some of the fields of your class final, which is always a good thing.

public void reset()

Similary, you should get rid of reset too. If you don't need the cipher anymore just throw it away and create a new one instead.

Why do you keep code that is commented out? I'd delete all the zeroKey code. You're not using it anyway.

lang-java

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