Search code examples
c#.netrefactoring

Separate method or modify existing for functionality


I'm building method Convert to turn string of zeroes and ones into int:

public int Convert(string binaryString)
{
    ValidateOrThrow(binaryString);

    return ConvertInternal(binaryString); // Peforms convertion.
}

This method runs validation for provided string:

private void ValidateOrThrow(string binaryString)
{
    if (binaryString is null || binaryString.Length == 0)
        throw new InvalidDataException("Can't convert null or empty string");

    if (binaryString.Any(value => (byte)value is not 48 or 49))
        throw new InvalidDataException("Can't convert non-binary string");
}

Now I want to add new method TryConvert which returns bool as the result of Valdate instead of throwing an exception.

public bool TryConvert(string binaryString, out int result)
{
    if (!Validate(binaryString))
        return false;

    result = ConvertInternal(binaryString);
    return true;
}

I can create one more method Validate with bool return type, but it breaks DRY.

Or I can create single parameterized method and use it in both cases with different parameter:

private bool Validate(string binaryString, bool throwOnInvalidData)
{
    if (binaryString is null || binaryString.Length == 0)
    {
        if (throwOnInvalidData)
            throw new InvalidDataException("Can't convert null or empty string");
        else
            return false;
    }

    if (binaryString.Any(value => (byte)value is not 48 or 49))
    {
        if (throwOnInvalidData)
            throw new InvalidDataException("Can't convert non-binary string");
        else
            return false;
    }

    return true;
}

Is this valid way or I'm missing something?


Solution

  • I don't like the nested if statements in your validate. What if you gave it an out parameter too -

    private bool Validate(string binaryString, out string error)
    {
         error = null;
         if (binaryString is null || binaryString.Length == 0)
         {
             error = "Can't convert null or empty string";
             return false;
         }
    
         if (binaryString.Any(value => (byte)value is not 48 or 49))
         {
              error = "Can't convert non-binary string";
              return false;
         }
    
         return true;
    }
    

    Then you have two methods that use it -

    public int Convert(string binaryString)
    {
        if(!Validate(binaryString, out var error))
        {
            throw new InvalidDataException(error);
        }
    
        return ConvertInternal(binaryString);
    }
    

    and

    public bool TryConvert(string binaryString, out int result)
    {
        result = 0;
    
        if (!Validate(binaryString, out var error))
        {
            return false;
        }
    
        result = ConvertInternal(binaryString);
        return true;
    }