Converting an integer to binary, hex, and octal

Question

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 <iostream>
#include <cstdlib>
#include <cstring>
#include <sstream>
#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 << "HEX: " << converter.to_hex(number) << std::endl;
 std::cout << "OCT: " << converter.to_oct(number) << std::endl;
 std::cout << "BIN: " << converter.to_bin(number) << std::endl;
 return 0;
}
int parse(int argc, char* argv[]){
 if (argc == 2){
 if ((strcmp(argv[1], "--help") == 0) || (strcmp(argv[1], "-h") == 0)
 || (strcmp(argv[1], "/?")) == 0){
 std::cout << "Usage: a R\\{0}\n";
 }
 else{
 int check;
 std::istringstream in(argv[1]);
 if ((in >> 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 <string>
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 << std::hex <<to_convert;
 ss >> result;
 return result;
}
std::string convert::to_oct(int to_convert){
 std::string result;
 std::stringstream ss;
 ss << std::oct << to_convert;
 ss >> result;
 return result;
}
std::string convert::to_bin(int to_convert){
 return int_to_bin(to_convert);
}

• ratchet freak: can you use lib funcs instead cause then it's just std::cout << std::hex << num; for hex
• user2225809: Good point. Next time I will try to keep my code as simple as possible.

Answer 1

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.

Answer 2

Instead of writing your own implementation for int_to_bin, consider exploiting std::bitset. You can simply do

std::string convert::int_to_bin(int number)
{
 const std::string result = std::bitset<32>(number).to_string();
 return result.substr(result.find("1", 0));
}

Note that std::bitset is not a dynamic beast, but you have to specify its size at compile time. My method gets rid of the leading zeros, assuming you don't want any. If you are working with larger integers, you could even write a template that builds bitsets of suitable size based on the size of the integer in question.

Answer 3

My only suggestion, other than what Jamal said, would be to add inline comments to the code to make the code easier to follow. Just a few, non-excessive comments walking the reader through each step would go a long way :)