HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Function that adds two integer strings

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functiontwothataddsstringsinteger

Problem

I wrote this method that adds two integers in the form of strings together. This way I can add large integers without overflowing.

How efficient is the code that I wrote? I am by no means a great coder, but I would consider myself somewhat decent. Is there anyway that would make it better?

I am a self taught coder and have only been doing it for about a year, so please be easy on me.

static string addString(string num1, string num2) {
        string retVal = null, cNum1 = null, cNum2 = null;
        int bf1 = 0, bf2 = 0;
        if (num1.Length > num2.Length) {
            for (int i = 0; i  9) {
                retVal += bf2.ToString().Substring(bf2.ToString().Length - 1, 1);
                bf1 = int.Parse(bf2.ToString().Substring(0, bf2.ToString().Length - 1));
            } else {
                retVal += bf2.ToString();
                bf1 = 0;
            }
        }
        if (bf1 != 0) {
            retVal += bf1.ToString();
        }
        char[] ret1 = retVal.ToCharArray(); 
        Array.Reverse(ret1); 
        retVal = new string(ret1);
        return retVal;
    }

Solution

Some style comments

  • Add vertical spaces to group code which logically belongs together.



  • Don't declare multiple variables on the same line.



  • Name your variables as meaningfully as possible. Names like bf1 won't tell Bob the maintainer what it is about, making it harder to maintain the code.



  • methods should be named using PascalCase casing. See: C# naming guidelines



  • Usually a C# developer would expect one to use the Allman indention style; i.e. place the opening brace on the next line.



  • Always add a scope specifier (access modifier) to methods and variables.



This method is violating the Single responsibility principle. It is just doing too much like normalizing the given input, adding the numeric values of the chars and constructing the result.

Let us take a look at what you are doing to normalize the string

string cNum1 = null;
    string cNum2 = null;
    if (num1.Length > num2.Length) {
        for (int i = 0; i < (num1.Length - num2.Length); i++) {
            cNum2 += "0";
        }
    } else if (num1.Length < num2.Length) {
        for (int i = 0; i < (num2.Length - num1.Length); i++) {
            cNum1 += "0";
        }
    }
    cNum1 += num1; 
    cNum2 += num2;


Each time you call cNum1 += "0" or cNum2 += "0" you are creating a new string. By using the PadLeft() method together with Math.Max() and extracting the the normalization to a separate method your code will look cleaner.

private static string NormalizeValue(string value, int maxLength)
{
    return value.PadLeft(maxLength, '0');
}


This has also the advantage that you won't need cNum1 and cNum2 anymore.

static string addString(string num1, string num2) {

    int maxLength = Math.Max(num1.Length, num2.Length);
    num1 = NormalizeValue(num1, maxLength);
    num2 = NormalizeValue(num2, maxLength);

    .....

}


If you start the for loop at maxLength - 1 and decrement the index you won't need the reversing of the char arrays anymore.

Using a StringBuilder to add the results of the single additions will result in fewer created strings.

If you are sure that the passed in strings only contain digits you can use a neat little trick. You can subtract a char '0' from the current char like e.g

Assume first = '2' and second = '4' are the chars currently be processed.

int result = first - '0' + second - '0';


this boils down to

int result = '2' - '0' + '4' - '0' ;


and is subtracting the ASCII values of the chars

int result = 50 - 48 - 52 - 48;


so there is no need to call int.Parse() anymore. By extracting the two uses of '0' to a const this will be beautified.

const int doubleZeroValue = 96;
int result = first + second - doubleZeroValue;


This will lead to

private static string NormalizeValue(string value, int maxLength)
{
    return value.PadLeft(maxLength, '0');
}

private static string AddString(string firstNumber, string secondNumber)
{
    const int doubleZeroValue = 96;
    int maxLength = Math.Max(firstNumber.Length, secondNumber.Length);
    firstNumber = NormalizeValue(firstNumber, maxLength);
    secondNumber = NormalizeValue(secondNumber, maxLength);

    StringBuilder sb = new StringBuilder(maxLength + 1);
    int result = 0;
    for (int i = maxLength - 1; i >= 0; i--)
    {
        result += firstNumber[i] + secondNumber[i] - doubleZeroValue;
        if (result  0)
    {
        sb.Append(result);
    }

    char[] resultDigits = sb.ToString().ToCharArray();
    Array.Reverse(resultDigits);

    return new string(resultDigits);

}


It still has some magic numbers in it like

sb.Append(result - 10);
result /= 10;


which could/should be extracted to meaningful constants but I couldn't come up with the names lacking enough english.

Code Snippets

string cNum1 = null;
    string cNum2 = null;
    if (num1.Length > num2.Length) {
        for (int i = 0; i < (num1.Length - num2.Length); i++) {
            cNum2 += "0";
        }
    } else if (num1.Length < num2.Length) {
        for (int i = 0; i < (num2.Length - num1.Length); i++) {
            cNum1 += "0";
        }
    }
    cNum1 += num1; 
    cNum2 += num2;
private static string NormalizeValue(string value, int maxLength)
{
    return value.PadLeft(maxLength, '0');
}
static string addString(string num1, string num2) {

    int maxLength = Math.Max(num1.Length, num2.Length);
    num1 = NormalizeValue(num1, maxLength);
    num2 = NormalizeValue(num2, maxLength);

    .....

}
int result = first - '0' + second - '0';
int result = '2' - '0' + '4' - '0' ;

Context

StackExchange Code Review Q#99074, answer score: 5

Revisions (0)

No revisions yet.