1
\$\begingroup\$

This program is a start point to a more comprehensive program that deals with different numerical systems from binary until 35 base. That includes the traditional decimal, octal and hex.

It is until 35 only, because no more digits other than the number 0-9 and the letters A-Z to represent digits for higher base systems.

The program needs a lot of validation code and other stuff. However, I would like to ask if the conversion from/to the decimal is good or you can show me a better approach.

My main focus is the algorithms for the conversion functions and the helper functions.

Notes:

  1. I do not want to use the built-in pow() function; I wrote my own version.
  2. I do not want to use ASCII in character conversions.

#include <iostream>
#include <string>

using namespace std;

string digit = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";

int power(int base, int exp)
{
    int result = 1;

    for (int i = 0; i < exp; ++i)
        result = result * base;

    return result;
}

int val(char ch)
{
    ch = toupper(ch);

    for (int i = 0; i < 37; ++i)
        if (digit[i] == ch)
            return i;

    return -1;
}




char chr(int d)
{
    return digit[d];
}

string int_to_string(int n)
{
    string result = "";
    int divisor = 10;
    int num = n;
    int r;
    int length = 0;

    while (num > 0)
    {
        num = num / 10;
        ++length;
    }

    num = n;
    divisor = power(10, length) / 10;

    while (num != 0)
    {
        r = num / divisor;
        num = num % divisor;
        result += chr(r);
        divisor /= 10;
    }

    return result;
}


string to_decimal(string str, int base = 10)
{
    if (base == 10) return str;
    if (base < 2 || base > 37) return "Error: base out of range";
    int length = str.length();
    if (length < 1) return "Error: No Number";
    int result = 0;

    for (int i = length - 1; i >= 0; --i)
        result = result + power(base, length - i - 1) * val(str[i]);

    return int_to_string(result);
}

string from_decimal(string str, int base = 10)
{

    if (base == 10) return str;
    if (base < 2 || base > 37) return "Error: base out of range";
    int length = str.length();
    if (length < 1) return "Error: No Number";

    string result = "";
    int divisor = base;
    int num = 0;
    int r;

    for (int i = 0; i < length; ++i)
        num = num + power(10, length - i - 1) * val(str[i]);

    while (num != 0)
    {
        r = num % divisor;
        num = num / divisor;
        result = chr(r) + result;
    }

    return result;
}

bool is_valid(string str, int frombase)
{
    for (char ch : str)
        if (val(ch) >= frombase)
            return false;
    return true;
}

string baseconvert(string str, int frombase,int tobase)
{
    if(is_valid(str, frombase))
        return from_decimal(to_decimal(str,frombase), tobase);
    else
        return "Error: Improper Number used!\n";
}

