1

Please help me to identify the error in this program, as for me it's looking correct,I have checked it,but it is giving wrong answers. In this program I have checked explicitly for A,B,C,D,E,F,and according to them their respective values.

[Edited]:Also,this question relates to how a character number is converted to actual integer number.

#include<iostream>
#include<cmath>
#include<bits/stdc++.h>
using namespace std;
void convert(string num)
{
   long int last_digit;
    int s=num.length();
    int i;
    long long int result=0;
    reverse(num.begin(),num.end());                 
    for(i=0;i<s;i++)
    {
        if(num[i]=='a' || num[i]=='A')
        {
            last_digit=10;
            result+=last_digit*pow(16,i);
        }
        else if(num[i]=='b'|| num[i]=='B')
        {
            last_digit=11;
            result+=last_digit*pow(16,i);
        }
        else if(num[i]=='c' || num[i]=='C')
        {
            last_digit=12;
            result+=last_digit*pow(16,i);
        }
        else if(num[i]=='d'|| num[i]=='D' )
        {
            last_digit=13;
            result+=last_digit*pow(16,i);
        }
        else if(num[i]=='e'|| num[i]=='E' )
        {
            last_digit=14;
            result+=last_digit*pow(16,i);
        }
        else if(num[i]=='f' || num[i]=='F')
        {
            last_digit=15;
            result+=last_digit*pow(16,i);
        }
        else {
            last_digit=num[i];
        result+=last_digit*pow(16,i);
        }
    }
    cout<<result;
}
int main()
{
    string hexa;
    cout<<"Enter the hexadecimal number:";
    getline(cin,hexa);
    convert(hexa);
}
Srijoy_paul
  • 77
  • 1
  • 6
  • 3
    Did you check that: `last_digit=num[i];` ? `num[i]` contains the ASCII code of the digit, not the digit. – Damien Mar 31 '21 at 08:43
  • 2
    `pow` is a function for floating point numbers, it may return inexact values. Don't use it for integers. – Yksisarvinen Mar 31 '21 at 08:44
  • 1
    As well as the other comments, you need to subtract the value of the '0' character in the last `else` block: `last_digit=num[i] - '0';` – Adrian Mole Mar 31 '21 at 08:50
  • 2
    The ASCII code of `'A' == 0x41`, `'B' == 0x42`, etc. Just subtract `'A'` and add `10` to get the hex digit value. Or use a lookup table for all digits - better. – zdf Mar 31 '21 at 08:54
  • 1
    Please read [Why should I not #include ?](https://stackoverflow.com/questions/31816095/why-should-i-not-include-bits-stdc-h) - you fell to cargo-cult programming. Especially in combination with `using namespace std;` this can cause a lot of trouble. – Lukas-T Mar 31 '21 at 09:05
  • @Damien Yes! Now I get that. That is what I am doing wrong. – Srijoy_paul Mar 31 '21 at 11:18
  • See all those lines that read `result+=last_digit*pow(16,i);`? Take them all out, and do it **once** after the `if ... else if...` ladder. That's not about correctness (if the code had a couple of errors in the `else` branch fixed it would work correctly), but about readability and maintainability. – Pete Becker Mar 31 '21 at 13:27

3 Answers3

2

Your code is very convoluted and wrong.

You probably want this:

void int convert(string num)
{
  long int last_digit;
  int s = num.length();
  int i;
  long long int result = 0;

  for (i = 0; i < s; i++)
  {
    result <<= 4;                     // multiply by 16, using pow is overkill
    auto digit = toupper(num[i]);     // convert to upper case

    if (digit >= 'A' && digit <= 'F')
      last_digit = digit - 'A' + 10;   // digit is in range 'A'..'F'
    else
      last_digit = digit - '0';        // digit is (hopefully) in range '0'..'9'

    result += last_digit;
  }

  cout << result;
}

But this is still not very good:

  • the function should return a long long int instead of printing the result
  • a few other thing can be done mor elegantly

So a better version would be this:

#include <iostream>
#include <string>

using namespace std;

long long int convert(const string & num)  // always pass objects as const & if possible
{
  long long int result = 0;

  for (const auto & ch : num)      // use range based for loops whenever possible
  {
    result <<= 4;
    auto digit = toupper(ch);

    long int last_digit;           // declare local variables in the inner most scope

    if (digit >= 'A' && digit <= 'F')
      last_digit = digit - 'A' + 10;
    else
      last_digit = digit - '0';

    result += last_digit;
  }

  return result;
}

int main()
{
  string hexa;
  cout << "Enter the hexadecimal number:";
  getline(cin, hexa);
  cout << convert(hexa);
}

There is still room for more improvements as the code above assumes that the string to convert contains only hexadecimal characters. Ideally a check for invalid characters should be done somehow. I leave this as an exercise.

The line last_digit = digit - 'A' + 10; assumes that the codes for letters A to F are contiguous, which in theory might not be the case. But the probability that you'll ever encounter an encoding scheme where this is not the case is close to zero though. The vast majority of computer systems in use today use the ASCII encoding scheme, some use EBCDIC, but in both of these encoding schemes the character codes for letters A to F are contiguous. I'm not aware of any other encoding scheme in use today.

Jabberwocky
  • 48,281
  • 17
  • 65
  • 115
  • Re: `last_digit = digit - 'A' + 10;` -- this makes the non-portable assumption that the character codes for `'A' .. 'F'` are contiguous and increasing. That's not required by the language definition. I don't know of a character encoding for which it's not true, though. – Pete Becker Mar 31 '21 at 13:25
  • @PeteBecker nor do I. I suppose we can safely assume that character codes from A to F are contiguous. I know 2 encodings, ASCII and EBCDIC, and even in the latter A to F are contiguous. – Jabberwocky Mar 31 '21 at 13:28
  • If you write programs based on what you can "safely assume" you're going to crash and burn sooner or later. – Pete Becker Mar 31 '21 at 13:29
  • @PeteBecker there are different levels of "safely assuming". But anyway this is obviously a student exercise where this issue is really not a problem. Feel free to edit my answer or post an other answer. – Jabberwocky Mar 31 '21 at 13:32
  • Sure, it's obvious to you and me; but remember that you're talking to a large group of readers, mostly beginners, who don't have the experience and judgment to recognize that what's obvious to them may well be wrong. – Pete Becker Mar 31 '21 at 13:56
  • @PeteBecker added some explanations – Jabberwocky Mar 31 '21 at 14:17
1

Your problem is in the elsecase in which you convert num[i] from char to its ascii equivalent. Thus, for instance, if you try to convert A0, the 0is converted into 48 but not 0. To correct, you should instead convert your num[i] into its equivalent integer (not in asci).

To do so, replace :

else {
            last_digit=num[i];
        result+=last_digit*pow(16,i);

with

  else {
            last_digit = num[i]-'0';
            result+=last_digit*pow(16,i);
    }

In the new line, last_digit = num[i]-'0'; is equivalent to last_digit = (int)num[i]-(int)'0';which substracts the representation code of any one-digit-number from num[i] from the representation code of '0'

It works because the C++ standard guarantee that the number representation of the 10 decimal digits are contiguous and in incresing order (official ref iso-cpp and is stated in chapter 2.3 and paragraph 3

Thus, if you take the representation (for instance the ascii code) of any one-digit-number num[i] and substract it with the representation code of '0' (which is 48 in ascii), you obtain directly the number itself as an integer value.

An example of execution after the correction would give:

A0
160

F5
245

A small codereview: You are repeating yourself with many result+=last_digit*pow(16,i);. you may do it only once at the end of the loop. But that's another matter.

Pat. ANDRIA
  • 2,330
  • 1
  • 13
  • 27
  • If anyone wants to know why we are subtracting character '0'. https://stackoverflow.com/a/5030086/14965920 this question is very helpful. – Srijoy_paul Mar 31 '21 at 12:10
  • 1
    Re: "substracts the ascii code" -- this works for **any** character encoding, not just ASCII. The C++ standard requires that the codes for the characters `'0' .. '9'` be contiguous and increasing, so subtracting `'0'` always works. – Pete Becker Mar 31 '21 at 13:24
0

You are complicating the problem more than you need to (std::pow is also kinda slow). std::stoul can take a numerical base and automatically convert to an integer for you:

#include <string>
#include <iostream>

std::size_t char_count{0u};
std::string hexa{};
std::getline(std::cin, hexa);
hexa = "0x" + hexa;
unsigned long value_uint = std::stoul(hexa, &char_count, 16);
Casey
  • 10,297
  • 11
  • 59
  • 88