Thread: Set myitem.insert(item).second returns always false

Hello ,

I'm coding a password generator for the purpose of solving a new exercise

This code generates 32 char ascii (from 97 to 122) and use set container to save and discard the duplicates.
There is an 'IF' for debug purpose in order to print the rnd number
when the insert action fails, but psw.insert(rnd).second is always False

maybe I did some error , can you help me pls ?
Thanks

Code:

#include "pch.h"
#include <iostream>
#include <ctime>
#include <set>
using namespace std;
const int pswLength = 32;

int getAscii(int start, int end);

int main()
{
  set < int >psw;
  set < int >::iterator it;
  int rnd = 0;

  std::cout << "Hello World!\n\n";

  srand(time(NULL));            // seed

  for (int n = pswLength; n > 0; --n) {

    rnd = getAscii(97, 122);
    psw.insert(rnd);
    if (psw.insert(rnd).second == false)
      cout << rnd << "-" << psw.insert(rnd).second << "-";
  }

  for (it = psw.begin(); it != psw.end(); it++) {
    cout << (char) *it;
  }

}

int getAscii(int start, int end)
{

  int rnd = 0;
  while (rnd < start) {
    rnd = (rand() % end) + 1;
  }
  return rnd;
}

Edit :
OK I understood becouse the code is so coloured I copied code from hastebin
The first time it wasnt deliberate . This second time is just a test

Re: colouring.
Just don't do it.
OK, so you lucked on a method that doesn't result in something looking like dog-food.
But it still doesn't have line numbers.
I will edit your posts, so don't waste my time or yours.

Code:

    psw.insert(rnd);
    if (psw.insert(rnd).second == false)
      cout << rnd << "-" << psw.insert(rnd).second << "-";

It seems like the 2nd insert will always fail, because you've already inserted it.

Your getAscii function could be simplified:

Code:

int getAscii(int start, int end)
{
  return rand() % (end - start) + start;
}

You should loop while (psw.size() < pswLength) in order to account for "failed" iterations in which a duplicate was generated.

You could use a range-based for loop to print psw. I'm surprised you don't have the password as a string, with the set merely to find duplicates quickly.

EDIT:

Originally Posted by Lag

This code generates 32 char ascii (from 97 to 122)

There are 26 such chars. To generate 32 chars in that range, there must be duplicates: a basic application of the pigeon hole principle. Since the requirement is that there be no duplicates, this program is impossible to write correctly.

For what it's worth, I'm thinking along these lines for a C++11 solution:

Code:

#include <ctime>
#include <iostream>
#include <random>
#include <set>
#include <string>

int main()
{
    using namespace std;

    const size_t password_length = 10;

    mt19937_64 random_number_generator{static_cast<unsigned long>(time(nullptr))};
    uniform_int_distribution<char> random_ascii{'a', 'z'};

    string password;
    set<char> password_chars;
    while (password.length() < password_length)
    {
        const auto random_char = random_ascii(random_number_generator);
        if (password_chars.insert(random_char).second)
        {
            password.push_back(random_char);
        }
    }

    cout << password << endl;
    for (auto password_char : password_chars)
    {
        cout << password_char;
    }
    cout << endl;
}

Originally Posted by Salem

Code:

    psw.insert(rnd);
    if (psw.insert(rnd).second == false)
      cout << rnd << "-" << psw.insert(rnd).second << "-";

It seems like the 2nd insert will always fail, because you've already inserted it.

Yes Thanks . Solved

Originally Posted by laserlight

Code:

uniform_int_distribution<char> random_ascii{'a', 'z'};

MSVC doesn't like 'char' ?I googled a bit but I admit that I dont know the subject. Did you use gcc ?
I read your remarks . Thanks I'll try to improve it
****
I've written the code. I'm not happy with it , but It works
The pourpose is to use every symbol ,number,letters only once
and we have to be sure to use them always together when the requested psw>= 7 chars

Code:

set<int> getAscii(int index );
void printAscii(set<int>);


const int nChar = 7;
int main()
{
    set<int> psw;
    
    std::cout << "Hello World!\n\n";
    srand(time(NULL)); // seed


    /* Gets n printable chars througth 7 symbol group */
    /* pick a random symbol for every group*/
    /* every symbol will be present only if nchar >=7*/


    psw = getAscii(nChar);  // Prints n chars and replaces eventual duplicated number
    printAscii(psw);
}


