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?

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:

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?

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.
–
sapiAug 16 '14 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.
–
Vogel612Aug 16 '14 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.
–
sapiAug 18 '14 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.

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");