Hexadecimal number to decimal conversion

Question

I am a semi-beginner in C++ and I have a project in C++ to convert a hexadecimal integer into decimal equivalent, but I extended it and now this program can also convert a fractional part.

Logic I have used:

1. First the whole program is inside a do-while loop so that the user can convert a number as many time as desired.
2. Another do-while loop checks if the inputted number is valid or invalid. If invalid, it again asks the user for input.
3. A for loop counts the number of characters before the decimal point (if any).
4. Another for loop converts the characters before the decimal point into a decimal number.
5. Another for loop converts the characters after the decimal point into a decimal fraction and adds it to the original number.
6. It prints it and asks if the user wants to again run the loop.

I have knowledge on selection statements (if-else), loops (for, while, do-while), operators and other basic things in C++.

Note: This program is compiled with MinGW and CodeBlocks. My school teacher uses TurboC++.

Please provide advice on things which I can understand; don't get too advanced. I want to score full marks on my school project but also want to understand the code.

// This is a C++ project for Unit Test 2 (2015-16). This program can convert a hexadecimal number into decimal equivalent.

#include <iostream>
#include<string.h>
#include<math.h>

using namespace std;
int main()
{
char choice;
cout<<" *** HEXADECIMAL TO DECIMAL CONVERTER ***";
do{                  //The whole program is inside a do-while loop, so that user can convert a number as many times as they want.
char hexd[120];
                                                         //This string will store the hexadecimal number input by the user.
int validn_count=0,length,lcopy,p=1,num,i,countp=0;
double decimal=0;

do{                                 //This do-while loop checks if the inputed number is valid or not. If not valid it again asks to enter the number.
cout<<"\n Enter a hexadecimal number for conversion (0-F): ";
cin>>hexd;
length=strlen(hexd);
for(int i=0;i<length;++i)
{
    if( (hexd[i]>=48 and hexd[i]<=57) or (hexd[i]>=65 and hexd[i]<=70) or      (hexd[i]>=97 and hexd[i]<=102) or hexd[i]==46)
      ++validn_count;
    else
       {
        validn_count=0;
        cout<<"\n ! WARNING! Invalid HEXADECIMAL NUMBER!!!!";
        break;
       }
}

}while(validn_count==0);




for(i=0;i<length;i++)   //This for loop checks how many characters are there before the decimal point (.)
{
    if(hexd[i]==46)
     break;
   else
     ++countp;


}
lcopy=countp;          // Copying the value of countp for usage in the  conversion.

for(i=0;i<countp;++i)  // This for loop converts all the character before    the decimal point (.) into decimal number and stores it in the variable 'decimal'.
 {
  if(hexd[i]>=48 and hexd[i]<=57)
    num=((int)hexd[i]-48);
  else if(hexd[i]>=65 and hexd[i]<=70)
   num=((int)hexd[i]-55);
  else if (hexd[i]>=97 and hexd[i]<=102)
    num=((int)hexd[i]-87);

 decimal=decimal+(num*pow(16,--lcopy));
 }


for(i=(countp+1);i<length;i++) // This for loop converts all the character after the decimal point (.) into decimal number and further stores it in the variable 'decimal'.
    {

    if(hexd[i]>=48 and hexd[i]<=57)
    num=((int)hexd[i]-48);
   else if(hexd[i]>=65 and hexd[i]<=70)
   num=((int)hexd[i]-55);
   else if (hexd[i]>=97 and hexd[i]<=102)
   num=((int)hexd[i]-87);

   decimal=decimal+(num*pow(16,-p));
    ++p;                                    //p is used for the multiply with 16 with negative powers.
   }


   cout<<" \n The decimal equivalent of hexadecimal number "<<hexd<<" is "    <<decimal;
  cout<<"\n Do you want to convert another hexadecimal number into decimal?   (Y/N): ";  // Asks if the user again wants to do a conversion.
 cin>>choice;
 }while(choice=='Y' or choice=='y');
cout<<"\n\n **** THANK YOU FOR USING THIS SOFTWARE ****";
return 0;
}

Answer 1

The main point I would make is readability.

• Your formatting is all over the place making it hard to read.
• Putting everything in one function also make it hard to read.
  The style of coding now popular is referred to as self documenting code. This basically means breaking the code down into well named functions (so you can tell what the code does just by reading the function) and variable names.

Good comment.
I like it. Which if you read my other views I give people a hard time about their comments. Because a bad comment is worse than no comment.

// This is a C++ project for Unit Test 2 (2015-16). This program can convert a hexadecimal number into decimal equivalent.

Be consistent with your style.

• Space or Tabs
• Alignment of {}
• Consistency of spacing.

Here we have inconsistency of spacing.

#include <iostream>
#include<string.h>
#include<math.h>

Its not a major thing. But it adds a cognitive drain to the reader. Try and make your code easy for the reader to decode by being consistent. Also those headers have C++ variants that place things in the standard namespace (std).

#include <iostream>
#include <string>   // I do hope you meant the C++ std::string lib
                    // Using the C string library is very inefficient in 
                    // comparison. This is because you spend a lot of time
                    // finding the end of a C-String.
