HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Extremely simple character manipulation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
extremelysimplecharactermanipulation

Problem

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 
#include 
#include 

int main() {
    int odd = 0;
    int even = 0;
    std::string letters{};
    bool set = false;
    std::cout > 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';
}

Solution

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 
#include 
#include 

/** 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.

Code Snippets

if (isdigit(c)) {
#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';
}

Context

StackExchange Code Review Q#82472, answer score: 6

Revisions (0)

No revisions yet.