Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
#include <iostream>
#include <vector>
#include <algorithm>

In smaller programs like yours, traversing a list of 3 #includes isn't an issue. In larger programs, the list of #included libraries can become large. Organizing your #includes into logical groups and alphabetizing those subgroups helps determine if something is #included. Think about how you search a phone book. You typically use some pivoted search to narrow down your search range. The same applies when looking at a list of library #includes.


sort(v.begin(), v.end());

Unless your intent is to invoke argument dependent lookup (ADL), I would suggest you qualify your calls. As the reader, I'm left to try to figure out if this is a call to std::sort or your own rolled sort. ADL can also result in an unintended function being called an unintended function being called.

If your intent was to use ADL, be explicit with a using-declaration.

using std::swap; // Explicit: opting into ADL.
swap(lhs, rhs); // unqualified lookup, find the best match.

for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {
 if (v[i] == number)
 ++countMode;
 if (countMode > count) {
 count = countMode;
 mode = number;
 }
}

It should be noted that the calculation for the mode is incorrect. You never reset the countMode value, so your function returns the last duplicate value seen rather than the mode.

Assuming you fix that, consider other possible sequence structures.

  • Many duplicates - where mode([1, 1, 2, 2, 3, 3]) should result in the multimodal set [1, 2, 3].
  • Unique values - where mode([1, 2, 3]) should result in the empty set [].
  • Empty set - where mode([]) should result in the empty set [].

Consider how your program handles these situations.

In the case of an empty set, v[0] doesn't exist. Attempting to access v[0] produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.


While it is easy for beginner's to just dump everything into main(), I would suggest you break your program up into smaller, focused, and testable logical abstractions. min and max elements are as simple as calls to front() and back() on sorted lists. If you want those values for unsorted lists, the standard library provides std::min_element, std::max_element and std::minmax_element in the <algorithm> library. mode should be abstracted into it's own function as well utilizing other abstractions to accomplish its task.

#include <iostream>
#include <vector>
#include <algorithm>

In smaller programs like yours, traversing a list of 3 #includes isn't an issue. In larger programs, the list of #included libraries can become large. Organizing your #includes into logical groups and alphabetizing those subgroups helps determine if something is #included. Think about how you search a phone book. You typically use some pivoted search to narrow down your search range. The same applies when looking at a list of library #includes.


sort(v.begin(), v.end());

Unless your intent is to invoke argument dependent lookup (ADL), I would suggest you qualify your calls. As the reader, I'm left to try to figure out if this is a call to std::sort or your own rolled sort. ADL can also result in an unintended function being called.

If your intent was to use ADL, be explicit with a using-declaration.

using std::swap; // Explicit: opting into ADL.
swap(lhs, rhs); // unqualified lookup, find the best match.

for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {
 if (v[i] == number)
 ++countMode;
 if (countMode > count) {
 count = countMode;
 mode = number;
 }
}

It should be noted that the calculation for the mode is incorrect. You never reset the countMode value, so your function returns the last duplicate value seen rather than the mode.

Assuming you fix that, consider other possible sequence structures.

  • Many duplicates - where mode([1, 1, 2, 2, 3, 3]) should result in the multimodal set [1, 2, 3].
  • Unique values - where mode([1, 2, 3]) should result in the empty set [].
  • Empty set - where mode([]) should result in the empty set [].

Consider how your program handles these situations.

In the case of an empty set, v[0] doesn't exist. Attempting to access v[0] produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.


While it is easy for beginner's to just dump everything into main(), I would suggest you break your program up into smaller, focused, and testable logical abstractions. min and max elements are as simple as calls to front() and back() on sorted lists. If you want those values for unsorted lists, the standard library provides std::min_element, std::max_element and std::minmax_element in the <algorithm> library. mode should be abstracted into it's own function as well utilizing other abstractions to accomplish its task.

#include <iostream>
#include <vector>
#include <algorithm>

In smaller programs like yours, traversing a list of 3 #includes isn't an issue. In larger programs, the list of #included libraries can become large. Organizing your #includes into logical groups and alphabetizing those subgroups helps determine if something is #included. Think about how you search a phone book. You typically use some pivoted search to narrow down your search range. The same applies when looking at a list of library #includes.


