Skip to main content
added 829 characters in body
Source Link
Deduplicator
  • 19.9k
  • 1
  • 32
  • 65

G. Sliepen already took care of the big picture issues, where you get the most bang for the buck.

Still, there are a few issues with the code aside from using a sub-optimal algorithm:

  1. You should consider std::string_view for the string-argument, and for getting a temporary slice of a long-lived string.
    Dynamic allocation is pretty expensive, and best avoided, both on calling a function if the input might not be in the desired format, and in the function itself.
    See "What is string_view?" and "How exactly is std::string_view faster than const std::string&?" for more details.

  2. Now that the functions no longer allocate memory, or contain any other potential exception-thrower, mark them noexcept so everyone knows (and the compiler enforces) that it won't throw. It won't do much of anything here, but is good documentation, informs the compiler if it only knows the declaration, and can be important later with using templated code consuming it for best performance and highest exception-safety guarantees.

  3. Also, mark them constexpr while you are at it, to allow use in constant expression, and encourage evaluation at compile-time and allow forcing that. AlsoThat is also a best-practice thing not actually changngchanging much of anything for your example program itself.

  4. You use std::cout twice (whyever you don't chain itpush all togetherthe output into it in a single expression escapes me there, but that can be argued either way), and std::endl once. TheWriting (and keeping in mind) those two using-declaration doesn't look like a good investment, evendeclarations costs more than prefixing the uses with std::. Even if you really dislike writing std::, you don't write it less often.

  5. Don't force flushing a stream unless you really mean it, as it flushes performance down the drain. And in that casestd::endl outputs a newline and then flushes, stream << std::endl being exactly equivalent to stream << '\n' << std::flush. Thus if you really have to, better be explicit and use std::flush.
    Flushing is normally expensive buzy-workSee "What is the C++ iostream endl fiasco?" for more detail.

G. Sliepen already took care of the big picture issues, where you get the most bang for the buck.

Still, there are a few issues with the code aside from using a sub-optimal algorithm:

  1. You should consider std::string_view for the string-argument, and for getting a temporary slice of a long-lived string.
    Dynamic allocation is pretty expensive, and best avoided, both on calling a function if the input might not be in the desired format, and in the function itself.

  2. Now that the functions no longer allocate memory, or contain any other potential exception-thrower, mark them noexcept. It won't do much of anything here, but is good documentation, informs the compiler if it only knows the declaration, and can be important later with using templated code consuming it for best performance and highest guarantees.

  3. Also, mark them constexpr while you are at it, to allow encourage evaluation at compile-time and allow forcing that. Also a best-practice thing not actually changng much of anything for your example program itself.

  4. You use std::cout twice (whyever you don't chain it all together escapes me), and std::endl once. The using-declaration doesn't look like a good investment, even if you dislike writing std::.

  5. Don't force flushing a stream unless you really mean it. And in that case, better be explicit and use std::flush.
    Flushing is normally expensive buzy-work.

G. Sliepen already took care of the big picture issues, where you get the most bang for the buck.

Still, there are a few issues with the code aside from using a sub-optimal algorithm:

  1. You should consider std::string_view for the string-argument, and for getting a temporary slice of a long-lived string.
    Dynamic allocation is pretty expensive, and best avoided, both on calling a function if the input might not be in the desired format, and in the function itself.
    See "What is string_view?" and "How exactly is std::string_view faster than const std::string&?" for more details.

  2. Now that the functions no longer allocate memory, or contain any other potential exception-thrower, mark them noexcept so everyone knows (and the compiler enforces) that it won't throw. It won't do much of anything here, but is good documentation, informs the compiler if it only knows the declaration, and can be important later with using templated code consuming it for best performance and highest exception-safety guarantees.

  3. Also, mark them constexpr while you are at it, to allow use in constant expression, and encourage evaluation at compile-time. That is also a best-practice thing not actually changing much of anything for your example program itself.

  4. You use std::cout twice (whyever you don't push all the output into it in a single expression escapes me there, but that can be argued either way), and std::endl once. Writing (and keeping in mind) those two using-declarations costs more than prefixing the uses with std::. Even if you really dislike writing std::, you don't write it less often.

  5. Don't force flushing a stream unless you really mean it, as it flushes performance down the drain. std::endl outputs a newline and then flushes, stream << std::endl being exactly equivalent to stream << '\n' << std::flush. Thus if you really have to, better be explicit and use std::flush.
    See "What is the C++ iostream endl fiasco?" for more detail.

Source Link
Deduplicator
  • 19.9k
  • 1
  • 32
  • 65

G. Sliepen already took care of the big picture issues, where you get the most bang for the buck.

Still, there are a few issues with the code aside from using a sub-optimal algorithm:

  1. You should consider std::string_view for the string-argument, and for getting a temporary slice of a long-lived string.
    Dynamic allocation is pretty expensive, and best avoided, both on calling a function if the input might not be in the desired format, and in the function itself.

  2. Now that the functions no longer allocate memory, or contain any other potential exception-thrower, mark them noexcept. It won't do much of anything here, but is good documentation, informs the compiler if it only knows the declaration, and can be important later with using templated code consuming it for best performance and highest guarantees.

  3. Also, mark them constexpr while you are at it, to allow encourage evaluation at compile-time and allow forcing that. Also a best-practice thing not actually changng much of anything for your example program itself.

  4. You use std::cout twice (whyever you don't chain it all together escapes me), and std::endl once. The using-declaration doesn't look like a good investment, even if you dislike writing std::.

  5. Don't force flushing a stream unless you really mean it. And in that case, better be explicit and use std::flush.
    Flushing is normally expensive buzy-work.