#include <cmath>    // Most C header files have a C++ variant
                    // where you drop the .h suffix and add c prefix.

Common mistake for all beginners.

using namespace std;

Never do this in real code. I know its in all the books. They do it because they have to save space. In real code you need to save the reader. Also there is a possibility for real errors to happen because of this.

Read: Why is “using namespace std;” considered bad practice?

What choice?

char choice;

Declare variables as close to the point of use as possible.

char hexd[120];

This has a couple of advantages. 1) They are not created/destroyed if they are inside a scope that is not used. 2) The type becomes more visible to the reader of the code.

One variable per line please.

int validn_count=0,length,lcopy,p=1,num,i,countp=0;

This is mostly for readability. There is one corner case where it becomes an issue. But if you use the one variable per line rule that will never come up. Also with the one variable per line its easier to spot the variables that are not initialized on creation (maybe you should move them to the point where they are first used?).

Though this is actually a good comment.

do{                                 //This do-while loop checks if the inputed number is valid or not. If not valid it again asks to enter the number.

Its not really required. If you had broken the code into functions. You could say all this with the name of the function.

Don't use magic numbers.

    if( (hexd[i]>=48 and hexd[i]<=57) or (hexd[i]>=65 and hexd[i]<=70) or      (hexd[i]>=97 and hexd[i]<=102) or hexd[i]==46)

First most people don't memorize the ascii table so its hard to read. You can use character literals (which are numbers).

Also the keywords 'or' and 'and' actually require you to include a specific header file before they are valid. Most C++ programmers are actually quite familiar with the || and && as an easy way to write this.

To make it easier to parse you can add some more space and break it neatly across a couple of lines.

    if(  (hexd[i] >= '0' && hexd[i] <= '9')
       ||(hexd[i] >= 'a' && hexd[i] <= 'F')
       ||(hexd[i] >= 'A' && hexd[i] <= 'F')
       || hexd[i] == '.')

Of course what you really want to do is use the built in functions.

    if (std::isxdigit(hexd[i]) || hexd[i] == '.')

You probably have not covered this. But you should probably use an algorithm to check the values rather than a for loop. Check out any_of. I'll leave that one until your teacher gets to algorithms.

If you can't use algorithms. Then prefer the range based for (C++11 required).

// This is what a range based for looks like
for(auto c: hexd) {        // It loops over the container `hexd`
    // STUFF               // puts each value into the variable `c`
}


// Note: Range based for is not available until C++11
//       It is prefered because it works out the start and the end
//       of the container automatically for you (so there is less
//       chance of a programmer error.



// this would be equivalient to
for(int loop = 0; loop < strlen(hexd); ++loop) {
    auto c = hexd[loop];
    //STUFF 
}

Answer 2

To take a quick first stab at some style issues:

• Follow a consistent indentation style. This makes the code extremely readable, and you will thank yourself later when you come back to your code.

  For example:

  void foo()
  {
    do 
    {
      bar();
    } while(0);
  }
  
  int main()
  {
    foo();
  }
  

• Try to break down your program into smaller code blocks that you can reason about them independently, and perhaps even test them independently.

  For example, how about start with this template, and fill in the code:

  // checks
  bool isValid(const std::string &hexInput)
  {
  
  }
  
  int64_t convertHexToDecimal(const std::string& hexString)
  {
  
  }
  
  int main()
  {
      std::string input;
      std::cin >> input;
  
      while(isValid(input))
      {
          // perform the conversion
          int64_t convertedDecimal = convertHexToDecimal(input);
  
          // output the conversion
          std::cout << "..." << convertedDecimal <<  std::endl;
  
          // get new input
          std::cin >> input;
      }
  }
  

• Use std::string liberally instead of a character array. I think this is a more c++11 style of going about it. This will allow you to utilize the wealth of STL. This is more recognizable, and reduces the changes of introducing bugs in your hand-rolled while/for loop.

  auto found = input.find('.');
  if (found != std::string::npos)
  {
      std::cout << "Period found at: " << found << '\n';
  }
  

• Your way of checking if the input string is hexadecimal is very awkward and unreadable. Whenever you use constants, you should declare them as consts, so you can update them without having to change the code.

  A hexadecimal string has characters from one of the following set of characters:

  0 1 2 3 4 5 6 7 8 9 a b c d e f A B C D E F and the ., also the . should appear exactly once, and should probably not be the last character of the string. Also, you can use the characters directly for the comparison instead of the ASCII values.

  bool isValidHex = true;
  
  // for_each will iterate between the the provided iterators
  // for each position, you can pass a lambda function that checks
  // the characters
  std::for_each(
      str.begin(), 
      str.end(), 
      [&isValidHex](char c) 
            {
              // std::cout << "Checking" << c << std::endl; 
              if ((c >= 'a' && c <= 'f') || 
                  (c >= 'A' && c <= 'F') || 
                  (c >= '0' && c  <= '9'))
              {
                  // this is a valid hex.
              }
              else
              {
                  isValidHex = false;
              }              
            });
  

  If lambdas are not your style, you can also use the new c++11 loop style, which is much more readable.

  for (char c : input)
  {
      // perform checks here
  }
  

To Be Continued..