38
\$\begingroup\$

A more proper version of this utility can be found at the following link here: Giveth me thy easier user input in C++ - follow up.


I've always been a little bothered by the fact, that in order to get user input in C++, one had to use up to three lines of code in an ugly manner like the below to get user input for a specific type, with a prompt:

int user_input;
std::cout << ">> ";
std::cin << user_input;

So, in order to make this process easier, I've created an easy_input function which allows for the user to specify a type, a prompt, and set user input to a variable, all in one line.

easy_input.h

#ifndef EASY_INPUT_H_
#define EASY_INPUT_H_
#pragma once

#include <iostream>
#include <string>

// We're simply "re-defining" the standard namespace
// here so we can patch our easy_input function into
// it for the user's sake.
namespace std
{
    template <typename TInput>
    TInput easy_input(const std::string& prompt);
}

/**
 * This function serves as a wrapper for obtaining
 * user input. Instead of writing three lines every
 * time the user wants to get input, they can just
 * write one line.
 * @param {any}    TInput - The type of the input.
 * @param {string} prompt - The prompt to use for input.
 * @returns - Whatever the user entered.
 */
template <typename TInput>
TInput std::easy_input(const std::string& prompt)
{
    TInput user_input_value;
    std::cout << prompt;
    std::cin >> user_input_value;
    return user_input_value;
}

#endif

main.cpp (tests)

#include <iostream>
#include <string>
#include "easy_input.h"

int main()
{
    const std::string user_input1 = std::easy_input<std::string>(">> ");
    std::cout << user_input1 << "\n";

    const int user_input2 = std::easy_input<int>(">> ");
    const int user_input3 = std::easy_input<int>(">> ");
    std::cout << user_input2 + user_input3 << "\n";
}

I'd (preferably) like to know the following things:

  • Am I using templates appropriately? I feel like I might have done something wrong here in the process.
  • Is there anything that can be improved performance-wise?
  • Is there a need for include guards?
  • Is it okay to patch easy_input into std without problems? Is this a good practice?
  • Is there anything else that's glaringly wrong?
\$\endgroup\$
64
+500
\$\begingroup\$

namespace std

Others have said this already but it's important enough to be repeated: Don't put your own definitions into namespace std. It's undefined behavior.

About the only thing I'm aware of that you may put into namespace std are specializations for templates already defined by the standard. So, for example, if you have

struct MyType
{
  int a;
};

inline constexpr bool
operator==(const MyType& lhs, const MyType& rhs) noexcept
{
  return lhs.a == rhs.a;
}

then you may do

#include <functional>  // std::hash

namespace std
{

  template <>
  struct hash<MyType>
  {
    using argument_type = MyType;
    using result_type = std::size_t;

    result_type
    operator()(const argument_type& mt) const noexcept
    {
      return mt.a;
    }
  };

}

so you can, say, use MyType as key in a std::unordered_map.

The whole point of namespaces is to separate stuff. So put your own stuff into your own namespace. For example, Boost has its stuff in namespace boost and sub-namespaces thereof. You could use namespace ethan_bierlein or something. In other languages, it is common practice to use one's domain as in package com.example.myproduct (assuming you own example.com) but I haven't seen this practice in C++ yet.

Correctness

Consider the following program.

int
main()
{
  const auto age = easy_input<int>("How old are you? ");
  std::cout << "Hello, " << age << " year old!\n";
}

If compiled and run as this

$ ./a.out
How old are you? 17
Hello, 17 year old!

everything seems fine. However, if the user inputs nonsense,

$ ./a.out
How old are you? don't care
Hello, 0 year old!
$

the result is probably not what you'd expect from a well-designed user interface. However, it gets worse.

int
main()
{
  const auto ounces = easy_input<int>("How many ounces of beer dou you want? ");
  const auto age = easy_input<int>("How old are you? ");
  //std::cout << "ounces = " << ounces << ", age = " << age << "\n";
  if (age >= 18)
    std::cout << "Here are your " << ounces << " ounces of beer.\n";
  else
    std::cout << "Sorry, you're not old enough.\n";
}