sort(v.begin(), v.end());

Unless your intent is to invoke argument dependent lookup (ADL), I would suggest you qualify your calls. As the reader, I'm left to try to figure out if this is a call to std::sort or your own rolled sort. ADL can also result in an unintended function being called.

If your intent was to use ADL, be explicit with a using-declaration.

using std::swap; // Explicit: opting into ADL.
swap(lhs, rhs); // unqualified lookup, find the best match.

for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {
 if (v[i] == number)
 ++countMode;
 if (countMode > count) {
 count = countMode;
 mode = number;
 }
}

It should be noted that the calculation for the mode is incorrect. You never reset the countMode value, so your function returns the last duplicate value seen rather than the mode.

Assuming you fix that, consider other possible sequence structures.

  • Many duplicates - where mode([1, 1, 2, 2, 3, 3]) should result in the multimodal set [1, 2, 3].
  • Unique values - where mode([1, 2, 3]) should result in the empty set [].
  • Empty set - where mode([]) should result in the empty set [].

Consider how your program handles these situations.

In the case of an empty set, v[0] doesn't exist. Attempting to access v[0] produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.


While it is easy for beginner's to just dump everything into main(), I would suggest you break your program up into smaller, focused, and testable logical abstractions. min and max elements are as simple as calls to front() and back() on sorted lists. If you want those values for unsorted lists, the standard library provides std::min_element, std::max_element and std::minmax_element in the <algorithm> library. mode should be abstracted into it's own function as well utilizing other abstractions to accomplish its task.

deleted 72 characters in body
Source Link
Snowhawk
  • 6.8k
  • 1
  • 19
  • 37

In the case of an empty set is processed, v[0] exists turns out to be false as well with an empty sequencedoesn't exist. Attempting to access v[0] in that case produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.

In the case of an empty set is processed, v[0] exists turns out to be false as well with an empty sequence. Attempting to access v[0] in that case produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.

In the case of an empty set, v[0] doesn't exist. Attempting to access v[0] produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.

Source Link
Snowhawk
  • 6.8k
  • 1
  • 19
  • 37
#include <iostream>
#include <vector>
#include <algorithm>

In smaller programs like yours, traversing a list of 3 #includes isn't an issue. In larger programs, the list of #included libraries can become large. Organizing your #includes into logical groups and alphabetizing those subgroups helps determine if something is #included. Think about how you search a phone book. You typically use some pivoted search to narrow down your search range. The same applies when looking at a list of library #includes.


sort(v.begin(), v.end());

Unless your intent is to invoke argument dependent lookup (ADL), I would suggest you qualify your calls. As the reader, I'm left to try to figure out if this is a call to std::sort or your own rolled sort. ADL can also result in an unintended function being called.

If your intent was to use ADL, be explicit with a using-declaration.

using std::swap; // Explicit: opting into ADL.
swap(lhs, rhs); // unqualified lookup, find the best match.

for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {
 if (v[i] == number)
 ++countMode;
 if (countMode > count) {
 count = countMode;
 mode = number;
 }
}

It should be noted that the calculation for the mode is incorrect. You never reset the countMode value, so your function returns the last duplicate value seen rather than the mode.

Assuming you fix that, consider other possible sequence structures.

  • Many duplicates - where mode([1, 1, 2, 2, 3, 3]) should result in the multimodal set [1, 2, 3].
  • Unique values - where mode([1, 2, 3]) should result in the empty set [].
  • Empty set - where mode([]) should result in the empty set [].

Consider how your program handles these situations.

In the case of an empty set is processed, v[0] exists turns out to be false as well with an empty sequence. Attempting to access v[0] in that case produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.


While it is easy for beginner's to just dump everything into main(), I would suggest you break your program up into smaller, focused, and testable logical abstractions. min and max elements are as simple as calls to front() and back() on sorted lists. If you want those values for unsorted lists, the standard library provides std::min_element, std::max_element and std::minmax_element in the <algorithm> library. mode should be abstracted into it's own function as well utilizing other abstractions to accomplish its task.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /