I have recently learnt SOLID patterns and started practicing them. I did follow all the guidelines but was thinking for a final review from experts if possible.
What is the code about ?
It encrypts any given string and outputs a hash.
Code
interface IHash
{
string Hash();
}
// Abstract class implements interface
public abstract class Encrypt : IHash
{
protected string _toBeHashed { get; private set; }
public Encrypt(string toBeHashed)
{
_toBeHashed = toBeHashed;
}
public abstract string Hash();
}
// Encrypt using BCrypt.NET library
public class BcryptEncrypt : Encrypt
{
// Store string value
public BcryptEncrypt(string password) : base(password)
{
}
// Output hash
public override string Hash() => BCrypt.Net.BCrypt.HashPassword(this._toBeHashed);
}
// Encrypt using SHA256
public class SHA256Encrypt : Encrypt
{
// Store string value
public SHA256Encrypt(string password) : base(password)
{
}
// Output hash
public override string Hash()
{
using (SHA256 hash = SHA256Managed.Create())
{
return String.Concat(hash.ComputeHash(Encoding.UTF8.GetBytes(this._toBeHashed)).Select(item => item.ToString("x2")));
}
}
}
My understanding
S - Single Responsibility Principle
- BcryptEncrypt only focuses on generating a hash for string
- It does not focus on decryption, storing or getting hash from database
- Same can be said for SHA256
O - Open/Closed Principle
- BcryptEncrypt is derived from abstract class Encrypt which implements interface IHash
- New hashing classes can be created using different hashing algorithms through abstract class
- Encryption of string is open for extension, closed for modification through abstract method
L - Liskov Substitution Principle
- Due to the usage of abstract method, this principle is already implemented
- Testing showed SHA256 hash to be same
I - Interface Segregation Principle
- Because there is only one method in interface, by default this principle is obeyed
D - Dependency Inversion Principle
- Not applicable, I think
Question: Am I doing it right ?
Attributions:
-
4\$\begingroup\$ "Is this code SOLID?" SOLID is not a binary state. It's a spectrum. Nothing every is perfectly SOLID, simply because (a) in fringe cases the principles start contradicting each other and/or (b) everything can always be abstracted one more level, but that doesn't mean it's useful. Try not to think of SOLID as a yes/no rule, but rather as a guideline that you implement to a reasonable degree. \$\endgroup\$Flater– Flater2019年11月06日 11:40:45 +00:00Commented Nov 6, 2019 at 11:40
-
4\$\begingroup\$ A sidenote: you are mixing the concepts of hashing and encryption. Hashing is a one-way, irreversible operation, mapping an input value to a fixed-length hash output. A small, even 1 char, change in input gives a big change in output. Encryption is a reversible process: You can encrypt something and decrypt the output. Both BCrypt and SHA256 that you use are hash functions. \$\endgroup\$TomG– TomG2019年11月06日 11:49:14 +00:00Commented Nov 6, 2019 at 11:49
1 Answer 1
D - Dependency Inversion Principle
Not applicable, I think
Classes should depend on abstractions and not concretions (paraphrased).
You have the interface and abstract class definition that can be used by consumers of your types. This allows consumers of your library the ability to apply DI.
That said, the code, as is, looks to be following the spirit of the principal.
My preference is to keep things simple (KISS).
Explicit Dependencies Principle
Methods and classes should explicitly require (typically through method parameters or constructor parameters) any collaborating objects they need in order to function correctly.
Which you also appear to be following.
If a type is to be used and reused externally you should put yourself in the shoes of those using your type and possible pain points.
If Encrypt
is to be used as a dependency
//cto
public SomeClass(Encrypt encrypt) {
//...
}
Some DI containers may have issue with activating implementations based on the string constructor argument. But I also see that as an implementation concern.
Code should also be intuitive. It should be genuine to its intent/responsibility
You stated
It encrypts any given string and outputs a hash.
The initial interface
interface IHash
{
string Hash();
}
could be implemented to hash not just strings.
Even the wording of the sentence can influence the design
//It encrypts any given string and outputs a hash.
public interface IEncrypt {
string Hash(string input);
}
Have a look at the following
// Abstract class implements interface
public abstract class Encrypt : IEncrypt
{
public abstract string Hash(string password);
}
// Encrypt using BCrypt.NET library
public class BcryptEncrypt : Encrypt
{
// Output hash
public override string Hash(string password) => BCrypt.Net.BCrypt.HashPassword(password);
}
// Encrypt using SHA256
public class SHA256Encrypt : Encrypt
{
// Output hash
public override string Hash(string password)
{
using (SHA256 hash = SHA256Managed.Create())
{
return String.Concat(hash.ComputeHash(Encoding.UTF8.GetBytes(password)).Select(item => item.ToString("x2")));
}
}
}