In action:

$ ./a.out
How many ounces of beer dou you want? too many
How old are you? Here are your 0 ounces of beer.
$

Don't worry if you cannot reproduce the results of the above two examples result of the second example. It's undefined behavior so anything (including but not limited to your cat getting pregnant) could happen.

What's the reason for this?

There are two problems. First, in

TInput user_input_value;        // (1)
std::cout << prompt;            // (2)
std::cin >> user_input_value;   // (3)
return user_input_value;        // (4)

if TInput is a builtin type like int, the variable user_input_value is not initialized on line 1. If the input on line 3 succeeds, the value is set to the input, which is fine. However, if the input is invalid, no value will be assigned and you'll return the uninitialized value. If the stream is good(), the extraction operator on line 3 will either successfully extract and assign the value or, if invalid input is given, set the failbit and assign 0. Therefore, if the first input is invalid, 0 will be returned (for ounces) and the failbit set. Then, on the second entry, nothing is assigned to user_input_value and an uninitialized int returned (for age). This results in undefined behavior.

Running the above examples second (“too many ounces of beer”) example through a tool like Valgrind can unveil the error. (To my surprise, neither ASan nor UbSan were able to detect the error.)

I originally thought that if input fails for whatever reason, the destination value would never be changed. This seemed to be the case but apparently was changed in C++11 such that now 0 is assigned for invalid input provided the stream was good() to begin with. Thanks to Mooing Duck for discovering this (see comments).

The seemingly simple fix to this problem is to use value-initialization for user_input_variable.

TInput user_input_value {};

Since your question is tagged with C++14, we can now at least use one C++11 feature (uniform initialization) with pride.

However, while this “solution” fixes the undefined behavior, it still has its issues. If the user enters invalid input, it should probably be yelled at and not 0 (or whatever a default-constructed TInput is) returned silently. So you should really check the stream after the operation.

TInput user_input_value {};  // value-initialization not strictly needed any more
std::cout << prompt;
if (std::cin >> user_input_value)
  return user_input_value;
throw std::istream::failure {"bad user input"};

You could have the same effect by setting the exceptions mask of std::stdin but this would also affect other uses of std::cin even outside your function so it might surprise users of your function. I'd consider a utility function messing with the mask bad.

Some people will argue that a user inputting invalid data is by no means an “exceptional” event so throwing an exception is inappropriate. If you're like them, you might prefer returning an std::experimental::optional<TInput> result. Unfortunately, it is not standard yet but many implementations support it and there is a readily available version in Boost.Optional.

But this is still not as useful as it could be. Consider the “beer” example again.

$ ./a.out
How many ounces of beer dou you want? 4 2
How old are you? Sorry, you're not old enough.
$

The first invocation of your function happily consumes the 4, stops at the white-space and leaves the 2 on the stream. The second invocation then sees the 2 readily available and without waiting for any user input, immediately and equally happily returns 2.

cr_oag has suggested you address this by calling std::cin.ignore. However, I don't think that this is the ideal solution. If you ask the user how many ounces of beer she wants and she inputs 4 2, it is unlikely she really meant 4. It would be better to treat this as an error and ask for clarification.

There is another, closely related, problem with your current version. Consider how this seemingly reasonable program…

int
main()
{
  const auto name = easy_input<std::string>("What's your name? ");
  std::cout << "Hello, " << name << "!\n";
}

…behaves not quite as expected.

$ ./a.out
What's your name? Ethan Bierlein
Hello, Ethan!
$

While it might create a warm and cozy feeling to be called with the first name by the program, most of us would probably consider this a bug.

For least surprise, I recommend that you always treat user input line-oriented: Read a single, but complete, line, and treat its entirety as one input. You can easily do this via std::getline. You can then try to convert that string into the target type and see if it has any characters remaining.

std::cout << prompt;
std::string line {};
if (!std::getline(std::cin, line))
  throw std::istream::failure {"I/O error"};
std::istringstream iss {line};
TInput value {};
if (!(iss >> value) || !iss.eof())
  throw std::istream::failure {"bad user input"};
return value;

This is still not ideal. While it behaves as expected for, say, numbers, the example asking for a name would now reject any input longer than one word. Not quite what we want. You would need some template meta-programming to special-case string input. But explaining how that would work is too much to go through in a code review.

As a matter of fact, there is already a function template in Boost.Lexical_Cast that does this (and more) for you (#include <boost/lexical_cast.hpp>).

std::string line {};
std::cout << prompt;
if (!std::getline(std::cin, line))
  throw std::istream::failure {"I/O error"};
return boost::lexical_cast<TInput>(line);

Portability

user86418 has already mentioned that #pragma once is not portable but if you use it, the #include guards become redundant. On the other hand, #pragma once can be more efficient than #include guards because the preprocessor can immediately stop processing the file while with #include guards, it has to process the whole header anyway to find out where the matching #endif is. So if you want the best of both worlds, you could use something like this.

#if HAVE_PRAGMA_ONCE
#pragma once
#endif

#ifndef EASY_INPUT_H
#define EASY_INPUT_H

// ...

#endif

Your users could then compile with -DHAVE_PRAGMA_ONCE to eventually speed up their compilations while still being portable to implementations that don't have #pragma once. If you're used to using GNU Autoconf, you'll be very familiar with those HAVE_${FEATURE} macros.

Another option is to use tricks like

#ifdef __GNUC__
#pragma once
#endif

(GCC defines __GNUC__) but I don't like this kind of being clever very much and prefer having the user have the last word.

As Mooing Duck points out, compilers ought to ignore unknown #pragmas. However, it is still a good idea to make them conditional. For example, if you compile with -Werror=unknown-pragmas (And you should, because it is enabled by -Wall -Werror.), GCC will reject code with unknown #pragmas. This is conforming as in standard configuration, GCC ignores them gracefully as it is supposed to. Your library should not force your users to use less rigorous warning levels. Such “noisy” or “unclean” libraries are very annoying, to say the least.

Genericity

Others have suggested making the stream to read from a parameter or accepting other prompts than strings. I don't think that this would add much value as I've yet to see a program that needs to read interactive user input from anything but standard input. Some lost souls might want to read from std::wcin, though. Those would probably also be interested in a std::wstring prompt that would be streamed to std::wcout. Eventually, there could be good reasons to print the prompt to standard error output (for example if standard output is to be redirected to some file).

Having an overload that uses no (or a default) prompts seems useful but anything else I think is superfluous. A default parameter comes in handy here.

template <typename TInput>
TInput easy_input(const std::string& prompt = "");

Yes, you'll needlessly construct a temporary std::string if you call the function with a string literal but so what (see next section)? And I don't think that using, say, an int as prompt would be a very frequent use-case.

Performance

Is there anything that can be improved performance-wise?

Maybe, but don't worry about it. You are asking for user input. It would take very bad code for an input function that would be slow enough to make any difference compared to how long humans need to read, think and type.

Going Fancy

If you want to make your program fancy, consider adding line-editing support. It is very annoying for a user, having to delete an almost completely typed answer only because you realize that you have a typo in the first few characters. The GNU Readline library provides a de-facto standard mechanism for this. With GNU Readline there also goes GNU History which allows a user to re-fetch a previous answer using the arrow-keys. This is very handy if you have to give the same answer or similar answers many times. Of course, you should make the feature optional because your users might not have installed the library.

Also note the license requirements; quoting the project website:

Readline is free software, distributed under the terms of the GNU General Public License, version 3. This means that if you want to use Readline in a program that you release or distribute to anyone, the program must be free software and have a GPL-compatible license.

If you don't publish your software at all, you're also fine. So you can always experiment around freely for your personal learning and private usage.

There might be other libraries providing similar line-editing support but I don't know any of them.

Williham Totland commented that there is another free software library for line-editing, Linenoise. While checking its website, I also found the Editline Library (libedit) (also free software). I've never used either of them.

Style

Besides the fact that you shouldn't declare stuff in namespace std, you have put the DocString alongside with the implementation. If you decide to separate declaration and definition, the DocString should go with the declaration because that's what is important for your users.

I assume you wanted to write a DocString that can be processed by Doxygen. If so, note that you should use @tparam to document template arguments.

\$\endgroup\$
  • 6
    \$\begingroup\$ Wow! This is a very thorough answer. Thank you! \$\endgroup\$ – Ethan Bierlein Oct 9 '15 at 15:17
  • 5
    \$\begingroup\$ @EthanBierlein It got somewhat longer than expected. I'm glad you found it useful. \$\endgroup\$ – 5gon12eder Oct 9 '15 at 15:21
  • 10
    \$\begingroup\$ I love that this is like a 40 paragraph code review that more or less boils down to "just don't do it." \$\endgroup\$ – Barry Oct 9 '15 at 15:25
  • 3
    \$\begingroup\$ IIRC, #pragma once doesn't need to be guarded, because the C spec says that unknown pragmas should be ignored. Also, virtually every compiler has it. \$\endgroup\$ – Mooing Duck Oct 10 '15 at 0:39
  • 2
    \$\begingroup\$ This answer has been selected as the winner of Best of Code Review 2015 — Best newcomer (answer) category. Congratulations! \$\endgroup\$ – 200_success Jan 16 '16 at 0:45
11
\$\begingroup\$

namespace std

is reserved. Don't add stuff to it. Especially since there's no reason to in this case. Just put it in your own namespace.

unnecessary string usage

There isn't actually any reason to take the prompt as a std::string. You could have kept it as a character literal. Really, anything streamable is good enough, so this could just be:

template <typename TInput, typename Prompt>
TInput easy_input(Prompt&& prompt)
{
    TInput user_input_value;
    std::cout << std::forward<Prompt>(prompt);
    std::cin >> user_input_value;
    return user_input_value;
}

To cin or not to cin

What if I want to input from somewhere else? Let's provide that opportunity:

template <typename TInput, typename Prompt>
TInput easy_input(Prompt&& prompt, std::istream& is = std::cin)
{
    TInput user_input_value;
    std::cout << std::forward<Prompt>(prompt);
    is >> user_input_value;
    return user_input_value;
}

To prompt or not to prompt

I guess you may as well provide an overload with no prompting:

template <typename TInput>
TInput easy_input(std::istream& is = std::cin)
{
    TInput user_input_value;
    is >> user_input_value;
    return user_input_value;
}

template <typename TInput,
          typename Prompt,
          typename = decltype(std::cout << std::declval<Prompt>())>
TInput easy_input(Prompt&& prompt, std::istream& is = std::cin)
{
    std::cout << std::forward<Prompt>(prompt);
    return easy_input<TInput>(is);
}

static error checking

This requires that TInput be default-constructible, so we should be nice to our users and assert that:

static_assert(std::is_constructible<TInput>{}, "!");

runtime error checking

What if the input streaming fails? How do we indicate that? Maybe we just don't and leave it as "easy".

\$\endgroup\$
  • \$\begingroup\$ What does the double ampersand (&&) after Prompt mean? \$\endgroup\$ – Ethan Bierlein Oct 9 '15 at 2:43
  • \$\begingroup\$ @EthanBierlein In this case, it's a forwarding reference. It will match either an lvalue or an rvalue. \$\endgroup\$ – Barry Oct 9 '15 at 2:47
  • \$\begingroup\$ I think the stream should always be the first argument. That's a convention that seems to be used universally across programming languages, so it's IMHO a bad idea to break it without need. \$\endgroup\$ – celtschk Oct 10 '15 at 15:28
11
\$\begingroup\$

I've actually implemented the same functionality in the past, so here are my comments.

Wrong behaviour?

It depends on what you can consider wrong. Consider running your own example and inputting a a. The 2nd a will be left in the stream buffer and will be automatically assigned to your 2nd variable. This clearly causes weird behaviour, but it is the same behaviour that occurs when normally using std::cin.

Suggestions

The following are what I consider to be useful features for such an utility function.

Better interface

In order to provide a nicer interface, you could instead read a single value from your stream and then discard anything else that's been left in the stream buffer; calls to easy_input<T>() will always return one single value and successive calls won't be forced to take what's left in the stream buffer.

Example

template <typename T>
T get( std::istream& is = std::cin )
{
    T value{};
    is >> value;
    is.ignore( std::numeric_limits<std::streamsize>::max(), '\n' );
    return value;
}

Specialization for special types

You can also provide a way to read whole lines through template specialization or through a different function, since it would be useful for strings (reading a phrase, for example).

More functionality

You can provide a way to obtain values based on a predicate. So that it can be a lot easier for people to get valid values. For example, I want to get easy input of an integer that is between 0 and 10 from an user.

Example

template <typename T, typename Predicate>
T get( Predicate pred, std::istream& is = std::cin )
{
    T value{};
    while ( !pred( value = get<T>() ) ) // uses regular get<T>()
    {}
    return value;
}

Which can then be called as follows, to get an integer in range [0, 10]:

int int_between_0_and_10 = get<int>( [] ( int i ) { return i >= 0 && i <= 10; } );

You can expand on this by providing functions that use the "Error hiding pattern". That is that they return a bool to indicate whether the input operation succeeded or not, while the result is stored in a reference parameter.

Other

std::cin isn't the only object that can be streamed from. For example, you can also stream from files. You should provide a way for users of your function to specify what they want to stream from (this can be as simple as having a std::istream& parameter.

\$\endgroup\$
3
\$\begingroup\$

Is there a need for include guards?

Yes, but #pragma once is redundant and also not portable.

Is it okay to patch easy_input into std without problems? Is this a good practice?

Don't put stuff into namespace std;. It's usually undefined behavior except in a few cases but even then it's not remotely necessary in this case.

Is there anything else that's glaringly wrong?

Yes, your "easy_input" actually doesn't make anything easier. It's just more verbose. The only improvement for such trivial code I can see is to add error-checking:

if (!(std::cin >> user_input_value))
{
    // error
}

FYI there's nothing in the code remotely related to C++14 or template meta programming.

\$\endgroup\$
2
\$\begingroup\$

First, adding the easy_input to std namespace is UB. The only additions you are allowed to make to the std namespace are template specializations for your own types (that is, if you create a type called MyClass, you can specialize swap for it in the std namespace).

Second, you hard-coded the input and output streams; Consider instead doing something like this:

namespace stdex // not std namespace
{

    /**
     * This function serves as a wrapper for obtaining
     * user input. Instead of writing three lines every
     * time the user wants to get input, they can just
     * write one line.
     * @param {any}    Input - The type of the input.
     * @param {any}    Prompt - The prompt to use for input.
     * @returns - Whatever the user entered.
     * @throws std::runtime_error on bad input
     */
    template <typename Input, typename Prompt>
    Input easy_input(const Prompt& prompt,
        std::istream& in = std::cin)
    {
        auto user_input_value = Input{}; // initialize value on creation
        auto tied_stream = in.tie();
        if(tied_stream)
            (*tied_stream) << prompt;
        if(!(in >> user_input_value))
            throw std::runtime_error{ "easy_input: Bad input" }; // handle errors
        return user_input_value;
    }
}

Client code:

int main()
{
    auto a = stdex::easy_input<std::string>("name: ");

    std::istringstream in{ "aaa bbb" };
    auto b = stdex::easy_input<std::string>(">> ", in); // will not print a prompt (in is not
                                                        // tied to any output stream)

    std::ostringstream out;
    in.tie(&out);
    auto c = stdex::easy_input<std::string>(">> ", in);
    assert(">> " == out.str());

    try
    {
        auto d = stdex::easy_input<int>(">> ", in);
    }
    catch(const std::runtime_error&)
    {
        // code will get here because in is at EOS
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.