4
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some helpful answers. \$\endgroup\$ – SirPython Oct 11 '15 at 2:19
  • \$\begingroup\$ Two very important things: 1) indent your code properly to convey nesting and structure. 2) put some spacing between operators. wedon'twriteplaintextwithoutspaces, so don't do that in your code! \$\endgroup\$ – glampert Oct 11 '15 at 3:13
  • 1
    \$\begingroup\$ I am guessing you are mixing tabs/spaces in your code (this can cause issues when pasting the code to websites like this). Anyway all real companies will dictate you use one or the other (ie all space or all tab, people have big fights over which is correct personally I think this is silly it does not matter as long as you are consistent within a project (the rest is simply setting up your editor to do the correct thing, all good editors will do the conversions automatically)). But the main point of this long winded point. Is formatting matters. Make sure it is nice and easy to read. \$\endgroup\$ – Martin York Oct 11 '15 at 8:03
  • 1
    \$\begingroup\$ What you may and may not do after receiving answers \$\endgroup\$ – rolfl Oct 17 '15 at 14:04
4
\$\begingroup\$

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 
}
\$\endgroup\$
  • \$\begingroup\$ Loki Astari thanks for the answer, its really good and I am realizing my faults...you see, I am from Indian and in Indian school ,the teacher teach coding with a TURBOC++!!!!, even 75 % of the books teacher with old TC++!....I am just a beginner here but I am trying to break the boundaries, but I know that I should not use C++03, hence I am trying to learn by myself as much as possible, thanks again for your advice, I will surely try to improve my code according to the new standards. My school has not covered the algorithms yet, any online resource to learn it? Sorry for bad English. \$\endgroup\$ – Anuraag Biswas Oct 11 '15 at 8:49
  • \$\begingroup\$ I didn't get that last part, From 'if you can't use algorithms. Then prefer the range based for...........' Can you explain it gain, I have not learned it, but will be happy if you teach it. Thank You \$\endgroup\$ – Anuraag Biswas Oct 11 '15 at 12:52
  • \$\begingroup\$ @AnuraagBiswas any_of does something that your professor may want you to implement manually for didatic reasons. So, maybe your professor wouldn't allow using functions of that sort. This is what he meant by "using algorithms". The range based for was described in the answer (the for (type variableName: thingToLoop) thing) \$\endgroup\$ – Kroltan Oct 11 '15 at 14:57
  • \$\begingroup\$ "Also the keywords 'or' and 'and' actually require you to include a specific header file before they are valid." That's wrong in C++. They are proper keywords (unlike in C where you indeed need to include a header). \$\endgroup\$ – celtschk Oct 11 '15 at 16:57
  • \$\begingroup\$ Thank You for your supports, another question , How and what functions can I make to improve this code? Everybody is advising me to put functions in there, and I just learned User Defined functions in school today, so any tips? \$\endgroup\$ – Anuraag Biswas Oct 12 '15 at 6:41
6
\$\begingroup\$

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..

\$\endgroup\$
  • \$\begingroup\$ Flew over my head, read the description, I am a beginner!!...thanks for the indentation advice btw. \$\endgroup\$ – Anuraag Biswas Oct 11 '15 at 3:16
  • \$\begingroup\$ I'm a beginner as well, and this is my first answer on this forum. Which part of my answer did you not understand? I can re-write / elaborate on it :-) \$\endgroup\$ – Koby Becker Oct 11 '15 at 3:27
  • \$\begingroup\$ er...all of it?, it's okay, I need to learn more. \$\endgroup\$ – Anuraag Biswas Oct 11 '15 at 3:43
  • \$\begingroup\$ all of it? let's start with point #2. What is it that you don't understand? Instead of a do-while, I suggested using a while, and the condition in the while loop be whether the input is valid or not. to check validity, you can call a function isValid that returns true if the string is a valid hex, otherwise 'false'. If it is not a valid hex string the very first time, we never enter the loop. does that make sense? \$\endgroup\$ – Koby Becker Oct 11 '15 at 4:13
  • \$\begingroup\$ but I also did the same thing in my code...didn't I? \$\endgroup\$ – Anuraag Biswas Oct 11 '15 at 7:31

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.