-1

I want to validate some doubles before executing the rest of the code. I have some double variables that should be checked if they are in the range else they should throw an exception. I am just wondering if this was a good practice by writing three if statements or do we have other good practices when we are trying to validate these values

public void Execute(Exam1 exam1, Exam2 exam2, Exam3 exam3)
{

    if(Exam1.Value >= -100.0 && Exam1.Value <= 101.2)
    {
       ///DO something
    }
    else
    {
       /// throw an exception
    }

    if(Exam2.Value >= -100.0 && Exam2.Value <= 101.2)
    {
       ///DO something
    }
    else
    {
       /// throw an exception
    }
    if(Exam3.Value >= -100.0 && Exam3.Value <= 101.2)
    {
       ///DO something
    }
    else
    {
        /// throw an exception
    }
}
Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
Mass
  • 1
  • 5
  • 1
    Note: Asking for a “best practice or way” results in general and systematic closure of questions regardless of the question. Sometimes it's better to ask "how to ..." (refactor or simplify or improve or generalize, like here). –  Dec 07 '20 at 15:01
  • not a full answer per se, so keeping this as a comment: I'd suggest to move from using `double` to using `decimal` (i.e. `101.2m`) so you don't suffer from the inherent imprecision of floating point numbers. See for instance [Is floating point math broken?](https://stackoverflow.com/questions/588004/is-floating-point-math-broken) – Pac0 Dec 07 '20 at 15:10
  • 1
    Also, consider asking this kind of question on [codereview.se] – Pac0 Dec 07 '20 at 15:11
  • With this code, if the third check fails, you have "done something" twice already – Hans Kesting Dec 07 '20 at 15:53
  • 1
    @Pac0 when suggesting users post on CR it would be great if there was also a suggestion like "_Please read the relevant help center pages like '[What topics can I ask about here?](https://codereview.stackexchange.com/help/on-topic)' and '[How do I ask a good question?](https://codereview.stackexchange.com/help/how-to-ask)_". In the current form the post above would likely be closed as off-topic because it is missing context, which happens frequently. – Sᴀᴍ Onᴇᴌᴀ Dec 07 '20 at 17:13
  • @SᴀᴍOnᴇᴌᴀ, oh ok, I'll keep this in mind – Pac0 Dec 08 '20 at 08:57

2 Answers2

2

Usually, we validate all the arguments in the very beginning of the routine, and only then do anything with these (valid) arguments:

public void Execute(Exam1 exam1, Exam2 exam2, Exam3 exam3) {
  void Validate(double value, string name) {
    if (value < -100.0 || value > 100.2)
      throw new ArgumentOutOfRangeException(name); 
  }

  Validate(Exam1.Value, nameof(exam1)); 
  Validate(Exam2.Value, nameof(exam2));
  Validate(Exam3.Value, nameof(exam3)); 

  //Do Something 
}
Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
0

You can refactor like that:

check(Exam1.Value, -100.0, 101.2, process1);
check(Exam2.Value, -100.0, 101.2, process2);
check(Exam1.Value, -100.0, 101.2, process3);

void check(double value, double value1, double value2, Action process)
{
  if ( value >= value1 && value <= value2 )
    process?.Invoke();
  else
    throw new Exception();
}

And if bounds are the same:

check(Exam1.Value, process1);
check(Exam2.Value, process2);
check(Exam1.Value, process3);

void check(double value, double value1, double value2, Action process)
{
  if ( value >= -100.0 && value <= 101.2 )
    process?.Invoke();
  else
    throw new Exception();
}

You can adapt this refactoring pattern to your needs, like by indicating the bounds in the method signature:

static public void Execute(Exam1 exam1, Exam2 exam2, Exam3 exam3,
                           double valueLow, double valueHigh)
{
  check(Exam1.Value, process1);
  check(Exam2.Value, process2);
  check(Exam1.Value, process3);
  bool check(double value, Action process)
  {
    if ( value >= valueLow && value <= valueHigh )
      process?.Invoke();
    else
      throw new Exception();
  }
}

So you can generalize more and more as you need, for example by passing a dictionary of objects to check associated to a dictionary of bounds and methods to call.

Also and if relevant, you can put all this behavior in an Exam base class to have a better design and cleaner code.

  • What about the process1,2 and 3 . Do I create three delegates? – Mass Dec 07 '20 at 15:10
  • Yes, or outer lambda, or inner lambda if the code is short like `check(Exam1.Value, () => Console.WriteLine("test"));`. So you can put a method or a lambda. These are the "*do something*" you wrote: Method, Delegate, Action or Func... –  Dec 07 '20 at 15:11