Throwing exceptions when validation fails
$begingroup$
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
?
c# .net exception mvp
$endgroup$
add a comment |
$begingroup$
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
?
c# .net exception mvp
$endgroup$
add a comment |
$begingroup$
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
?
c# .net exception mvp
$endgroup$
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
?
c# .net exception mvp
c# .net exception mvp
asked Aug 15 '14 at 19:02
CADCAD
90341835
90341835
add a comment |
add a comment |
4 Answers
4
active
oldest
votes
$begingroup$
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?
$endgroup$
$begingroup$
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 aValidateX
method to handle validation errors.
$endgroup$
– sapi
Aug 16 '14 at 0:17
2
$begingroup$
@sapi what if it were a functionIsValid ()
? Would you expect a Validation exception or a false? Exceptions are for Exceptional circumstances. And not for the everyday incorrect user input.
$endgroup$
– Vogel612♦
Aug 16 '14 at 17:11
2
$begingroup$
@Vogel612 If it's namedIsValid
, 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.
$endgroup$
– sapi
Aug 18 '14 at 3:02
add a comment |
$begingroup$
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"); }
}
$endgroup$
add a comment |
$begingroup$
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");
$endgroup$
add a comment |
$begingroup$
Whenever I do validation I end up with a type that does validation and the Validate method returns a result. I let the thing calling it decide whether or not validation failure is an exceptional circumstance (which in my opinion it isn't).
public class MyValidator : IValidator<TypeToValidate> {
public ValidationResult Validate(TypeToValidate objToValidate) {
// Do whatever validation here...
return new ValidationResult(false, "Some useful message here");
}
}
I like the result to tell me everything that is wrong with what I passed in to have validated instead of just failing on the first thing and returning. You end up in a situation where the user gets the thing that fails, tries again, sees another thing fail, fixes it, etc. Not a good user experience in my opinion.
New contributor
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f60176%2fthrowing-exceptions-when-validation-fails%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
4 Answers
4
active
oldest
votes
4 Answers
4
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
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?
$endgroup$
$begingroup$
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 aValidateX
method to handle validation errors.
$endgroup$
– sapi
Aug 16 '14 at 0:17
2
$begingroup$
@sapi what if it were a functionIsValid ()
? Would you expect a Validation exception or a false? Exceptions are for Exceptional circumstances. And not for the everyday incorrect user input.
$endgroup$
– Vogel612♦
Aug 16 '14 at 17:11
2
$begingroup$
@Vogel612 If it's namedIsValid
, 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.
$endgroup$
– sapi
Aug 18 '14 at 3:02
add a comment |
$begingroup$
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?
$endgroup$
$begingroup$
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 aValidateX
method to handle validation errors.
$endgroup$
– sapi
Aug 16 '14 at 0:17
2
$begingroup$
@sapi what if it were a functionIsValid ()
? Would you expect a Validation exception or a false? Exceptions are for Exceptional circumstances. And not for the everyday incorrect user input.
$endgroup$
– Vogel612♦
Aug 16 '14 at 17:11
2
$begingroup$
@Vogel612 If it's namedIsValid
, 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.
$endgroup$
– sapi
Aug 18 '14 at 3:02
add a comment |
$begingroup$
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?
$endgroup$
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?
answered Aug 15 '14 at 19:11
Mathieu GuindonMathieu Guindon
60.9k14147419
60.9k14147419
$begingroup$
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 aValidateX
method to handle validation errors.
$endgroup$
– sapi
Aug 16 '14 at 0:17
2
$begingroup$
@sapi what if it were a functionIsValid ()
? Would you expect a Validation exception or a false? Exceptions are for Exceptional circumstances. And not for the everyday incorrect user input.
$endgroup$
– Vogel612♦
Aug 16 '14 at 17:11
2
$begingroup$
@Vogel612 If it's namedIsValid
, 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.
$endgroup$
– sapi
Aug 18 '14 at 3:02
add a comment |
$begingroup$
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 aValidateX
method to handle validation errors.
$endgroup$
– sapi
Aug 16 '14 at 0:17
2
$begingroup$
@sapi what if it were a functionIsValid ()
? Would you expect a Validation exception or a false? Exceptions are for Exceptional circumstances. And not for the everyday incorrect user input.
$endgroup$
– Vogel612♦
Aug 16 '14 at 17:11
2
$begingroup$
@Vogel612 If it's namedIsValid
, 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.
$endgroup$
– sapi
Aug 18 '14 at 3:02
$begingroup$
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.$endgroup$
– sapi
Aug 16 '14 at 0:17
$begingroup$
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.$endgroup$
– sapi
Aug 16 '14 at 0:17
2
2
$begingroup$
@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.$endgroup$
– Vogel612♦
Aug 16 '14 at 17:11
$begingroup$
@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.$endgroup$
– Vogel612♦
Aug 16 '14 at 17:11
2
2
$begingroup$
@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.$endgroup$
– sapi
Aug 18 '14 at 3:02
$begingroup$
@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.$endgroup$
– sapi
Aug 18 '14 at 3:02
add a comment |
$begingroup$
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"); }
}
$endgroup$
add a comment |
$begingroup$
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"); }
}
$endgroup$
add a comment |
$begingroup$
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"); }
}
$endgroup$
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"); }
}
answered Aug 15 '14 at 19:10
RubberDuckRubberDuck
27.2k456161
27.2k456161
add a comment |
add a comment |
$begingroup$
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");
$endgroup$
add a comment |
$begingroup$
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");
$endgroup$
add a comment |
$begingroup$
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");
$endgroup$
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");
answered Aug 15 '14 at 19:23
IEatBagelsIEatBagels
8,98323378
8,98323378
add a comment |
add a comment |
$begingroup$
Whenever I do validation I end up with a type that does validation and the Validate method returns a result. I let the thing calling it decide whether or not validation failure is an exceptional circumstance (which in my opinion it isn't).
public class MyValidator : IValidator<TypeToValidate> {
public ValidationResult Validate(TypeToValidate objToValidate) {
// Do whatever validation here...
return new ValidationResult(false, "Some useful message here");
}
}
I like the result to tell me everything that is wrong with what I passed in to have validated instead of just failing on the first thing and returning. You end up in a situation where the user gets the thing that fails, tries again, sees another thing fail, fixes it, etc. Not a good user experience in my opinion.
New contributor
$endgroup$
add a comment |
$begingroup$
Whenever I do validation I end up with a type that does validation and the Validate method returns a result. I let the thing calling it decide whether or not validation failure is an exceptional circumstance (which in my opinion it isn't).
public class MyValidator : IValidator<TypeToValidate> {
public ValidationResult Validate(TypeToValidate objToValidate) {
// Do whatever validation here...
return new ValidationResult(false, "Some useful message here");
}
}
I like the result to tell me everything that is wrong with what I passed in to have validated instead of just failing on the first thing and returning. You end up in a situation where the user gets the thing that fails, tries again, sees another thing fail, fixes it, etc. Not a good user experience in my opinion.
New contributor
$endgroup$
add a comment |
$begingroup$
Whenever I do validation I end up with a type that does validation and the Validate method returns a result. I let the thing calling it decide whether or not validation failure is an exceptional circumstance (which in my opinion it isn't).
public class MyValidator : IValidator<TypeToValidate> {
public ValidationResult Validate(TypeToValidate objToValidate) {
// Do whatever validation here...
return new ValidationResult(false, "Some useful message here");
}
}
I like the result to tell me everything that is wrong with what I passed in to have validated instead of just failing on the first thing and returning. You end up in a situation where the user gets the thing that fails, tries again, sees another thing fail, fixes it, etc. Not a good user experience in my opinion.
New contributor
$endgroup$
Whenever I do validation I end up with a type that does validation and the Validate method returns a result. I let the thing calling it decide whether or not validation failure is an exceptional circumstance (which in my opinion it isn't).
public class MyValidator : IValidator<TypeToValidate> {
public ValidationResult Validate(TypeToValidate objToValidate) {
// Do whatever validation here...
return new ValidationResult(false, "Some useful message here");
}
}
I like the result to tell me everything that is wrong with what I passed in to have validated instead of just failing on the first thing and returning. You end up in a situation where the user gets the thing that fails, tries again, sees another thing fail, fixes it, etc. Not a good user experience in my opinion.
New contributor
New contributor
answered 1 hour ago
Logan KLogan K
1
1
New contributor
New contributor
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f60176%2fthrowing-exceptions-when-validation-fails%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown