5
\$\begingroup\$

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:

Tutorial followed and used similar design

SHA256 hash generating code

\$\endgroup\$
2
  • 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
    Nov 6 '19 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
    Nov 6 '19 at 11:49
5
\$\begingroup\$

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")));
        }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.