HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Converting an integer to binary, hex, and octal

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
hexbinaryoctalandconvertinginteger

Problem

I've started caring about standards and the beauty of my code. I doubt whether I've written it correctly and aesthetically. Can you please judge this simple, exemplary program? I want to know of any mistakes I've made. It converts an integer number given in the argument to binary, hexadecimal and octal number. Ignore the "1." prefix.

1.conversion_main.cpp:

//Conversion
#include 
#include 
#include 
#include 
#include "1.conversion_header.h"
#include "1.convert.cpp"

int parse(int, char**);

int main(int argc, char *argv[]){
    int number = parse(argc, argv);
    if (number == 0)
      return 0;
    convert converter;
    std::cout > check) && (in.eof()))
                return atoi(argv[1]);
        }
    }
    return 0;
}


1.conversion_header.h:

//Conversion header

class convert{
    public:
          std::string to_hex(int);
          std::string to_oct(int);
          std::string to_bin(int);
  protected:
          std::string int_to_bin(int);
          std::string reverse(std::string);    
};


1.convert.cpp:

```
//Convert definition

#include

std::string convert::int_to_bin(int number){
static std::string result;
static int level = 0;
level++;
if (number > 0){
if (number % 2 == 0){
result.append("0");
}
else{
result.append("1");
}
int_to_bin(number / 2);
level--;
}
if (level == 1) return reverse(result);
return result;
}

std::string convert::reverse(std::string to_reverse){
std::string result;
for (int i = to_reverse.length()-1; i >=0 ; i--)
result += to_reverse[i];
return result;
}

std::string convert::to_hex(int to_convert){
std::string result;
std::stringstream ss;
ss > result;
return result;
}

std::string convert::to_oct(int to_convert){
std::string result;
std::stringstream ss;
ss > result;
return result;
}

std::string convert::to_bin(int to_convert){
return int_to_bin(to_conv

Solution

I'll just address several things regarding good practice. This can probably be simplified greatly overall, but I won't try to focus on that particularly.

main():

-
I don't see the need for if (number == 0), especially in relation to parse(). Are you just skipping further calculation if the return is 0? If you do that, the outputs at the end will not be printed, even though 0 is a valid number.

-
You don't need to use std::endl in main(). That also flushes the buffer, which is unnecessary here, and you do it multiple times. Just use "\n" within a print statement.

parse():

  • You can define it above main(), allowing you to remove the function prototype.



  • It doesn't need the command line parameters; only main() does.



  • Its control paths and returns don't make sense to me. If the if statement is executed, it displays a message and returns 0. If the else is executed, it returns a converted integer. Instead, the function should only be called if it will parse something, which is its main job. Functions should focus on just one thing. Let the calling code decide if it should be called.



Header:

-
There's no need for protected if you're not using inheritance (I believe even Bjarne Stroustrup himself has regretted adding that keyword). Either change it to private, or just remove the keyword since classes are private by default.

-
If you're not maintaining one or more data members, then this program may not need to utilize classes. This one just contains functions, but class functions are supposed to change the state of one or more data members. Instead, those functions could just be free functions (non-member) and the class removed entirely.

Other:

-
This:

if (number % 2 == 0){
    result.append("0");
}
else{
    result.append("1");
}


can just be made into a single-ternary statement:

(number % 2 == 0) ? result.append("0") : result.append("1");


Better yet, since std::string supports +=, use that instead:

result += (number % 2 == 0) ? "0" : "1";


-
The local variables in int_to_bin() don't need to be static.

-
You don't need your own reverse function; just use std::reverse().

-
As @rachet freak has mentioned in the comments, you can just use std::hex to manipulate the I/O when displaying this value. In addition, you can do this with all three number systems.

Code Snippets

if (number % 2 == 0){
    result.append("0");
}
else{
    result.append("1");
}
(number % 2 == 0) ? result.append("0") : result.append("1");
result += (number % 2 == 0) ? "0" : "1";

Context

StackExchange Code Review Q#51453, answer score: 8

Revisions (0)

No revisions yet.