Skip to main content
fix typo
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

OtherOthers have mentioned this, but declaring an array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and its associated count.

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Other have mentioned this, but declaring an array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and its associated count.

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Others have mentioned this, but declaring an array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and its associated count.

Typo fixes
Source Link
Toby Speight
  • 81.8k
  • 14
  • 101
  • 309

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Other have mentioned this, but declaring andan array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and it'sits associated count.

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Other have mentioned this, but declaring and array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and it's associated count.

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Other have mentioned this, but declaring an array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and its associated count.

added generic algorithm
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Other have mentioned this, but declaring and array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and it's associated count.

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Other have mentioned this, but declaring and array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Here are some ideas to help you improve your code, and perhaps inspire you to learn more.

Reconsider using

The code currently contains these two lines:

using std::cout;
using std::cin;

That's not too terrible, but there are at least two ways to make it better. First would be to put those inside main to limit any possible conflicts there instead of globally. Better, in my view, would be to simply omit them and use std::cin and std::cout since they're only used in three lines in your entire program. That way readers will instantly see that you're using the std:: versions and not some other version.

Decompose your program into functions

All of the logic here is in main in one dense chunk of code. It would be better to decompose this into a separate function or functions.

Avoid non-standard features

Other have mentioned this, but declaring and array with anything other than a compile-time number is not in standard C++ and so that should be avoided. I hope your teacher isn't writing C++ code like this!

Use modern C++ features

It is very likely you haven't learned about std::ranges or various other algorithms and data structures yet, but just to whet your appetite, here's a rewrite of your program using C++20 and all of the tools in the toolbox:

#include <iostream>
#include <map>
#include <ranges>
#include <algorithm>

int main () {
    using myDataType = float;
    using myMapType = std::map<myDataType, unsigned>;
    myDataType value;
    myMapType counts;
    int N;
    for (std::cin >> N; N && std::cin >> value; --N) {
        ++counts[value];
    }
    auto mode = std::ranges::max_element(counts,{},&myMapType::value_type::second);

    std::cout << "The mode of the data set is " << mode->first 
              << " appearing " << mode->second << " times\n";
}

Use a more efficient algorithm

It's good to learn how to write your own code, but given your restrictions, your options are somewhat limited. Here's a suggestion for an alternative implementation that is somewhat more efficient, expressed in psuedo-code:

  1. create "big enough" arrays for data and count
  2. initialize overall item count and max element index to 0
  3. for each read value:
    1. linearly search the data array for a matching value
    2. if it's new, add it to the array and set the count to 1 and increment the overall unique item count
    3. otherwise, it's not new so increment the count and compare to current max element index (updating the latter as appropriate)

At the end of this, the max element index will point to the index of the mode value and it's associated count.

Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282
Loading