Throwing exceptions when validation fails

Question

When I want to check the validity of an attendance being entered into the system, I perform following action.

AttendancePresenter Class

    void _View_OnCheckValidity(object sender, EventArgs e)
    {
        //ExecuteAction performs exception handling in Base Class
        this.ExecutAction(() =>
        {
            ValidateAttendance();
        });
    }

    private void ValidateAttendance()
    {
        //DataService method returns true if the attendance is valid.
        var validity = _DataService.CheckValidityOfAnAttendance(_View.EmployeeID, _View.Date, _View.ShiftType);
        //Set the validity of the attendance in a View property.
        //So that View can stop execution if validity is false.
        _View.AttendanceValidity = validity;
        //If validation fails, throw an exception
        if (!validity)
        { throw new Exception("Invalid Attendance. Already there is a matching attendance for this employee"); }
    }

View

private void txtOutTime_KeyPress(object sender, KeyPressEventArgs e)
{
    if (e.KeyChar == (char)Keys.Enter & !string .IsNullOrWhiteSpace(txtEmployeeID.Text) &  txtOutTime.Text != DefaultText)
    {                                
        OnCheckValidity(sender, e);
        if (this.AttendanceValidity)
        { OnEnterAttendance(sender, e); } //This line of code enters attendance into the DB
    }
}

Appreciate if you could review this code and give your feedback. Specially what I want to know is about the exception thrown in presenter if the validation fails. Is it acceptable? Is this is a standard use of throw?

Answer 1

You're throwing System.Exception. Don't do that. If you're going to have to throw an exception for a validation exception, throw a custom ValidationException exception.

You haven't shown the code where you catch and handle that exception, but it's going to have to look like this:

try
{
    // some code
}
catch (Exception exception)
{
    // handle the [validation?] exception
}

The problem with that, is that you don't know if the exception you're catching is due to a validation error, or if there's a division by zero, or if you ran out of memory, or if a database connection failed. With a custom exception type, you can do this:

try
{
    // some code
}
catch (ValidationException exception)
{
    // handle the validation exception
}

And any exception thrown that is not a ValidationException, will bubble up the stack.

---

That said, I wouldn't throw an exception for that. Exceptions should be for exceptional things. If you already know how you're going to handle it (show the error message in a message box?), why not just do that instead of throwing?

Answer 2

private void ValidateAttendance()
{
    //DataService method returns true if the attendance is valid.
    var validity = _DataService.CheckValidityOfAnAttendance(_View.EmployeeID, _View.Date, _View.ShiftType);
    //Set the validity of the attendance in a View property.
    //So that View can stop execution if validity is false.
    _View.AttendanceValidity = validity;
    //If validation fails, throw an exception
    if (!validity)
    { throw new Exception("Invalid Attendance. Already there is a matching attendance for this employee"); }
}

If you replace valdidity with the more standard isValid name, you could get rid of the comment saying //If Validation fails.... I think it just reads more naturally that way. Give it some breathing space around comments too. Removing comments like set this to that is also a good idea.

private void ValidateAttendance()
{
    //DataService method returns true if the attendance is valid.
    var isValid = _DataService.CheckValidityOfAnAttendance(_View.EmployeeID, _View.Date, _View.ShiftType);

    //So that View can stop execution if validity is false.
    _View.AttendanceValidity = isValid;

    if (!isValid)
    { throw new Exception("Invalid Attendance. Already there is a matching attendance for this employee"); }
}

Answer 3

You shouldn't throw Exception, create a custom Exception as proposed @Mat's Mug and throw that one instead, otherwise you might trap an exception you didn't want to trap (ex : MyDatabaseJustExplodedException)!

I'd add that it is never good to have as much lines of comment as you have lines of code! Basically, comments shouldn't explain what you are doing but why you are doing it. If you need to explain what you are doing, something is wrong with your code.

I think the only comment that is worth its place is this one
//So that View can stop execution if validity is false.

I would change AttendanceValidity to IsAttendanceValid, it is way more clear what your property is in charge of.

Finally, I don't think you should have braces around one liners if it is to put them on the same line as your code. What I mean is either do this :

if (!validity) //changed by isValid as suggested @ckuhn203
{
    throw new Exception("Invalid Attendance. Already there is a matching attendance for this employee"); 
}

or this

if (!validity) //changed by isValid as suggested @ckuhn203
    throw new Exception("Invalid Attendance. Already there is a matching attendance for this employee");