C++: Simplifying my program to convert numbers to from one base to another.

Question

Hello,

I'm taking a beginner C++ course. I received an assignment telling me to write a program that converts an arbitrary number from any base between binary and hex to another base between binary and hex. I was asked to use separate functions to convert to and from base 10. It was to help us get used to using arrays. (We already covered passing by reference previously in class.) I already turned this in, but I'm pretty sure this wasn't how I was meant to do it:

#include <iostream>
#include <conio.h>
#include <cstring>
#include <cmath>

using std::cout;
using std::cin;
using std::endl;

int to_dec(char value[], int starting_base);
char* from_dec(int value, int ending_base);

int main() {
    char value[30];
    int starting_base;
    int ending_base;
    cout << "This program converts from one base to another, so long as the bases are" << endl
        << "between 2 and 16." << endl
        << endl;
input_numbers:
    cout << "Enter the number, then starting base, then ending base:" << endl;
    cin >> value >> starting_base >> ending_base;
    if (starting_base < 2 || starting_base > 16 || ending_base < 2 || ending_base > 16) {
        cout << "Invalid base(s). ";
        goto input_numbers;
    }
    for (int i=0; value[i]; i++) value[i] = toupper(value[i]);
    cout << "Base " << ending_base << ": " << from_dec(to_dec(value, starting_base), ending_base) << endl
        << "Press any key to exit.";
    getch();
    return 0;
}

int to_dec(char value[], int starting_base) {
    char hex[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
    long int return_value = 0;
    unsigned short int digit = 0;
    for (short int pos = strlen(value)-1; pos > -1; pos--) {
        for (int i=0; i<starting_base; i++) {
            if (hex[i] == value[pos]) {
                return_value+=i*pow((float)starting_base, digit++);
                break;
            }
        }
    }
    return return_value;
}

char* from_dec(int value, int ending_base) {
    char hex[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
    char *return_value = (char *)malloc(30);
    unsigned short int digit = (int)ceil(log10((double)(value+1))/log10((double)ending_base));
    return_value[digit] = 0;
    for (; value != 0; value/=ending_base) return_value[--digit] = hex[value%ending_base];
    return return_value;
}

I'm pretty sure this is more advanced than it was meant to be. How do you think I was supposed to do it?

I'm essentially looking for two kinds of answers:

1. Examples of what a simple solution like the one my teacher probably expected would be.
2. Suggestions on how to improve the code.

Answer 1

I don't think you need the inner loop:

for (int i=0; i<starting_base; i++) {

What is its purpose?

Rather, you should get the character at value[ pos ] and convert it to an integer. The conversion depends on base, so it may be better to do it in a separate function.

You are defining char hex[ 16 ] twice, once in each function. It may better to do it at only one place.

---

EDIT 1: Since this is "homework" tagged, I cannot give you the full answer. However, here is an example of how to_dec() is supposed to work. (Ideally, you should have constructed this!)

Input:

  char * value = 3012, 
  int base = 4, 

Math:

Number = 3 * 4^3 + 0 * 4^2 + 1 * 4^1 + 2 * 4^0 = 192 + 0 + 4 + 2 = 198

Expected working of the loop:

  x = 0
  x = 4x + 3 = 3
  x = 4x + 0 = 12
  x = 4x + 1 = 49
  x = 4x + 2 = 198

  return x;

---

EDIT 2:

Fair enough! So, here is some more :-)

Here is a code sketch. Not compiled or tested though. This is direct translation of the example I provided earlier.

unsigned
to_dec( char * inputString, unsigned base )
{
  unsigned rv = 0; // return value
  unsigned c;      // character converted to integer

  for( char * p = inputString; *p; ++p ) // p iterates through the string
  {
    c = *p - hex[0];
    rv = base * rv + c;
  }

  return rv;
}

Answer 2

You can initialize arrays by string literals (notice that the terminating \0 is not included because the size of the array doesn't permit that):

char const hex[16] = "0123456789ABCDEF";

Or just use a pointer to the string literal for the same effect:

char const* hex = "0123456789ABCDEF";

Answer 3

I would stay away from GOTO statements unless they are absolutely necessary. GOTO statements are easy to use but will lead to 'spaghetti code'.

Try using a loop instead. Something along the lines of this:

bool base_is_invalid = true;
while ( base_is_invalid ) {

    cout << "Enter the number, then starting base, then ending base:" << endl;
    cin >> value >> starting_base >> ending_base;

    if (starting_base < 2 || starting_base > 16 || ending_base < 2 || ending_base > 16)
        cout << "Invalid number. ";
    else
        base_is_invalid = false;

}

Answer 4

to_dec() looks to complicated, here is my shot at it:

int to_dec(char* value, int starting_base)
{
    int return_value = 0;
    for (char* cur = value + strlen(value) - 1; cur >= value; cur--) {
        // assuming chars are ascii/utf: 0-9=48-57, A-F=65-70
        // faster than loop
        int inval = *cur - 48;
        if (inval > 9) {
            inval = *cur - 55;
            if (inval > 15) {
                // throw input error
            }
        }
        if (inval < 0) {
            // throw input error
        }
        if (inval >= starting_base) {
            // throw input error
        }
        // now the simple calc
        return_value *= starting_base;
        return_value += inval;
    }
    return return_value;
}

Answer 5

Apart from the things already mentioned, I would suggest using the new-operator instead of free. The advantages of new are that it also does call constructors - which is irrelevant here since you're using a POD type, but important when it comes to objects such as std::string or your own custom classes - and that you can overload the new operator to suit your specific needs (which is irrelevant here, too :p). But don't go ahead using malloc for PODs and new for classes, since mixing them is considered bad style.

But okay, you got yourself some heap memory in from_dec... but where is it freed again? Basic rule: memory that you malloc (or calloc etc) must be passed to free at some point. The same rule applies to the new-operator, just that the release-operator is called delete. Note that for arrays, you need new[] and delete[]. DON'T ever allocate with new and release with delete[] or the other way around, since the memory won't be released correctly.

Nothing evil will happen when your toy program won't release the memory... I guess your PC has got enough RAM to cope with it and when you shut down your program, the OS releases the memory anyway.. but not all programs are (a) that tiny and (b) shut down often.

Also I'd avoid conio.h, since this is not portable. You're not using the most complicated IO, so the standard headers (iostream etc) should do.

Likewise, I think most programmers using modern languages follow the rule "Only use goto if other solutions are really crippled or tons of more work". This is a situation that can be easily solved by using loops, as shown by emceefly. In your program the goto is easy to handle, but you won't be writing such small programs forever, will you? ;) I, for example, was presented with some legacy code recently.. 2000 lines of goto-littered code, yay! Trying to follow the code's logical flow was almost impossible ("Oh, jump ahead 200 lines, great... who needs context anyway"), even harder was to rewrite the damn thing.

So okay, your goto doesn't hurt here, but where's the benefit? 2-3 lines shorter? Doesn't really matter overall (if you're paid by lines of code, this could also be a major disadvantage ;)). Personally I find the loop version more readable and clean.

As you see, most of the points here can be ignored easily for your program, since it's a toy program. But when you think of larger programs, they make more sense (hopefully) ;)

Answer 6

for the initial conversion from ascii to an integer, you can also use a lookup table (just as you are using a lookuptable to to the conversion the other way around) , which is much faster then searching through the array for every digit.

 int to_dec(char value[], int starting_base) 
 {
      char asc2BaseTab = {0,1,2,3,4,5,6,7,8,9,-1,-1,-1,-1,-1,-1,-1,10,11,12,13,14,15, //0-9 and A-F (big caps)
                         -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,  //unused ascii chars
                          10,11,12,13,14,15};  //a-f (small caps)

      srcIdx = strlen(value);
      int number=0;
      while((--srcIdx) >= 0)
      {
           number *= starting_base;          
           char asciiDigit = value[srcIdx];
           if(asciiDigit<'0' || asciiDigit>'f') 
           {
                //display input error
           } 
           char digit = asc2BaseTab[asciiDigit - '0'];
           if(digit == -1)
           {
                 //display input error
           }
           number += digit;
      }
      return number;
 }

p.s. excuses if there are some compile errors in this...I couldn't test it...but the logic is sound.

Answer 7

In your description of the assignment as given it says:

"I was asked to use separate functions to convert to and from base 10."

If that is really what the teacher meant and wanted, which is doubtful, your code doesn't do that:

int to_dec(char value[], int starting_base)

is returning an int which is a binary number. :-) Which in my opinion does make more sense.

