Skip to main content
Redundant variables section added
Source Link
KjMag
  • 641
  • 3
  • 11

Redundant variables

It seems you are not using snum anywhere in your code right now and even if you do use it in the future, it is probably to serve exactly the same puropse as vnum, so it's better to convert one type to another instead of keeping both and using only one of them depending on what's passed to the constructor - right now you would have to double your code by checking which variable is set inside each of your functions.

I would ditch vnum and use snum everywhere instead as std::string is a semantic equivalent of std::vector<char>, except you can benefit from small string optimization.

Redundant variables

It seems you are not using snum anywhere in your code right now and even if you do use it in the future, it is probably to serve exactly the same puropse as vnum, so it's better to convert one type to another instead of keeping both and using only one of them depending on what's passed to the constructor - right now you would have to double your code by checking which variable is set inside each of your functions.

I would ditch vnum and use snum everywhere instead as std::string is a semantic equivalent of std::vector<char>, except you can benefit from small string optimization.

Source Link
KjMag
  • 641
  • 3
  • 11

Abusing the use of macros

I think there is no good reason for implementing to_int() and to_char() as macros - such an implementation may result in hard to find bugs as you don't have any type checking. Lack of parameter types also makes it harder to understand what the function is expected to do in case of more complex functions.

Also, the names of these functions are misleading as they don't inform in a clear way that you are expecting an input to represent a single digit and any other input is invalid.

If you stick to the macro anyway, it's a common convention to use uppercase letters for macro names - most people would expect an ordinary function after seeing lowercase to_int()/to_char().

Single responsibility principle violation

Using catch this way:

catch (InvalidBigInt)
        {
            std::cout << "*****************      Input invalid integer      *****************" << std::endl << \
                "*****************      Please input again         *****************" << std::endl;
        }

you are restricting the use of your class to console applications. Things such as text display should be moved outside of your class as the responsibility of your class is to calculate numbers, not display them nor interact with a user via an interface.

Unclear variable names

Tha name flag is very vague and it's necessary to read the code to see what it's supposed to do (assuming it is used in the intended way). I'm not sure what is its purpose anyway since it's only set and not referred to anywhere in the code. I also suspect it shouldn't be a static variable.

Organizing code into headers and implementation files

Don't put all your code within your main.cpp file. It's more versatile and easier to manage if you separate it and put declarations in headers and definitions in .cpp files - you'll see that as soon as you decide to separate the text display functionality mentioned above from your main class.

Inconsistent and uncommon naming convention

Here:

for (auto IteratorMore = more.begin(), IteratorLess = less.begin();
    IteratorMore < more.end() && IteratorLess < less.end();
    IteratorMore++, IteratorLess++)

the names of the variables IteratorMore and IteratorLess start with a capital 'I', whereas it is a common practice to start the name of a class with the capital letters and start variable names with a lowercase. You are also following this convention correctly throughout your code, except in this part.

Const correctness

You don't always adhere to the concept of const correctness: your operators don't modify the BigInteger argument and thus it should be passed as a const reference to avoid unnecessary copies and make room for possible compiler optimizations.

Code lines' length

In some places in your code, you are writing very long lines, such as:

auto more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum, less = b.vnum.size() < this->vnum.size() ? b.vnum : this->vnum;

Even though most modern IDE's have built-in line wrapping, it's a good practice to write your code in such a way that a single line comprises approximately 80 characters. It's more comfortable and faster to read and comprehend the code if you are not forced to scroll it or move your eyeballs repeatedly from one side of a wide screen to another. Also that way you have control over where you wrap your code, whereas line wrapper functionality may visually organize the code in a way that makes it less readable. Also it's easier to use diff tools and open several editor windows side-by-side if you stick to this rule. See the discussion e.g. here:

https://stackoverflow.com/questions/276022/line-width-formatting-standard