6

I am getting confused on how exactly should I be using exception handling inside a class that respects S-Principle.

For example consider this code in C#:

public class BcryptDecrypt    
{    
    private string _password;
    private string _hash;

    public BcryptDecrypt(string password, string hash)
    {
      this._password = password;
      this._hash = hash;
    }

    public bool Verify()
    {
        return BCrypt.Net.BCrypt.Verify(this._password, this._hash);
    }    
}

Clearly it states that it decrypts a bcrypt hash. But you can see in the Verify method there is no try-catch block to handle an exception.

If we add a try-catch to handle exception then we cannot follow S-Principle. So how do software engineers solve this issue?

// After adding try-catch
public class BcryptDecrypt    
{    
    private string _password;
    private string _hash;

    public BcryptDecrypt(string password, string hash)
    {
      this._password = password;
      this._hash = hash;
    }

    public bool Verify()
    {
        try
        {
            return BCrypt.Net.BCrypt.Verify(this._toDecrypt, 
                                            this._hash);
        }
        catch (System.Exception SomeException)
        {
            // Handle exception as you like and break S-Principle
        }
    }    
}

One way I thought of is to make a separate class to handle each method or class exception or make a universal class to handle it.

I saw similar question on Stack Overflow, but the only given answer said to re-throw the exception to let the controller handle it. This doesn't sound like a solution to me considering the controller itself would be following the S-Principle.

  • 3
    In what way are you handling the exception? – JacquesB Nov 6 '19 at 9:57
  • 1
    OK, but you can handle exception in a way which doesn't break single-responsibility. It depends on what you actually do. Having a catch does not in itself break the SRP, but the code inside the catch could break it, depending on what the code actually does. In that case the solution is to not handle the exception at that level. – JacquesB Nov 6 '19 at 10:09
  • 3
    If you get an unknown exception, then you cannot meaningfully handle it. So the correct is not to catch it. Only catch exception you can meaningfully handle on the current level. – JacquesB Nov 6 '19 at 10:12
  • 2
    @JacquesB apart from perhaps in main where you log "oops I died" before exiting – Caleth Nov 6 '19 at 10:13
  • 2
    I wish people never heard about the SRP. It is singlehandedly the most misinterpreted rule of all. – Andy Nov 6 '19 at 21:18
13

Single responsibility ?

There is a fundamental misunderstanding about SRP here:

  • The single responsibility does not mean that a class should do only one thing, but that it should have only one reason to change.

  • In other words, it's not the single responsibility OF the class, but the single responsibility FOR the class to change. So it's about decision making.

  • Here an enlightening article from Uncle Bob, who invented SOLID, and explains it better than I with all his authority on the subject.

In consequence, you may very well do exception handling in that class without any design concern. You could even have a single Cypher class for encrypting, decrypting and autochecking itself, if you'd really want it.

Do one thing ?

In Clean Code, Uncle Bob also promotes the Do-one-thing-and-do-it-well, for functions. This could be questioned for exception handling:

  • Do one thing is not the same as do half of the thing
  • If something bad happens when you do the thing, and if you would not handle it as you should, then you would not do the thing well.
  • So exception handling does not infringe do-one-thing.

Your option 2 is therefore acceptable.

Anything about try/catch ?

The only remaining principle to consider for your function is Single Level of Abstraction Principle (SLAP):

  • if you try on some high level function, you should catch and do some high level function.
  • It would not be sound to have a catch clause that would be very low level and detailed, like two page of code for the catch vs 1 line for the try.

If exception handling gets too complex, here a nice pragmatic advice:

It is better to extract the bodies of the try and catch blocks out into functions of their own.

  • 4
    The "Reason to change" description is equally if not more confusing and is not a clarification. Uncle Bob clearly did a terrible job of explaining it because there is so much confusion on this subject. – whatsisname Nov 7 '19 at 5:07
10

Don't catch exceptions as soon as they are thrown. Catch exceptions at the point you can do something meaningful with them.

But your example code shouldn't be throwing there. The only reasonable exception is ArgumentNullException, which you should throw in the constructor.

public class BcryptDecrypt    
{    
    private string _password;
    private string _hash;

    public BcryptDecrypt(string password, string hash)
    {
      Ensure.ArgumentNotNull(password, nameof(password));
      Ensure.ArgumentNotNull(hash, nameof(hash));

      this._password = password;
      this._hash = hash;
    }

    public bool Verify()
    {
        return BCrypt.Net.BCrypt.Verify(this._password, this._hash);
    }    
}

Ensure.ArgumentNotNull from here.

  • But how would that handle the exception ? If I use if-else block to check and return on null then it would mean I am breaking S-Principle as the class is meant to decrypt hash only. – Suleman Nov 6 '19 at 9:53
  • @Suleman It wouldn't. Somewhere closer to the user would, where you have the context to know what to do. – Caleth Nov 6 '19 at 9:56
  • I see, so basically we handle the possible exceptions at the point of their possible existence. But then this means I actually remove the try-catch block ? Wouldn't it mean running a code without handling unknown exceptions ? – Suleman Nov 6 '19 at 10:09
  • 2
    @Suleman Yes. By definition you shouldn't handle unknown exceptions, because you don't know what to do. – Caleth Nov 6 '19 at 10:14
  • 1
    @Suleman Your method also becomes more reusable if you let the caller decide how to handle exceptions. – user560822 Nov 6 '19 at 18:50
4

You didn’t understand the Single Responsibility Principle.

It means: Do one thing - completely. With everything that is needed to make it work. Like exception handling. Adding another class for exception handling would be utterly ridiculous.

A chef cooking a lobster takes a pot, fills it with water, puts it on the oven, turns the heat on, waits until the water is boiling, throws in the lobster, and so on. SRP does not say that you need a PotTakerClass, a PotWithWaterFillerClass, a PotOnOvenPutterClass and another dozen classes.

SRP doesn’t even say you need a LobsterCookerClass. All you need is one ChefClass with the single responsibility to cook food. PS. A good chef is like god, and there's nothing wrong with that.

  • 3
    I don't think you understand SRP either. It doesn't say anything like "Do one thing - completely." – JacquesB Nov 6 '19 at 14:11
  • 1
    You're describing a god class, and it's considered harmful. – user560822 Nov 6 '19 at 18:52
  • @JacquesB I'm not reciting any definition. OP has probably seen a definition and completey misinterpreted it. – gnasher729 Nov 6 '19 at 21:15
  • I'm not opposed to this way of thinking about it provided you understand that "things" can nest. Your "one thing" should be one abstraction. One simple idea. Not something that can't be broken down into smaller things. That story is different for functions. – candied_orange Nov 7 '19 at 0:21
  • @JacquesB: the catch is the understanding of level of abstraction. The Lobster class example is appropriate for a restaurant. It's not appropriate, and a PotWaterFiller class would make sense, in other situations, such as a huge industrial plant churning out 50 cooked lobsters a second. Not all software targets the same environments. – whatsisname Nov 8 '19 at 2:12
0

As others have said, you may be misunderstanding SRP. Don't feel bad though, SRP is not well defined. You would be better off just to ignore it and concentrate on other more well-defined principles like Coupling and Cohesion and the Law of Demeter.

I'm not really here to comment about that though, but about your design. You say:

Clearly it states that it decrypts a bcrypt hash.

It shouldn't state what it does, but what it is. Objects are things, not procedures. So, what it should be is "BcryptHash". And it probably should verify whether a string hashes into it. Something like:

public class BrcyptHash {
   private string _hash;
   ...
   public bool IsOf(string text)
      ...

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.