Customizing output in accordance to the input

Question

I wanted to know if I overdid it and maybe could have kept this code simpler and cleaner. I need to execute this assignment:

Write a program called multi_input.cpp that prompts the user to enter several integers in any combination of octal, decimal, or hexadecimal, using the 0 and 0x base suffixes; interprets the numbers correctly, and converts them to decimal form. Then your program should output the values in properly spaced columns like this:

 0x4 hexadecimal converts to 67 decimal
 0123 octal converts to 83 decimal
 65 decimal converts to 65 decimal

Now the code compiles and does what it's intended to do, but I still need to get into that programmer state of mind where you keep only the stuff that really matters, to the last letter (thus keeping the code simple, succinct and efficient). Showing me what I can get rid of and revise would bring me closer to achieving this.

#include"..\..\Header.h"
void print_number(const string& concatenation, const string& type) {
 istringstream s(concatenation);
 int number = 0;
 if(type == "hexadecimal") s >> hex >> number;
 if (type == "octal") s >> oct >> number;
 else s >> number;
 cout << concatenation << '\t' << type << "\tconverts to " << dec << number << " decimal.\n";
}
int main() {
 while (cin) {
 string da;
 cin >> da;
 if (da[0] == '0') {
 if (da[1] == 'x') print_number(da, "hexadecimal");
 else print_number(da, "octal");
 }
 else print_number(da, "decimal");
 }
 system("pause");
 return 0;
}

Header contains these lines (just in case):

 #include <algorithm>
#include <cmath>
#include <vector>
#include <iostream>
#include <sstream>
#include <fstream>
#include <string>
using namespace std;
void error(const string& a) {
 throw runtime_error(a);
}

• πάντα ῥεῖ: To start with:: using namespace std; in a header file is a terrible idea.

Answer 1

Let's start off with a few general hints and tips:

1. First of all, as πάντα ῥεῖ already stated in the comments, using using namespace std is not a good idea, and doing that in a header if a lot worse. You're begging to introduce subtle bugs into your code as time goes on and things are added to your project and the standard libraries. Typing std:: a few times is not going to kill you, I'm sure.
2. #include"..\..\Header.h" is a code smell; relative paths in an include often indicate that your project structure is not good or your compile flags are not complete. You should specify all necessary include directories in the compiler invocation command line.
3. system("pause"); is not portable and should thus not be used unless absolutely necessary. Is this the case here? I don't think so. Make users of other operating systems (i.e. operating systems that aren't Microsoft Windows) happy and remove that line.
4. What happens if I enter the string "0" to your program? Whoops, undefined behavior! Why? if (da[1] == 'x') requires da to be at least two characters long, which in my case it simply doesn't happen to be. You should add a check to verify that this condition is satisfied.
5. Header.h seems redundant and superfluous. All of those includes should be at the top of your implementation file, not in the header. All includes that are not used (e.g. fstream) should be removed. The function error doesn't serve any real purpose, except obscuring the fact that something throws and exception, which can lead to people wondering where those exceptions come from. Also it isn't used, so it should be removed as well.

---

Now, let's talk about print_number in particular. One issue I have with this function is that type should never, ever be a string. You're wasting performance and causing binary bloat, when the fix would be to simply add an enum class with three members HEX, OCT and DEC. The other big issue is that this function limits its own usability by necessarily writing to std::cout. This has the potential to cause people using that function and wanting to separate outputs much grief, and is easily circumvented by having the function take a std::ostream& as parameter and writing to it instead. When invoking the function, you then simply pass std::cout as an argument.

Finally, this function does a little bit too much; two completely separate tasks, to be precise. One is taking a number in a given format and converting it to decimal, and the other is actually printing that number in a nicely formatted way. You should thus split it up into two functions: One which does the extraction, and one which does the output.

• C.H.: I am aware that the code is quite patchy, how would you recommend I work improving my efficiency on that end ? Can you elaborate about the enum class option and get into a little detail please?
• Ben Steffan: @ChenHaviv The solution is quite simple. You have an enum class called Base (or whatever name you find fitting), and instead of passing type as a string, you pass an enum value instead. In the function, you switch on the type that you've got and do the appropriate action. Something like this.
• Incomputable: @BenSteffan, I tried to implement extraction function, but the best I was able to come up with is triple pass algorithms (I wanted it bulletproof), one over the stream, twice over buffer. I would be really interested in your implementation if you have the time. I can think of others, but the implementation will be quite ugly (I tried two ideas: one with state machine, and second with branching on the prefix).
• Ben Steffan: @Incomputable Well, if you really want to have something bulletproof, you're going to need about that many passes (depending on your definition of pass). I'll try writing an extraction function myself tomorrow if I have the time.

Answer 2

Streams already have a mode in which they detect the base of a number from the prefix, exactly as desired here. To get to it, you set the base for the stream to 0.

So, a mildly simplified version of your code could look something like this:

template <class T, class U>
T lexical_cast(U const &in) {
 std::stringstream s(in);
 T out;
 s >> std::setbase(0) >> out;
 return out;
}
int main() { 
 std::string s;
 while (std::cin >> s) 
 std::cout << s << " converted to decimal is: " << lexical_cast<int>(s) << "\n";
}

Other points:

1. concatentation is a pretty awful name. Where you use it, it looks to me like it's simply the input to the function; lacking a reason to do otherwise, I'd consider naming it input.
2. As Ben already noted, you're use of a string to indicate the input format makes little sense.
3. Likewise, calling that a type instead of a format (or something on that order) gives a false impression about what it really means.
4. The primary reason for using a header is when you need to share some declarations between translation units. It doesn't make sense in this case.
5. Any loop of the form while (cin) (or while (cin.good()), etc.) is essentially guaranteed to be incorrect. You almost always want to attempt to read something, and test whether that attempt at reading succeeded (as I've shown in my example above).