Software Engineering
object-oriented c++ functional-programming refactoring
Updated Wed, 27 Jul 2022 03:27:17 GMT

Should I use classes instead of functions with a state needed for computation?


I have implemented the cows and bulls game in C++. The code:

#include <cstdio>
#include <cstdlib>
#include <ctime>
struct DigitMatches {
    int matches_in_right_positions;
    int matches_in_wrong_positions;
};
struct MatchesCounterData {
    int generated_number_left_digits[10];
    int guess_number_left_digits[10];
};
void CountNumberDigits(int (&counts)[10], int n) {
    while (n > 0) {
        counts[n % 10]++;
        n /= 10;
    }
}
int CountMatchesInRightPositions(MatchesCounterData *data, int generated_number, int guess_number) {
    int matches = 0;
    while (generated_number > 0 && guess_number > 0) {
        const int generated_number_digit = generated_number % 10;
        const int guess_number_digit = guess_number % 10;
        if (generated_number_digit == guess_number_digit) {
            matches++;
            data->guess_number_left_digits[generated_number_digit]--;
            data->generated_number_left_digits[generated_number_digit]--;
        }
        generated_number /= 10;
        guess_number /= 10;
    }
    return matches;
}
int CountMatchesInWrongPositions(MatchesCounterData *data, int guess_number) {
    int matches = 0;
    while (guess_number > 0) {
        const int guess_number_digit = guess_number % 10;
        int &generated_number_left_digits = data->generated_number_left_digits[guess_number_digit];
        int &guess_number_left_digits = data->guess_number_left_digits[guess_number_digit];
        if (generated_number_left_digits > 0 && guess_number_left_digits > 0) {
            matches++;
            generated_number_left_digits--;
            guess_number_left_digits--;
        }
        guess_number /= 10;
    }
    return matches;
}
DigitMatches CheckMatchedDigits(int generated_number, int guess_number) {
    MatchesCounterData matches_counter_data = {};
    CountNumberDigits(matches_counter_data.generated_number_left_digits, generated_number);
    CountNumberDigits(matches_counter_data.guess_number_left_digits, guess_number);
    DigitMatches matches;
    matches.matches_in_right_positions = CountMatchesInRightPositions(&matches_counter_data,
                                                                      generated_number, guess_number);
    matches.matches_in_wrong_positions = CountMatchesInWrongPositions(&matches_counter_data, guess_number);
    return matches;
}
void InitializeRandomNumberGenerator() {
    srand(time(nullptr));
}
int GenerateNumber(int min, int max) {
    return min + (rand() % (max - min + 1));
}
int InputNumber() {
    int n;
    scanf("%d", &n);
    return n;
}
int main() {
    InitializeRandomNumberGenerator();
    const int kTotalNumberOfDigits = 4;
    const int generated_number = GenerateNumber(1000, 9999);
    int attempts = 0;
    while (true) {
        const int guess_number = InputNumber();
        if (guess_number < 1000 || guess_number > 9999) {
            printf("Invalid number. Must be between 1000 and 9999.\n");
            continue;
        }
        const DigitMatches matches = CheckMatchedDigits(generated_number, guess_number);
        printf("%d cows, %d bulls\n", matches.matches_in_right_positions, matches.matches_in_wrong_positions);
        attempts++;
        if (matches.matches_in_right_positions == kTotalNumberOfDigits)
            break;
    }
    printf("You won! Attempts: %d\n", attempts);
    return 0;
}

The question. Should I use classes instead of functions with a state needed for computation? In my case, the functions are CountMatchesInRightPositions and CountMatchesInWrongPositions, the state is MatchesCounterData structure. It seems like it's reasonable to create a class for this functions, because a state is mostly about classes and because these functions should be private I think, because they must be called in a specific order and MatchesCounterData values must be pre-computed before their call. In view of the above, those functions should not be public due to the requirements for calling them. I think they should be encapsulated in another function that would follow the requirements. Currently, that function is CheckMatchedDigits. So, another implementation of that code but using a class:

#include <cstdio>
#include <cstdlib>
#include <ctime>
struct DigitMatches {
    int matches_in_right_positions;
    int matches_in_wrong_positions;
};
void CountNumberDigits(int (&counts)[10], int n) {
    while (n > 0) {
        counts[n % 10]++;
        n /= 10;
    }
}
class DigitsMatchChecker {
public:
    DigitMatches CheckMatchedDigits(int generated_number, int guess_number) {
        CountNumberDigits(this->generated_number_left_digits_, generated_number);
        CountNumberDigits(this->guess_number_left_digits_, guess_number);
        DigitMatches matches;
        matches.matches_in_right_positions = this->CountMatchesInRightPositions(generated_number, guess_number);
        matches.matches_in_wrong_positions = this->CountMatchesInWrongPositions(guess_number);
        return matches;
    }
private:
    int CountMatchesInRightPositions(int generated_number, int guess_number) {
        int matches = 0;
        while (generated_number > 0 && guess_number > 0) {
            const int generated_number_digit = generated_number % 10;
            const int guess_number_digit = guess_number % 10;
            if (generated_number_digit == guess_number_digit) {
                matches++;
                this->guess_number_left_digits_[generated_number_digit]--;
                this->generated_number_left_digits_[generated_number_digit]--;
            }
            generated_number /= 10;
            guess_number /= 10;
        }
        return matches;
    }
    int CountMatchesInWrongPositions(int guess_number) {
        int matches = 0;
        while (guess_number > 0) {
            const int guess_number_digit = guess_number % 10;
            int &generated_number_left_digits = this->generated_number_left_digits_[guess_number_digit];
            int &guess_number_left_digits = this->guess_number_left_digits_[guess_number_digit];
            if (generated_number_left_digits > 0 && guess_number_left_digits > 0) {
                matches++;
                generated_number_left_digits--;
                guess_number_left_digits--;
            }
            guess_number /= 10;
        }
        return matches;
    }
private:
    int generated_number_left_digits_[10] = {};
    int guess_number_left_digits_[10] = {};
};
void InitializeRandomNumberGenerator() {
    srand(time(nullptr));
}
int GenerateNumber(int min, int max) {
    return min + (rand() % (max - min + 1));
}
int InputNumber() {
    int n;
    scanf("%d", &n);
    return n;
}
int main() {
    InitializeRandomNumberGenerator();
    const int kTotalNumberOfDigits = 4;
    const int generated_number = GenerateNumber(1000, 9999);
    int attempts = 0;
    while (true) {
        const int guess_number = InputNumber();
        if (guess_number < 1000 || guess_number > 9999) {
            printf("Invalid number. Must be between 1000 and 9999.\n");
            continue;
        }
        DigitsMatchChecker digits_match_checker;
        const DigitMatches matches = digits_match_checker.CheckMatchedDigits(generated_number, guess_number);
        printf("%d cows, %d bulls\n", matches.matches_in_right_positions, matches.matches_in_wrong_positions);
        attempts++;
        if (matches.matches_in_right_positions == kTotalNumberOfDigits)
            break;
    }
    printf("You won! Attempts: %d\n", attempts);
    return 0;
}

On the one hand, now it looks better, because those functions are encapsulated in a class as well as the state now looks better as fields of the class. But on the other hand, the class is one-time usable. I mean that for another computation I need to create another instance of the class, because the state of the previous instance is not at beginning. Of course, I can create a method something like Reset that would reset the state to the beginning, but is it a good practice? Also, is it a good practice to have a class for one computation at all? Does this type of classes have a name?




Solution

There's nothing wrong with creating an instance of a class to be used for a short computation and then be thrown away. Long lived "entity" objects are not the only kind, there can also be ephemeral and temporary objects.

I can't say anything about your code (tl;dr) and it would probably be better to ask that part of the question on the code review site.