I have an array:
names[4]={john,david,jack,harry};
and i want to do it randomly shuffle, like:
names[4]={jack,david,john,harry};
I tried to use this but it just shuffled the letters of the first word in the array:
random_shuffle(names->begin(), names->end());
Here is the full code, it reads names from a .txt file and puts in an array:
#include <iostream>
#include <fstream>
#include <string>
#include <algorithm>
using namespace std;
int main() {
ifstream readName("names.txt");
string names[197];
int i = 0;
for (string line; getline(readName, line); ){
readName >> names[i];
i ;
}
readName.close();
random_shuffle(names->begin(), names->end());
for (int i = 0; i < 197; i ) {
cout << names[i] << endl;
}
return 0;
}
I tried few other things from different people but I couldn't manage to work it.. Anything helps, thank you!
CodePudding user response：
Here's your code with what I felt were the smallest amount of changes. One could argue that I didn't need to change your first for loop as much, but I figure that if you have the prescience to know how many names you're reading, you might as well use the knowledge.
#include <algorithm>
#include <fstream>
#include <iostream>
#include <iterator> // std::begin(), std::end(); required for C-arrays
#include <random> // std::mt19937; needed to feed std::shuffle()
#include <string>
// using namespace std; // BAD PRACTICE
int main() {
constexpr int size = 4; // Give your magic number a name; only need to change
// a single location
std::ifstream readName("names.txt");
if (!readName) { // Always check that you successfully opened the file.
std::cerr << "Error opening file.\n";
return 1;
}
std::string names[size];
// int i = 0;
for (int i = 0; i < size; i) { // Retool the loop entirely
std::getline(readName, names[i]);
}
readName.close();
// This is a fragile solution. It's only working because the array is in
// scope
std::shuffle(std::begin(names), std::end(names),
std::mt19937{std::random_device{}()});
for (int i = 0; i < size; i ) {
std::cout << names[i]
<< '\n'; // Don't use std::endl unless you actually need it
}
return 0;
}
This is not ideal code, though. Any change to the size of the input file requires changing the code and re-compiling. The biggest single change was to get rid of std::random_shuffle and use std::shuffle() instead. std::random_shuffle was deprecated with C 14 and removed in C 17. It's bad to use it. std::shuffle() does add the requirement of providing a PRNG, but it's not so bad. It leads to better code if you have a PRNG that needs to randomize many different things in a bigger program. This is because it's good to have a single PRNG and let it live for the length of your program as opposed to constantly building new ones.
And the C-array just makes things a bit clunkier. Enter std::vector.
#include <algorithm>
#include <fstream>
#include <iostream>
#include <iterator>
#include <random> // std::mt19937; needed to feed std::shuffle()
#include <string>
#include <vector>
int main() {
std::ifstream readName("names.txt");
if (!readName) { // Always check that you successfully opened the file.
std::cerr << "Error opening file.\n";
return 1;
}
std::vector<std::string> names;
std::string name;
while (std::getline(readName, name)) { // Retool the loop entirely
names.push_back(name);
}
readName.close();
std::shuffle(std::begin(names), std::end(names),
std::mt19937{std::random_device{}()});
for (const auto& i : names) {
std::cout << i << '\n';
}
return 0;
}
The vector can grow as needed, so you see how much simpler the loop that reads the names becomes. It's also more flexible since you don't have to know ahead of time how many entries to expect. It will "just work." With the call to std::shuffle() I kept the std::begin(names) syntax because many consider this a best practice, but you could have also used names.begin() if you wanted since the vector class provides its own iterators.
CodePudding user response：
Lets have a look at you main question:
I tried to use this but it just shuffled the letters of the first word in the array:
random_shuffle(names->begin(), names->end());
The reason that it only shuffles the first word is because of the types and usage.
So names is an array of strings.
string names[197];
The problem stems from the C world. Were arrays decay into pointers exceedingly easily (just by being used in an expression). So here names-> has decayed into a pointer to the first element of the array. This allows you to use the -> operator which normally only works on pointers. So you are calling the functions begin() and end() on the pointer to the first element in the array. Thus only the first name is being shuffled.
The fix this problem use std::begin() method.
// here std::begin / std::end find the beginning and end
// of the array. So you are shuffling the array.
random_shuffle(std::begin(names), std::end(names));
But I would note that random_shuffle() is outdated. As mentioned by @sweenish you should use std::shuffle() See his answer for details.
A couple of things we can improve:
You use C array to store the names. Sure it works, but it is susceptible to a couple of issues because it can not be re-sized (and unless you think the file is never going to be changed that may be an issue). Potentially a hidden issue to a far distant maintainer.
std::vector<std::string> names; // resizeable container.
I would note that the current implementation ignores the first line. Then reads the first word from each subsequent line. There is also a slight issue that the last line may be empty and you read an empty name into the last element of the array (but you don't track how many names you read so unless you use all elements in the array you may never notice that).
I would change that. As it is not obvious. I would delibrately and seprately ignore the first line. Then I would simply read all the first words into a vector (so you know the size).
std::string line
std::getline(file, line); // Ignore the first line.
std::string word
while(file >> word) {
names.push_back(word);
std::getline(file, line); // ignore the rest of the line.
}
We could get fancy. Use iterators to simply create the array directly.
class Line
{
std::string firstWord;
friend std::istream& operator>>(std::istream& stream, Line& data) {
stream >> data.firstWord;
stream.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); retrun stream;
}
operator std::string() const {
return firstWord;
}
};
Now you can create and load the vector in one line:
std::vector<std::string> names(std::istream_iterator<Line>(file),
std::istream_iterator<Line>{});
Then finally copy the names out can be made easier using foreach loop. Also don't use std::endl in a loop like this. It forces a flush of the underlying bugger after each new line. This is very ineffecient.
for(auto const& name: names) {
std::cout << name << "\n";
}
So the result is:
#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <iterator>
#include <algorithm>
class Line
{
std::string firstWord;
friend std::istream& operator>>(std::istream& stream, Line& data) {
stream >> data.firstWord;
stream.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); return stream;
}
operator std::string() const {
return firstWord;
}
};
int main()
{
std::ifstream file("names.txt");
std::string line
std::getline(file, line); // Ignore the first line.
std::vector<std::string> names(std::istream_iterator<Line>(file),
std::istream_iterator<Line>{});
random_shuffle(std::begin(names), std::end(names));
for(auto const& name: names) {
std::cout << name << "\n";
}
}