3
\$\begingroup\$

I wrote a small method for convert number from base-10 number system to any number system (code is not perfect but that is not the point). My question is if I get right single responsibility principle. Is 2nd approach better than 1st or 'charReplace' functionality is too small for creating own method ?

    public static string convertNumber(BigInteger num, int baseNum)
    {
        string symbols = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
        if (baseNum < 2 || symbols.Length + 10 < baseNum)
        {
            throw new Exception;
        }

        var result = "";
        do
        {
            var partResult = (int)(num % baseNum);
            if (partResult >= 10)
            {
                result = result.Insert(0, symbols[partResult - 10].ToString());
            }
            else
            {
                result = result.Insert(0, partResult.ToString());
            }
            num /= baseNum;
        } while (num != 0);

        return result;
    }

vs.

    static readonly string Symbols = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";

    public static string convertNumber(BigInteger num, int baseNum)
    {
        if (baseNum < 2 || Symbols.Length + 10 < baseNum)
        {
            throw new Exception;
        }

        var result = "";
        do
        {
            var partResult = (int)(num % baseNum);
            result = result.Insert(0, charReplace(partResult));
            num /= baseNum;
        } while (num != 0);

        return result;
    }

    private static string charReplace(int number){
        if (number < 10){
            return number.ToString();
        }

        return Symbols[number - 10].ToString();
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Pedantic note: BigInteger is not exactly a "base-10" number. It is just abstractly a number. While it's true that BigInteger.Parse() and BigInteger.ToString() both work in base ten, the BigInteger itself isn't in any particular base. \$\endgroup\$ – 200_success Mar 11 '17 at 13:31
2
\$\begingroup\$

I believe the 1st approach is good enough since it personally more readable.

Little suggestions

  • I'd prefer to store the whole alphabet in the symbols string, instead of only letters. This could simplify the logic.
  • You can calculate the resulting length of the output string and avoid string insertions.

Summarizing:

public static string ConvertNumber(BigInteger num, int baseNum)
{
    const string symbols = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
    if (baseNum < 2 || symbols.Length < baseNum)
    {
        throw new ArgumentOutOfRangeException(nameof(baseNum));
    }

    if (num < 0)
    {
        throw new ArgumentOutOfRangeException(nameof(num));
    }

    var resultLength = 1 + Math.Max((int)BigInteger.Log(num, baseNum), 0);
    var result = new char[resultLength];
    int index = resultLength - 1;
    do
    {
        result[index--] = symbols[(int)(num % baseNum)];
        num /= baseNum;
    } while (num != 0);

    return new string(result);
}

PS. It's recommended to name all classes and all methods in UpperCamelCase regardless of their publicity.

\$\endgroup\$
  • \$\begingroup\$ Thank you. Can you explain why is there Math.Max() ? \$\endgroup\$ – GRO Mar 13 '17 at 20:22
  • \$\begingroup\$ @GRO Because if (num == 0) the log is -Infinity. \$\endgroup\$ – Dmitry Mar 13 '17 at 21:56

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.