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

Count of Maximum - CodeChef

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

Problem

I'm doing an easy problem in codechef. Here's the problem statement for maxcount:


Given an array A of length N, your task is to find the element which
repeats in A maximum number of times as well as the corresponding
count. In case of ties, choose the smaller element first.

OK, I cheated. I used std:map because I want to play with it. Please review my code:

#include
#include

int main() {
    int test_cases{};
    std::cin >> test_cases;
    for (auto i = 0; i  count;
        std::size_t size{};
        std::cin >> size;
        int x{};
        for (std::size_t i = 0; i > x;
            count[x] = ++count[x];
        }
        int number{};
        int numberCount{};
        for (auto& i : count) {
            if (i.second > numberCount) {
                number = i.first;
                numberCount = i.second;
            } else if (i.second == numberCount) {
                if (i.first < number) {
                    number = i.first;
                }
            } else {
                continue;
            }
        }
        std::cout << number << " " << numberCount << '\n';
    }
}


How can I make this better and faster?

Solution

Simplify:

count[x] = ++count[x];

        // Why not just;
        ++count[x];


Variable name length:

for (auto i = 0; i < test_cases; ++i) {


Have you ever tried searching for all occurrences of the variable i in the resulting loop. The number of false positives will be a pain in the arse. Name your loop variables so you can find them easily.

Hiding variables names:

for (std::size_t i = 0; i < size; ++i) {


Though not technically illegal. This becomes a maintenance problem. It's OK for you today as you just wrote the code. But for anybody else (or you in years time) this is can be a pain. Try and give your variables unique meaningful names (self documenting code is a brilliant practice but it requires variable names to be meaningful).

The whole loop where you search for the largest repeat:

for (std::size_t i = 0; i < size; ++i) {


This can be done inline while you were counting. I would not have a second loop to work it out after the fact.

I would simplify this condition:

} else if (i.second == numberCount) {
            if (i.first < number) {

// I find it easier to read as:

} else if (i.second == numberCount) && (i.first < number) {


This seems a bit redundant:

} else {
            continue;
        }


Personally I think initializing integers with {} looks terrible (and is slightly confusing).

std::size_t size{};

std::size_t size = 0;  // Much easier to read.


Whats the point in initizliaing a variable just before you write over it?

std::size_t size{};   // Why initialize it here
    std::cin >> size;     // Only to trash the initialization here.


Address Comments:

int number{};
    int numberCount{};
    for (std::size_t i = 0; i > x;
        std::size_t&  countV = count[x];

        ++countV;

        if (countV > numberCount || (countV == numberCount && x < number))
        {
            number      = x;
            numberCount = countV;
        }
    }

Code Snippets

count[x] = ++count[x];

        // Why not just;
        ++count[x];
for (auto i = 0; i < test_cases; ++i) {
for (std::size_t i = 0; i < size; ++i) {
for (std::size_t i = 0; i < size; ++i) {
} else if (i.second == numberCount) {
            if (i.first < number) {

// I find it easier to read as:

} else if (i.second == numberCount) && (i.first < number) {

Context

StackExchange Code Review Q#71171, answer score: 3

Revisions (0)

No revisions yet.