1

I'm writing code that imports values from a CSV file. As part of the import process, I need to write warnings to a log (displayed on screen) when values in the CSV file are invalid.

The method below takes a string value, and a maximum value to check against.

My concern is that the method does two things: it parses a value, and it generates log entries to display. Does this violate the single responsibility principle, and if so, what can be done about it without duplicating the same number-check logic in two methods?

private double? GetValidDoubleInMinMaxRange(string importVal, double? maxScore)
{
    if(importVal == null)
    {
        return null;
    }

    double val;
    if(double.TryParse(importVal, out val))
    {
        if(val < 0)
        {
            AppInstance.Log.Add(this, LogLevel.Warning, Resources.DataScreenImport_ValueMustBePositive);
            return null;
        }

        if(maxScore.HasValue && val > maxScore.Value)
        {
            AppInstance.Log.Add(this, LogLevel.Warning, Resources.DataScreenImport_ValueGreaterThanMax);
            return null;
        }

        return val;
    }
    else
    {
        AppInstance.Log.Add(this, LogLevel.Warning, Resources.DataScreenImport_ValueWasNotValidInt);
        return null;
    }
}
  • 4
    With a different title, I think this would be a good question for Code Review SE. – RubberDuck Feb 25 '16 at 11:23
  • 1
    I'd be more worried about passing nulls around – NimChimpsky Feb 25 '16 at 11:36
  • 1
    Not directly answering the question, but your method does not do what it says it does. It says it gets a valid double, but actually it might return a null in case of an error. I fear you are using nulls as error codes, which is a bad idea in .net. – JacquesB Feb 25 '16 at 12:16
  • related (possibly a duplicate): Style for control flow with validation checks. Most blatant violation of SRP here seems to be mixing validation checks with the rest of code – gnat Feb 25 '16 at 12:46
  • Seems odd to have a warning "ValueMustBePositive". That sounds more like an error. I'd probably throw an exception. On the other hand, to avoid exceptions, one could introduce a notion of a Report as first-class object and add it as a parameter to the function. That avoids using statics which hamper testability and tightly couples dependencies. – Erik Eidt Feb 25 '16 at 15:37
2

You have some case of primitive obsession here.

In your case, I would create a dedicated parser DoubleMinMaxParser class and have TryParse(string) static method that then returns how the parsed string is invalid with valid string in valid case and "undefined" in all others.

Maybe something like :

class DoubleMinMaxParser
{
    private readonly double _min;
    private readonly double? _max;

    public enum Result
    {
        Valid,
        TextWasNull,
        InvalidNumber,
        LessThanMin,
        MoreThanMax,
    }

    public DoubleMinMaxParser(double min, double? max)
    {
        _min = min;
        _max = max;
    }

    public Result TryParse(string importVal, out double value)
    {
        if (importVal == null)
        {
            value = 0;
            return Result.TextWasNull;
        }

        if (double.TryParse(importVal, out value))
        {
            if (_min < 0)
            {
                return Result.LessThanMin;
            }

            if (_max.HasValue && value > _max.Value)
            {
                return Result.MoreThanMax;
            }

            return Result.Valid;
        }
        else
        {
            return Result.InvalidNumber;
        }
    }
}

internal class CsvParser
{
    public string GetMessageForMinMaxResult(DoubleMinMaxParser.Result result)
    {
        switch (result)
        {
            case DoubleMinMaxParser.Result.TextWasNull:
                return Resources.DataScreenImport_ValueWasEmpty;
            case DoubleMinMaxParser.Result.InvalidNumber:
                return Resources.DataScreenImport_ValueWasNotValidInt;
            case DoubleMinMaxParser.Result.LessThanMin:
                return Resources.DataScreenImport_ValueMustBePositive;
            case DoubleMinMaxParser.Result.MoreThanMax:
                return Resources.DataScreenImport_ValueGreaterThanMax;
            default:
                throw new ArgumentOutOfRangeException("result");
        }
    }

    private double? GetValidDoubleInMinMaxRange(string importVal, DoubleMinMaxParser parser)
    {
        double value;
        DoubleMinMaxParser.Result result = parser.TryParse(importVal, out value);

        if(result == DoubleMinMaxParser.Result.Valid)
        {
            return value;
        }
        else
        {
            AppInstance.Log.Add(this, LogLevel.Warning, GetMessageForMinMaxResult(result));
            return null;
        }
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.