Code smells regarding "Validation" methods

Description:

Any method, which the name hints the intent of validation (eg: PhoneValidation, TryValidateEmail, etc) should:

  • Take a single input
  • Return a boolean or a instance of a object/struct
  • Do not explicitly throw any kind of Exception
  • Should not change the input

WHY?

  1. Single responsibility: One validation, one input…
    bad code:Validate(CompleteAddress addr, SocialSecurityNumber ssn)
    good code: Validate(CompleteAddress addr); Validate(SocialSecurityNumber ssn);
  2. When you validate something, you expect a true/false or a detailed/structured response explaining where is the problem
    bad code: public string ValidatePhone(string phone)
    good code: public bool ValidateLogin(Credentials cred) or public ValidationSummary ValidateAddress(CompleteAddress addr)
  3. A validation method should not alter the input parameter because a validation is generally understood as a idempotent operation. A common violation of such principle is when you also “format” the input, eg: string “+5531999914162” after validation becomes “+55 (31) 99991-4162”
  4. Throwing exceptions is a established bad practice except when you’re dealing with a real unexpected code path… it is not a shortcut to “end the function”, so if you comply with proposition number 2, you won’t need to throw an exception.
    bad code:
if (phoneNumber.Length < 10) 
    throw new ArgumentInvalidException("Phone must be 10 digit long")

good code:

if (phoneNumber.Length < 10) 
    return false;

or

if (phoneNumber.Length < 10) 
    return new Summary(false,"Phone must be 10 digit long");

Hi @Leonardo-Ferreira,

I think this rule is very specific to your code base and it would not generally apply. We automatically import 3rd party roslyn analyzer results so you have the option to write the rule and import the results.

Thank you,
Costin

Hi @costin.zaharia,

I don’t think that is a option for sonar source, correct?

This should happen by default for third-party Roslyn analyzers. You can check the notes about .Net issues from the bottom of this page: Importing third-party issues

Issues from third-party Roslyn analyzers (including Roslyn analyzers provided by Microsoft) are included in the MSBuild output and imported by default into SonarQube so no properties exist to enable that behavior. Instead, properties are available to adjust the import and to stop importing those issues.

Note that Roslyn issues with an error severity automatically fail the build. We don’t recommended running the Scanner for MSBuild’s end step if the MSBuild step fails for any reason because it will result in an essentially empty analysis.