patterncsharpMinor
Function that adds two integer strings
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.
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
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
Each time you call
This has also the advantage that you won't need
If you start the
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
this boils down to
and is subtracting the ASCII values of the chars
so there is no need to call
This will lead to
It still has some magic numbers in it like
which could/should be extracted to meaningful constants but I couldn't come up with the names lacking enough english.
- 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
bf1won't tell Bob the maintainer what it is about, making it harder to maintain the code.
- methods should be named using
PascalCasecasing. 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.