As Jamal implied, std::cin >> input
will only extract one word. Since the prompt says words, I would assume this is wrong, and you should use std::getline
like he suggested.
set
is vaguely named. What is set? Something like firstEven
might be a bit clearer (though then off course you have to default to true).
This would be clearer as int number = c - '0';
.
(Also, if you're wanting to be incredibly pedantic, it's worth noting that this depends on ASCII (or an ASCII compatible encoding) and is not portable. The standard functions (e.g. std::stoi
like MORTAL suggested) are guaranteed to be portable.)
While I'm at it with pednaticness...
if (isdigit(c)) {
That should be std::isdigits
(same with toupper/tolower). When a C header is included via the C++ version of the header, it's guaranteed that the functions will be present in the std
namespace, but it's only optional that they'll be present in the global namespace. Every major compiler exports them in the global namespace along with std
, but CodeReview brings out the pedant in me :p.
Even though you're wanting to make this shorter, I think it's a bit too dense already, and it could use a bit of breaking out into smaller pieces. Luckily, since none of the 3 things interact, you can pretty easily encapsulate each item and keep the code reasonable. Unfortunately... doing so will almost inevitably come at a pretty steep amount-of-code cost.
The best way to do this that I can think of (at least without getting way carried away) is functors (or functor-ish things anyway):
#include <cctype>
#include <iostream>
#include <string>
/** Mutlipy odd numbers together as they're encountered, or default to 0 if none are seen. */
class odd_mult {
private:
int val_ = 0;
public:
void operator()(int x) {
if (x % 2 == 0) { return; }
val_ = (val_ == 0) ? x : val_ * x;
}
int val() const { return val_; }
};
/** Subtract subsequent even numbers from the first one encountered, defaulting to 0. */
class even_sub {
private:
bool first = true;
int val_ = 0;
public:
void operator()(int x) {
if (x % 2 != 0) { return; }
if (first) {
val_ = x;
first = false;
} else {
val_ -= x;
}
}
int val() const { return val_; }
};
/** Build up a string such that each encountered letter has its case reversed (i.e. lower -> UPPER, UPPER -> lower) */
class letter_upcase {
private:
std::string val_;
public:
void operator()(char c) {
if (!std::isalpha(c)) { return; }
val_ += std::isupper(c) ? std::tolower(c) : std::toupper(c);
}
const std::string& val() const { return val_; }
};
int main() {
odd_mult odd;
even_sub even;
letter_upcase letters;
std::cout << "enter words: ";
std::string input;
std::getline(std::cin, input);
for (char c : input) {
if (std::isdigit(c)) { // This could go in the functors, but then it'd have to be repeated.
// Better would be a number_fn, but that would require templating to do correctly.
odd(c - '0'); // Creating a string to do the conversion seems a bit excessive. I'm willing to rely on ASCII.
even(c - '0');
}
letters(c);
}
std::cout << "odd: " << odd.val() << '\n';
std::cout << "even: " << even.val() << '\n';
std::cout << "letters: " << letters.val() << '\n';
}
All in all, I'm not very happy with this approach, even though I'm suggesting it :/. It's incredibly long compared to your code, though a dozen lines or so can be cut out by changing the classes to structs. Additionally, a few sacrifices had to be made in terms of design to avoid it growing even longer. I do still feel though that your code is a bit hard to follow (I had to resort to the English description to understand it after trying to read it a few times -- what if the English hadn't been there?), but it seems this might be a situation where it's just too much logic to try to cram into a small amount of code.
std::stoi
to convert string to int. also, no need to increment odd just initialize it to 1. \$\endgroup\$ – MORTAL Feb 24 '15 at 14:53std::cout << "odd: " << ((odd == 1) ? 0 : odd) << '\n';
\$\endgroup\$ – MORTAL Feb 24 '15 at 16:14