Is this a good enough system-error-to-user-friendly-error translator?

Question

this is how I translate the thrown errors:

        catch (NpgsqlException ex)
        {
            if (tx != null) tx.Rollback();

            if (ex.Message.Contains("foreign key constraint"))
                throw new Exception("Cannot delete this, this record is already referenced on other record(s)");
            else
                throw new Exception(ex.Message, ex.InnerException);
        }

Npgsql sample constraint error:

ERROR: 23503: update or delete on table "color" violates foreign key constraint "fk_order_detail__color" on table "order_detail"

Answer 1

You should write your problem that way so it doesn't cause SQL errors.

Or, if you know specific cause of a error happening, just show it in interface as a message, not a error. I.e. "you should delete dependencies between deleting this object" not "internal error: whatever".

Error is something unexpected; your constraint errors aren't.

Answer 2

If you would like to handle some error more specifically, then ideally you would create a ColorAlreadyUsedException class and throw it. Think of how the caller is going to differentiate the two cases: will it need to check the Message as well?

Still include the original exception as the inner exception.

Answer 3

Your removing information from your exception and not logging it.

I think this is a bad call.
Log the message to be able to work on it, also this error should not happen ever.

Refactor so that this can't happen again.

Answer 4

It seems to me rather flaky to rely on the text of the message when it already has an error code. While there are some other issues with the pattern as outlined by the other answers at the very least I would search for "23503" rather than "foreign key constraint". Otherwise what happens if the user's install of the database is in a different culture, or an updated version of the server changes the text of the message?

---

Actually looking at some online documentation for the exception class you are using it looks like there is even a Code property, this would be much faster and more reliable to compare against than the message property.

---

Final thought...

I agree with Van that you should be throwing a more concrete exception when the constraint is broken, but also you shouldn't be throwing a base Exception when the error is something else. Firstly is is never good practice to throw the base exception class since it doesn't give the client code any easily testable information about what the error is. Secondly you are losing a lot of information, all the extra properties that the NpgsqlException implements plus the stack trace. I'd replace the throwing with something like:

if (ex.Message.Contains("foreign key constraint"))
    throw new ConstraintException("Cannot delete this, this record is already referenced on other record(s)");
else
     throw;

Basically if you aren't going to do anything productive with the exception then don't touch it, you never know what information code back up the call stack may need.