0

For exampe, method of my class should validate input values before using. Suggest, 2 parameters of method are :

int start
int count

We should validate, that start not less than 0 (should be 0 or greater), count should be in range 0..999. If these 2 parameters are valid, we continue executing our method, otherwise throw an exception BillComInvalidRangeException:

public class BillComInvalidRangeException : Exception
{
    const string invalidRangeMessage = "The range is not valid. 'Start' should be greater than 0, 'Count' in range 0..999.";

    public BillComInvalidRangeException(int start, int count) : base($"{invalidRangeMessage} Passed 'Start' : {start}, 'Count' : {count}")
    {
    }
}

I want to follow SRP and create one more class, named ValidateListRange. I can implement it in 3 approaches:

  1. validate values in method:

    public bool Validate(int start, int count)
{
    return !(start < 0
    || count < 0
    || count > 999);
}

and then just to use:

var validateObject = new ValidateListRange();
if (!validateObject.Validate(start, count))
    throw new BillComInvalidRangeException(start, count);

  1. validate values in static method:

    public static bool Validate(int start, int count)
{
    return !(start < 0
        || count < 0
        || count > 999);
}

then for using:

        if (!ValidateListRange.Validate(start, count))
            throw new BillComInvalidRangeException(start, count);

more shorter record with the same functional. Then our ValidateListRange class is the simple Util class, which can include many methods (validate, generate etc) like this around the project.

But with non static class we have one huge benefit - we can work with interface and then pass necessary validate object without changing our project source code. For example, in the future we should validate 9999, not 999 and we can write a new implementation of ValidateListRange class. If it's necessary

Which approach is better? Or any another approach?

Gauravsa
  • 6,330
  • 2
  • 21
  • 30
Oleg Sh
  • 8,496
  • 17
  • 89
  • 159
  • In my opinion, the best approach, because it doesn't use any fields of the type, so there is no reason to declare a new instace of object to call this method – Sergey Anisimov Jan 30 '19 at 01:46
  • Second approach (with static) is certainly better as not require memory allocation and following garbage collection. I also may think about defining an `InputData` struct with embedded `IsValid` property. But it doesn't make huge difference. – Max Jan 30 '19 at 01:46
  • @Max but it's additional struct, which I don't want to pull to external interface – Oleg Sh Jan 30 '19 at 01:52
  • Personally I write private static methods inside the class itself and instance methods call those static methods for validation. I only do this if validation is required in multiple places. (for single use just write the condition directly) – M.kazem Akhgary Jan 30 '19 at 01:59
  • 1
    @OlegSh, agree to that. I don't know all details, so that was just another suggestion, not necessarily the best one. However, if you have a large number of input parameters, where some should be defaulted most of the time, such approach may look more accurate. So far, I think your static validator is the best way. – Max Jan 30 '19 at 02:00
  • 1
    Also take a look at https://stackoverflow.com/questions/4879521/how-to-create-a-custom-attribute-in-c-sharp – M.kazem Akhgary Jan 30 '19 at 02:05
  • @M.kazemAkhgary but if you need to modify validation rules we have to modify class inside. Also, about writting condition directly, I don't like "dirty" code, so, it would be better don't mix conditions with other code – Oleg Sh Jan 30 '19 at 02:15
  • @Max, so, seems, it would be better to create internal static method if we know, that rules should not be changed and use interface for "external" validate class (with non-static method) if we assume about changing validation rules during using – Oleg Sh Jan 30 '19 at 02:17
  • Nobody likes dirty code, but that's your view of what code is dirty and what is not. about your first approach, you can follow "singleton pattern" to avoid overhead of allocating new objects. notice that those validators should not store state of any kind (they must be immutable), otherwise you can not follow singleton pattern. Also its better that your `ValidateListRange` implement interface and use that interface instead for loose coupling. – M.kazem Akhgary Jan 30 '19 at 02:28
  • one drawback of first approach, it becomes really hard to debug because you have to track what kind of validator you are currently using and I'm not sure if thats the right path. be careful. static methods or attributes are a better option. – M.kazem Akhgary Jan 30 '19 at 02:32
  • Agree with interface (+external validator if conditions may change) and singleton. Say, a reference to validator interface implementation may be a parameter to your main object constructor. Then use sigleton (or several singletons for different "contracts"), so you may apply different validators to different instances of your main class instances. – Max Jan 30 '19 at 02:32
  • @M.kazemAkhgary why I think, that direct validation inside method is dirty. It like violation of SRP, where should be one and only one reason to change your class(method). In this case there are 2 reasons : change logic of method and change validation. Also, difficult to read, what is logic, what is different validation/logging etc :) – Oleg Sh Jan 30 '19 at 03:01
  • @M.kazemAkhgary about testing. Seems to me, vice versa, more easy to test if we can change validator. We can even mock that validator – Oleg Sh Jan 30 '19 at 03:03
  • @Max. Yes, but "It depends" :) Not necessary to make code more complex, if we don't change validator logic in the future. As gurus write : don't make easy things more complex and vice versa :) – Oleg Sh Jan 30 '19 at 03:05
  • You are the boss here, go ahead with what you have in mind, I hope it works out for you :) – M.kazem Akhgary Jan 30 '19 at 03:06
  • @M.kazemAkhgary I just discuss here and just say my thoughts, no matter who is "boss"... What don't like you in my responses? I feel free to hear any ideas :) – Oleg Sh Jan 30 '19 at 03:11
  • No its alright. I just shared a few things that I knew but all approaches that you discussed can be good if used in right scenarios. – M.kazem Akhgary Jan 30 '19 at 03:19

1 Answers1

1

Oleg, I am not sure how stuck you are on throwing that specific exception, but have you considered?:

  1. FluentValidation (https://www.nuget.org/packages/fluentvalidation)

  2. Code Contracts (https://learn.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts)

Mirko
  • 4,284
  • 1
  • 22
  • 19
  • I know about contracts (but not deep yet) and FluentValidation. For FluentValidation we should use additional class (I don't want to make external interface more complex for these 2 parameters, it's more clear for clients of my library, `start` and `count`). For contracts - I will read. But why Exception is bad approach? – Oleg Sh Jan 30 '19 at 03:09
  • I think code contracts is not up to date anymore. https://github.com/Microsoft/CodeContracts/issues/476 – M.kazem Akhgary Jan 30 '19 at 03:10
  • 1
    I didn't have an opinion on Exception being a bad approach. I wasn't sure how well the different frameworks handled throwing custom exceptions. I agree with Code Contracts maybe not being well supported, but your approach of input validations screamed code contracts more than FluentValidation. I like FluentValidation a lot better and use it on a lot of projects. – Mirko Jan 30 '19 at 03:14
  • 1
    To clarify I like FluentValidation as it is more testable. – Mirko Jan 30 '19 at 03:18
  • I agree with you, in this example Fluent Validation is good approach. But when I had been creating this question I thought more general. Inside t`Validate` can be some more complex logic, not only validation data :) – Oleg Sh Jan 31 '19 at 21:37