set<int> getAscii(int howManyTimes)
{
    const int end = 1;
    const int start = 0;


    set<int> psw;
    set<int>::iterator it;


    int ascii[7][2] = {
    {97, 122},  /* Lower case*/
    {48, 57} ,  /* Numbers   */
    {32, 47} ,  /*  Symbols 1*/
    {58, 64} ,  /*  Symbols 2*/
    {91, 96} ,  /*  Symbols 3*/
    {123, 127} ,/*  Symbols 4*/ 
    {65, 90} ,  /* upper case*/
    };
    int rnd = 0;
    int index = 0;
    int first = 0;
    int second = 0;
    
    bool success;
    
    /* Pick x number and swaps ascii[][] x times( a kind of "shuffle" )*/
    int times = rand() % 32 +1;


    /* swap ascii[][] elements by random position */
    for (int sw = 0; sw < times; ++sw) {
        first = rand() % 7;
        second = rand() % 7;
        swap(ascii[first][start], ascii[second][start]);
        swap(ascii[first][end], ascii[second][end]);
    }


    /*  Generates random psw 'howManyTimes' times */
    for (int times = 0; times < howManyTimes; times++) { 
        
        do { // If insert fails (duplicate) , retry to gets a new number
                rnd = rand() % (ascii[index][end] - ascii[index][start]) + ascii[index][start];
                success = psw.insert(rnd).second;
           } while (!success);


            index = (index + 1) % 7; // ready to pick number fron next ascii group
    }
    
    return psw;
    
}
void printAscii(set<int> psw)
{
    set<int>::iterator it;


    setlocale(LC_ALL, ".437");
    cout << "PSW:";
    for (it = psw.begin(); it != psw.end(); it++) {
        cout << (char)*it;
    }
    cout << " Size:" << psw.size() << endl;
}

Originally Posted by Lag

MSVC doesn't like 'char' ?I googled a bit but I admit that I dont know the subject. Did you use gcc ?

I used g++. It looks like MSVC is right to have issues with that: although the standard talks about the result type being an integral type (which includes char), when exhaustively enumerating the integral types it excludes char and the signed/unsigned versions thereof, so that g++'s standard library implementation allows for char is a case of them doing what's acceptable for undefined behaviour, but it is actually non-standard behaviour.

It's easy enough to fix:

Code:

uniform_int_distribution<> random_ascii{'a', 'z'};

Then do a probably unnecessary cast to make it obvious that you're going from int to char:

Code:

const auto random_char = static_cast<char>(random_ascii(random_number_generator));

Originally Posted by Lag

I've written the code. I'm not happy with it , but It works
The pourpose is to use every symbol ,number,letters only once
and we have to be sure to use them always together when the requested psw>= 7 chars

It is unnecessarily complicated. Generally, when you want to generate a random sequence of N items such that there are no duplicates, you would pick one of two approaches:

• Randomly generate items one by one, checking the previously generated items on each iteration to check that it is not a duplicate, until N items have been randomly generated.
• Generate (non-randomly) all the possible random items, shuffle (or "partial shuffle"), then pick the first N items.

You would use the first approach when N is small compared to the number of possible random items, otherwise you would use the second approach.

Note that your shuffling algorithm is also inefficient: use a Knuth shuffle instead, which basically involves choosing random items from the list without replacement. This can be achieved in-place by looping over each element in the sequence except the last, swapping it with a randomly chosen element that includes the current element, but that excludes the elements in positions for which a randomly chosen element has already been swapped. std::shuffle from <algorithm> is specified with complexity requirements that imply that this is the algorithm that will be used, so unless you're trying to do a partial shuffle (i.e., you only loop for N iterations, effectively randomly choosing N items from the list without replacement), you should use that.

Hello ,

I thought about it and I want to say that I can't reinvent the whell and I'm finding C++ "charming".
Your code works and I think it's ready, It has what I need.
So I've written another code where I used Mersenne Twister and reading something about it.

I'll create another topic with my beginner code, I still hope to get useful advices from you
Thanks