Extremely simple character manipulation

Question

Given a string, this code

• Multiplies odd numbers
• Subtracts even numbers, do nothing if only one even number is found
• Turns uppercase letters to lowercase and vice versa

I think this code is too long for those operations. There are certainly better ways of doing this. How can I make this shorter, better and faster? And what are better ways of doing this?

#include <string>
#include <cctype>
#include <iostream>

int main() {
    int odd = 0;
    int even = 0;
    std::string letters{};
    bool set = false;
    std::cout << "enter words: ";
    std::string input;
    std::cin >> input;
    for (char c : input) {
        if (isdigit(c)) {
            int number = c-48;
            if (number % 2 == 0) {
                if (!set) {
                    even = number;
                    set = true;
                } else {
                    even -= number;
                }
            } else {
                if (odd == 0) {
                    odd++;
                }
                odd *= number;
            }
        } else if (isalpha(c)) {
            letters += (isupper(c)) ? tolower(c) : toupper(c);
        }
    }
    std::cout << "odd: " << odd << '\n';
    std::cout << "even: " << even << '\n';
    std::cout << "letters: " << letters << '\n';
}

Answer 1

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.

Answer 2

You don't need to declare the std::string with {} since it's always declared as an empty string:

std::string letters;

It's also more common to input into an std::string using std::getline():

std::getline(std::cin, input);

Answer 3

Those extra nested conditionals can be avoided, by using, @MORTAL's advise for the odd numbers and simply taking a count for the even numbers:

#include <string>
#include <cctype>
#include <iostream>

int main()
{
    int odd = 1;
    int even = 0;
    int evenCount = 0;
    std::string letters = "";
    std::cout << "enter words: ";
    std::string input;
    std::cin >> input;
    for (char c : input)
    {
        if (isdigit(c))
        {
            int temp = c - '0';
            if (temp % 2 == 0)
            {
                even -= temp;
                evenCount++;
            }
            else
            {
                odd *= temp;
            }
        }
        else if (isalpha(c))
        {
            letters += (isupper(c)) ? tolower(c) : toupper(c);
        }
    }
    std::cout << "odd: " << (odd == 1 ? 0 : odd) << '\n';
    std::cout << "even: " << (evenCount > 1 ? even : 0) << '\n';
    std::cout << "letters: " << letters << '\n';
}

You do end up with extra conditionals on the output, but they are only checked once, not for every digit.

Answer 4

Perhaps an easier compromise than Corbin's three classes is having just one to encapsulate most state. This separates the iteration and printing from the iteration step, and IMHO comes out cleaner.

#include <cctype>
#include <iostream>
#include <string>

class LetterMachine {
public:
    int multiplied = 0;
    int subtracted = 0;
    bool seen_subtract = false;
    std::string letters;

    void push(int num) {
        if (num % 2) {
            multiplied = (multiplied ? multiplied : 1) * num;
        } else if (seen_subtract) {
            subtracted -= num;
        } else {
            subtracted = num;
            seen_subtract = true;
        }
    }
    void push(char letter) {
        letters += std::isupper(letter) ? std::tolower(letter) : std::toupper(letter);
    }
};

int main() {
    LetterMachine machine;

    std::cout << "enter words: ";
    std::string input;
    std::getline(std::cin, input);

    for (char character : input) {
        if (std::isdigit(character)) {
            machine.push((int)(character - '0'));
        } else if (std::isalpha(character)) {
            machine.push(character);
        }
    }

    std::cout << "odd: " << machine.multiplied << '\n';
    std::cout << "even: " << machine.subtracted << '\n';
    std::cout << "letters: " << machine.letters << '\n';
}