Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

3 Answers 3

up vote 10 down vote accepted

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?

share|improve this answer
    
I wouldn't say there's anything wrong with throwing an exception for failed validation; failing validation is exactly the kind of circumstance which your model-level code can't handle, and should pass upwards. I would never expect a ValidateX method to handle validation errors. –  sapi Aug 16 at 0:17
2  
@sapi what if it were a function IsValid ()? Would you expect a Validation exception or a false? Exceptions are for Exceptional circumstances. And not for the everyday incorrect user input. –  Vogel612 Aug 16 at 17:11
1  
@Vogel612 If it's named IsValid, I'd expect a bool. I certainly wouldn't expect a normal validation method (eg, database validation) to meekly return False when it gets bad data, though, and I've never used a framework which doesn't throw exceptions for failed validation. –  sapi Aug 18 at 3:02
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"); }
}
share|improve this answer

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"); 
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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