Did the teacher even notice that?

Answer 8

C and C++ are different languages, and with different styles of programming. You better not to mix them. (Where C and C++ differ)

If you are trying to use C++, then:

Use std::string instead of char* or char[].

int to_dec(string value, int starting_base);
string from_dec(int value, int ending_base);

No any mallocs, use new/delete. But actually C++ manages memory automatically. The memory is freed as soon as variable is out of scope (unless you are dealing with pointers). And pointers are the last thing you need to deal with.

We don't need here any lookup tables, just a magic string.

string hex = "0123456789ABCDEF";//The index of the letter is its decimal value. A is 10, F is 15.
//usage
char c = 'B';
int value = hex.find( c );//works only with uppercase;

The refactored to_dec can be like that.

int to_dec(string value, int starting_base) {
  string hex = "0123456789ABCDEF";
  int result = 0;
  for (int power = 0; power < value.size(); ++power) {
    result += hex.find( value.at(value.size()-power-1) ) * pow((float)starting_base, power);
  }
  return result;
}

And there is a more elegant algorithm to convert from base 10 to any other See there for example. You have the opportunity to code it yourself :)

Answer 9

In your from_dec function, you're converting the digits from left to right. An alternative is to convert from right to left. That is,

std::string from_dec(int n, int base)
{
    std::string result;
    bool is_negative = n < 0;

    if (is_negative)
    {
       n = - n;
    }

    while (n != 0)
    {
        result = DIGITS[n % base] + result;
        n /= base;
    }

    if (is_negative)
    {
        result = '-' + result;
    }

    return result;
}

This way, you won't need the log function.

(BTW, to_dec and from_dec are inaccurate names. Your computer doesn't store numbers in base 10.)