How do I write this method to adhere to the Single Responsibility Principle?

Question

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;
    }
}

Euphoric · Accepted Answer · 2016-02-25T12:22:45.153

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;
        }
    }
}

How do I write this method to adhere to the Single Responsibility Principle?

1 Answers1