3
\$\begingroup\$

Before I begin using this code in vital systems, I wanted to quadruple-check that it was soundproof. I plan to implement this as part of a larger program, obviously not by itself. Are there any clear flaws (output is incorrect) or optimization issues in this code?

#include <iostream>

using namespace std;

int main() {
    int convertFromCopy, digit, convertTo;
    unsigned int convertFrom;
    string stringDigit, answer;

    cout << "Num (base 10): ";
    cin >> convertFrom;
    cout << "Base: ";
    cin >> convertTo;

    convertFromCopy = convertFrom;

    while (convertFrom != 0){
        digit = convertFrom % convertTo;

        if (digit < 10)
            stringDigit = '0' + digit;
        else
            stringDigit = digit - 10 + 'A';

        answer = stringDigit + answer;

        convertFrom /= convertTo;                
    }
    cout << convertFromCopy << " written in base " << convertTo << " is: " << answer;
    return 0;
}

And as a note, the input will always be int, so I do not need to account for an incorrect input (i.e. if a char is entered where an int should go).

\$\endgroup\$
1
\$\begingroup\$

Do not use using namespace std.

Name your variables consistently: when convertFrom is the number you convert from, convertTo should be the number you convert to. Currently it is the base.

Extract the conversion into its own function. Otherwise your "obviously larger" program will become too hard to read and understand.

Don't reinvent the wheel. Printing a number in a base different from 10 has already been solved by many other people.

Understand that an int is not stored in base 10 during calculations.

Write similar code in the same way: currently you have '0' + digit for digits, but digit - 10 + 'A' for letter digits. Why is the character constant sometimes on the left side and sometimes of the right side?

If you have the slightest chance of running your code on an IBM computer using EBCDIC, be aware that 'I' + 1 ≠ 'J'.

Check all I/O operqtions for errors; here it is the >> operator.

\$\endgroup\$
  • 1
    \$\begingroup\$ I took all of your suggestions under consideration. I removed using namespace std (I agree, using it isn't a wise choice), and instead used std::. I also renamed my variables to make more sense. I noticed on my own that if the num was <= 0 or base <=1 that undefined behavior arose. I added if statements to deal with this (for my purposes I do not need negative bases or nums). I will not be using it on IBM, no worries there. I reordered the '0' + digit code, it is more readable now. Thanks for the suggestions! (Here's the modified code, if you're interested: pastebin.com/MLA9Pj84) \$\endgroup\$ – Anonymous Sep 2 '16 at 1:43
  • \$\begingroup\$ I just found another thing: converting 0 to any base should result in "0", not in an empty string. \$\endgroup\$ – Roland Illig Sep 2 '16 at 6:43
2
\$\begingroup\$
using namespace std;

You may have already seen Why is “using namespace std;” considered bad practice?

Personally, I just find it easier and more readable to write out std:: where necessary.

Declare where used

    int convertFromCopy, digit, convertTo;
    unsigned int convertFrom;
    string stringDigit, answer;

In old C, you used to have to define every variable at the beginning of a block. In modern C and all C++, you can declare them anywhere.

    int convertTo;
    unsigned int convertFrom;

    std::cout << "Num (base 10): ";
    std::cin >> convertFrom;
    std::cout << "Base: ";
    std::cin >> convertTo;

    std::string answer;
    int convertFromCopy = convertFrom;

    while (convertFrom != 0){
        int digit = convertFrom % convertTo;

        std::string stringDigit;
        if (digit < 10) {
            stringDigit = '0' + digit;
        } else {
            stringDigit = digit - 10 + 'A';
        }

        answer = stringDigit + answer;

        convertFrom /= convertTo;                
    }

    std::cout << convertFromCopy << " written in base " << convertTo << " is: " << answer;

I greatly prefer to use the block form for control statements (i.e. add {} to if and else here). I find it both easier to read and less subject to editing errors.

Abstract into functions

    int convertTo;
    unsigned int convertFrom;

    std::cout << "Num (base 10): ";
    std::cin >> convertFrom;
    std::cout << "Base: ";
    std::cin >> convertTo;

    std::string answer = convertFromBaseTo(convertFrom, convertTo);

    std::cout << convertFrom << " written in base " << convertTo << " is: " << answer;

Note that the function removes the need for the convertFromCopy variable.

std::string convertFromBaseTo(unsigned int convertFrom, int convertTo) {
    std::string answer;

    while (convertFrom != 0){
        int digit = convertFrom % convertTo;

        char stringDigit;
        if (digit < 10) {
            stringDigit = '0' + digit;
        } else {
            stringDigit = digit - 10 + 'A';
        }

        answer.insert(answer.begin(), stringDigit);

        convertFrom /= convertTo;                
    }

    return answer;
}

I changed stringDigit to a char because it's just a single character. Adding to answer changed a bit as well.

\$\endgroup\$
  • \$\begingroup\$ I agree, using using namespace std is unwise, so I used std:: instead. I personally prefer declaring my variables at the top of my code, it makes it easier to read for me, but I totally understand what you're getting at. I would use a function, but currently this isn't an option (I won't get into it, but trust me, I don't like it either). As for using char instead of string, when I did this it resulted errors. Thanks for the suggestions! \$\endgroup\$ – Anonymous Sep 2 '16 at 1:49

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.