This code is very, very simple, but I was wondering how it could be improved:
#include <iostream>
int main() {
std::string input;
std::getline(std::cin, input);
for(int i = 1; i <= input.length(); i++){
std::cout << input[input.length() - i];
}
std::cout << '\n';
return 0;
}
This code takes a string input
, and prints it backwards. For example:
This will be printed backwards.
Becomes:
.sdrawkcab detnirp eb lliw sihT
Given the simplicity of this code, is there a way to optimize it? I understand that since it is very short, it will run fast no matter what. However, I hate inefficiencies in code, and I want to help improve the way that I write code.
This is related to, but not copied directly from, this SO answer. (I only found this SO answer after writing the code)
6 Answers 6
One simple improvement would be to restructure the for()
loop:
for(int i = input.length() - 1; i >= 0; --i){
std::cout << input[i];
}
A simple decrement operation might lead to more efficient machine code than calculating input.length() - i
in every iteration.
Another improvement would be to use a more c++-ish way. But that means to change the string itself:
std::getline(std::cin,input);
std::reverse(std::begin(input),std::end(input));
std::cout << input << '\n';
Or using the reverse iterators in the loop:
for(auto it = input.rbegin(); it != input.rend(); ++it) {
std::cout << *it;
}
As I've mentioned in my comment, the most generic (and c++stylish) way to achieve that is to write your own adapter to achieve iterating a sequence in the reverse order. So I'm citing that solution here:
// ------------------------------------------------------------------- // --- Reversed iterable using namespace std; // for rbegin() and rend() template <typename T> struct reversion_wrapper { T& iterable; }; template <typename T> auto begin (reversion_wrapper<T> w) { return rbegin(w.iterable); } template <typename T> auto end (reversion_wrapper<T> w) { return rend(w.iterable); } template <typename T> reversion_wrapper<T> reverse (T&& iterable) { return { iterable }; }
Above taken from here.
-
\$\begingroup\$ Ah yes, this is what was suggested in the SO answer. Does restructuring it just improve the visual aspect of reading the code, or is it a common practice, or does it improve how it runs? \$\endgroup\$esote– esote2017年01月14日 08:59:47 +00:00Commented Jan 14, 2017 at 8:59
-
\$\begingroup\$ @Idempotence I'd say both. \$\endgroup\$πάντα ῥεῖ– πάντα ῥεῖ2017年01月14日 09:00:28 +00:00Commented Jan 14, 2017 at 9:00
-
1
-
2\$\begingroup\$ It does make a difference in the generated machine code, @Idempotence, at least on x86. Either of the ways πάντα ῥεῖ suggested are going to result in more efficient machine code being generated, and thus will theoretically be faster. (This won't be benchmarkable in practice, though, because the runtime will be dwarfed by the call to
std::cout
.) Looking at Clang's output, both of these alternatives result in comparable machine code, but the first one tends to result in slightly more efficient code inside the inner loop, whereas I'd say the iterator-based solution is more readable. \$\endgroup\$Cody Gray– Cody Gray2017年01月14日 11:19:15 +00:00Commented Jan 14, 2017 at 11:19 -
\$\begingroup\$ @πάνταῥεῖ Thanks that SO Q&A was very helpful. I also appreciate you updating your question, I had no idea that
std::reverse
,.rbegin()
, and.rend()
were even things! \$\endgroup\$esote– esote2017年01月14日 18:03:03 +00:00Commented Jan 14, 2017 at 18:03
another way:
#include <iostream>
#include <iterator>
#include <algorithm>
int main() {
std::string input;
std::getline(std::cin, input);
std::copy(input.rbegin(), input.rend(),
std::ostream_iterator<char>(std::cout));
std::cout << '\n';
return 0;
}
If I were going to do this often, I'd be tempted to make a little library extension:
#include <iostream>
#include <iterator>
#include <algorithm>
#include <string>
namespace notstd {
template<class Iter>
struct reverser
{
using value_type = typename std::iterator_traits<Iter>::value_type;
template<class Ch, class Traits>
std::basic_ostream<Ch, Traits>&
operator ()(std::basic_ostream<Ch, Traits>& os) const
{
std::copy(first, last,
std::ostream_iterator<value_type, Ch, Traits>(os));
return os;
}
Iter first, last;
template<class Ch, class Traits>
friend std::basic_ostream<Ch, Traits>&
operator <<(std::basic_ostream<Ch, Traits>& os, reverser eng)
{
return eng(os);
}
};
template<class Iter>
auto make_reverser(Iter first, Iter last)
{
return reverser<Iter> {first, last};
}
template<class Iter,
std::enable_if_t<std::is_same<std::remove_cv_t<typename std::iterator_traits<Iter>::value_type>, char>::value>* = nullptr
>
auto to_string(const reverser<Iter>& eng)
{
return std::string(eng.first, eng.last);
}
template<class Iter,
std::enable_if_t<std::is_same<std::remove_cv_t<typename std::iterator_traits<Iter>::value_type>, wchar_t>::value>* = nullptr
>
auto to_string(const reverser<Iter>& eng)
{
return std::wstring(eng.first, eng.last);
}
template<class Range>
auto reversed(Range&& range)
{
return make_reverser(std::rbegin(range), std::rend(range));
}
template<class Elem, std::size_t N,
std::enable_if_t<
std::is_same<char, std::remove_cv_t<Elem>>::value or
std::is_same<wchar_t, std::remove_cv_t<Elem>>::value
> * = nullptr
>
auto reversed(Elem (& range)[N])
{
static_assert(N > 0, "");
return make_reverser(std::rbegin(range) + 1,
std::rend(range));
}
}
int main(int argc, char **argv)
{
std::string input = argv[0];
// std::getline(std::cin, input);
std::cout << notstd::reversed(input) << '\n';
std::cout << notstd::reversed("Hello") << std::endl;
std::wcout << notstd::reversed(L"World") << std::endl;
std::cout << to_string(notstd::reversed("baloney")) << std::endl;
std::wcout << to_string(notstd::reversed(L"wide baloney")) << std::endl;
return 0;
}
example output:
don/gubed-dliub-ekamc/tcejorp/yalp/segdohr/sresU/
olleH
dlroW
yenolab
yenolab ediw
-
\$\begingroup\$ I think this is the best answer. \$\endgroup\$coderodde– coderodde2017年01月15日 11:14:06 +00:00Commented Jan 15, 2017 at 11:14
-
\$\begingroup\$ @coderodde Well, I think it mostly resembles that SO answer I have linked in my comment. Isn't that plagiarism to some degree? I refrained to extend my answer citing that generic code. But yes, it's probably the best way to do that using c++. \$\endgroup\$πάντα ῥεῖ– πάντα ῥεῖ2017年01月15日 19:22:56 +00:00Commented Jan 15, 2017 at 19:22
-
\$\begingroup\$ @πάνταῥεῖ I honestly have no idea, I didn't read the comment or the SO answer. I don't know whether it's fair to call proper use of the standard library, "plagiarism" :-) Certainly the concepts I have used here are not new (to you or me), but they may be new to other readers of this site. I hope I have covered enough concepts in this answer to provoke the OP to think more deeply about idiomatic and expressive code, plus highlight the options available in the standard library. \$\endgroup\$Richard Hodges– Richard Hodges2017年01月15日 19:59:40 +00:00Commented Jan 15, 2017 at 19:59
-
\$\begingroup\$ @Richard I didn't want to say your answer is inherently bad, don't let me be misunderstood ;-) ... \$\endgroup\$πάντα ῥεῖ– πάντα ῥεῖ2017年01月15日 20:02:03 +00:00Commented Jan 15, 2017 at 20:02
-
\$\begingroup\$ @πάνταῥεῖ no offence taken I assure you. I think we all contribute because we like to share knowledge and be appreciated for it. If we're inadvertently cluttering the site by repeating the same points, it's right to highlight that so we can undo the damage. \$\endgroup\$Richard Hodges– Richard Hodges2017年01月15日 20:05:21 +00:00Commented Jan 15, 2017 at 20:05
You can construct a string from a pair of iterators, so it seems to me that the method that's probably simplest is something on this order:
std::string my_string = "Hello, World!";
std::cout << std::string(my_string.rbegin(), my_string.rend()) << '\n';
Return from main
I'd also eliminate the return 0;
from main
. If you don't specify otherwise, main
returns 0
by default.
#include what you use
Your code uses std::string
, so it should #include <string>
to make it available.
To print any container that supports bidirectional iterators, which include not only plain ascii strings but also unicode strings, vectors, dequeues ...
// somecode.h
#include <utility>
template<typename Cont>
void print_bidir_cont(Cont& cont) noexcept(noexcept(std::cout))
{
auto lside{ c.rbegin() };
auto rside{ c.rend() };
while( lside < rside) std::cout << *lside++;
}
then use it like:
// main.cpp
#include "somecode.h"
#include <string>
#include <iostream>
#include <vector>
int main()
{
std::cout << "Please enter the string to revert: ";
std::string s;
std::cin >> s;
std::cout << std::endl;
print_bidir_cont(s);
std::cout << std::endl;
// ...
std::vector<double> v{ 1.0, 2.0, 3.0, 4.0 };
std::cout << "Reverting a vector too:";
print_bidir_cont(v);
std::cout << std::endl;
return 0;
}
...yet I still ask myself why do some have their strings stored backwards.
Using a simple reverse iterator is probably the simplest and most effective way of improving your code:
std::string my_string = "Hello World!";
for(auto IT = my_string.rbegin(); IT != my_string.rend(); IT++)
{
std::cout<<*IT<<std::endl;
}
I would avoid ostreams as 1) over complicates a very simple operation and 2) std::cout
fits you problem.
You could use the XOR swap algorithm: https://en.wikipedia.org/wiki/XOR_swap_algorithm
#include <iostream>
#include <string>
using namespace std;
string revStr(string str)
{
int end= str.length() - 1;
int start = 0;
while(start < end)
{
str[start] ^= str[end];
str[end] ^= str[start];
str[start] ^= str[end];
++start;
--end;
}
return str;
}
int main()
{
cout << revStr("Test");
return 0;
}