patterncppMinor
Converting an integer to binary, hex, and octal
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:
1.conversion_header.h:
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
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.
-
I don't see the need for
-
You don't need to use
Header:
-
There's no need for
-
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:
can just be made into a single-ternary statement:
Better yet, since
-
The local variables in
-
You don't need your own reverse function; just use
-
As @rachet freak has mentioned in the comments, you can just use
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
ifstatement is executed, it displays a message and returns0. If theelseis executed, it returns a convertedinteger. 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.