int main()
{
    cout << baseconvert("FZ", 16,8) << "\n\n\n";
    cout << baseconvert("FE", 16,2) << "\n\n\n";
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You are reinventing the wheel? Why? \$\endgroup\$ – πάντα ῥεῖ May 28 '17 at 19:18
  • \$\begingroup\$ @πάντα ῥεῖ : if you mean the pow() function, this is a requirement that I did not make. if you mean the ASCII, the program should be portable and not every system uses ASCII or even guarantee numeric characters to be consecutive. if you mean something else please clarify. \$\endgroup\$ – Kamal Zidan May 28 '17 at 19:33
  • \$\begingroup\$ Get rid of these magic numbers! \$\endgroup\$ – πάντα ῥεῖ May 28 '17 at 19:35
  • \$\begingroup\$ @πάντα ῥεῖ : You speak too few words. any how, if you mean the binary, oct, hex and others. It is an assignment. beside why should I get rid of it? if you mean something else please be specific. \$\endgroup\$ – Kamal Zidan May 28 '17 at 19:48
  • \$\begingroup\$ More specific: stackoverflow.com/questions/47882/… \$\endgroup\$ – πάντα ῥεῖ May 28 '17 at 19:51
3
\$\begingroup\$

Instead of trying to create your own versions, you could try doing it without.

Not wanting to use ASCII conversions makes sense since that would be inefficient. using a string to get the char value from a specific integer value, works well. However, to go the other way a map would work better, and ignores how the native mapping works.

map<char,int> values = 
{
    {'0',0},
    {'1',1},
    {'2',2},
    {'3',3},
    {'4',4},
    {'5',5},
    {'6',6},
    {'7',7},
    {'8',8},
    {'9',9},
    {'A',10},
    {'B',11},
    {'C',12},
    {'D',13},
    {'E',14},
    {'F',15},
    {'G',16},
    {'H',17},
    {'I',18},
    {'J',19},
    {'K',20},
    {'L',21},
    {'M',22},
    {'N',23},
    {'O',24},
    {'P',25},
    {'Q',26},
    {'R',27},
    {'S',28},
    {'T',29},
    {'U',30},
    {'V',31},
    {'W',32},
    {'X',33},
    {'Y',34},
    {'Z',35}
};

Using a power function reiterates over the same loop over and over again each time you call it. It would make more sense to start at the end of the string and increase a multiplier in each iteration of the existing loop. Also when working with base 10 it is alot simpler to keep the values as integers:

int ToBase10(string value, int base)
{
    int decValue = 0;
    int baseMult = 1;
    for (int i = value.size()-1; i > -1; i--)
    {
        decValue += values[value[i]] * baseMult;
        baseMult *= base;
    }
    return decValue;
}

Since decimal has other meanings, it would be clearer to say base10 instead.

Lastly wrapping your functions up in a static class allows you to expose only what needs to exposed. Also this keeps the validation in only one place:

#include <map>
using std::map;
using std::string;
class BaseConverter
{
    static map<char, int> values;
    static bool GoodValue(string& value, int base);
    static string FromBase10(int value, int base);
    static int ToBase10(string value, int base);
public:
    static string Converter(string value, int fromBase, int toBase);
    static string Converter(int decValue, int base);
};

map<char,int> BaseConverter::values = 
{
    {'0',0},
    {'1',1},
    {'2',2},
    {'3',3},
    {'4',4},
    {'5',5},
    {'6',6},
    {'7',7},
    {'8',8},
    {'9',9},
    {'A',10},
    {'B',11},
    {'C',12},
    {'D',13},
    {'E',14},
    {'F',15},
    {'G',16},
    {'H',17},
    {'I',18},
    {'J',19},
    {'K',20},
    {'L',21},
    {'M',22},
    {'N',23},
    {'O',24},
    {'P',25},
    {'Q',26},
    {'R',27},
    {'S',28},
    {'T',29},
    {'U',30},
    {'V',31},
    {'W',32},
    {'X',33},
    {'Y',34},
    {'Z',35}
};

string BaseConverter::Converter(string value, int fromBase, int toBase)
{
    if(!GoodValue(value,fromBase))
    {
        return "Improper Value";
    }
    if (fromBase == toBase)
    {
        return value;
    }
    return FromBase10(ToBase10(value,fromBase),toBase);
}
string BaseConverter::Converter(int decValue, int base)
{
    return FromBase10(decValue,base);
} 



bool BaseConverter::GoodValue(string& value, int base)
{
    for(int i = 0; i < value.length();i++)
    {
        value[i] = toupper(value[i]);
        if (values[value[i]]>= base)
        {
            return false;
        }   
    }
    return true;
}

string BaseConverter::FromBase10(int value, int base)
{
    const string chars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    string baseValue = "";
    while (value > 0)
    {
        baseValue.insert(baseValue.begin(), chars[value % base]);
        value /= base;
    }
    return baseValue;
}

int BaseConverter::ToBase10(string value, int base)
{
    int decValue = 0;
    int baseMult = 1;
    for (int i = value.size()-1; i > -1; i--)
    {
        decValue += values[value[i]] * baseMult;
        baseMult *= base;
    }
    return decValue;
}

Notice the simplicity of only having to call one wrapper function(string test = BaseConverter::Converter("291", 10, 16);)

\$\endgroup\$
  • \$\begingroup\$ Thanks @tinstaafl. That was a real Code Review. There are a lot of ideas that areuseful not only for this problem but for coding in general. I tried to vote up for your answer, but unfortunately, I have not enough reputations to do so. \$\endgroup\$ – Kamal Zidan May 29 '17 at 7:49
  • 1
    \$\begingroup\$ As the std::map::operator[] is non-const, it will wreak havoc once you use it with a key which isn't yet present in the map (even when only reading!)... if you have to use a std::map, you might want to make it const and use find() instead. \$\endgroup\$ – jvb May 29 '17 at 11:00
  • \$\begingroup\$ There were enough nags in this answer that I was motivated to post my own - see below. \$\endgroup\$ – Reinderien May 30 '17 at 0:45
1
\$\begingroup\$

Notes in the comment block - though @tinstaafl's answer is an improvement from the original, there are some bits that I would definitely change.

#include <assert.h>
#include <string>
#include <string.h>

/*
Compile via
g++ --std=c++11 -o baseconv baseconverter.cpp

Notes:
 - Don't have a class full of statics. This isn't Java; this is a good
   use case for a regular namespace.
 - Don't use std::string unless there's a clear need for it.
 - It's not a good idea to rewrite pow, period. The built-in version will
   be faster and better tested.
 - Try not to write "37" as a literal; instead base it off of the number
   of digit characters you know about.
 - Your chr should have some validation.
 - When you actually do have a string instance, don't use numeric indexes;
   use a real iterator.
 - Pass around const references when appropriate.
*/

namespace BaseConverter
{
    const char chars[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    const int N = sizeof(chars)-1;

    int *index = NULL;

    void init()
    {
        // Since the quantity of data is so low, this will
        // outperform a map.

        if (index) return;
        const int n_ascii = 127; // The size of the lower ASCII range
        index = new int[n_ascii];
        memset(index, -1, sizeof(int)*n_ascii);
        for (int i = 0; i < N; i++)
        {
            index[tolower(chars[i])] = i;
            index[toupper(chars[i])] = i;
        }
    }

    int val(char ch)
    {
        init();
        return index[ch];
    }

    int chr(int d)
    {
        if (d < 0 || d >= N)
            return -1;
        return chars[d];
    }

    std::string int_to_string(int x, int base = 10)
    {
        std::string result;
        for (; x; x /= base)
            result.insert(0,1, chars[x % base]);
        return result;
    }

    int string_to_int(const std::string &str, int base = 10)
    {
        int result = 0;

        for (std::string::const_iterator i = str.cbegin(); i != str.cend(); i++)
        {
            int d = val(*i);
            if (d == -1 || d >= base) return -1;
            result = result*base + d;
        }
        return result;
    }

    bool is_valid(const std::string &str, int base)
    {
        for (std::string::const_iterator i = str.cbegin(); i != str.cend(); i++)
        {
            int d = index[*i];
            if (d == -1 || d >= base)
                return false;
        }
        return true;
    }
}

int main()
{
    assert(BaseConverter::val('A') == 10);
    assert(BaseConverter::chr(15) == 'F');
    assert(BaseConverter::int_to_string(430) == "430");
    assert(BaseConverter::string_to_int("FFFF", 16) == 0xFFFF);
    assert(BaseConverter::is_valid("FE30ee00", 16));
    assert(!BaseConverter::is_valid("3409", 2));
    return 0;
}
\$\endgroup\$

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.