Re: unique numbers using srand( ) and rand( ) functions in C++[ caution long post]

From: Robert Stankowic (pcdoktor_at_netway.at)
Date: 12/09/03 

Date: Tue, 9 Dec 2003 04:32:13 +0100

"Stefan Höhne" <do_not_use_this_adress@127.0.0.1> schrieb im Newsbeitrag
news:br24v4$cv2$1@news1.wdf.sap-ag.de...
> Hi,
>
>
>
> "Robert Stankowic" <pcdoktor@netway.at> wrote in message
> news:<3fd45e86$0$22358$91cee783@newsreader01.highway.telekom.at>...
>
> [...]
>

[....]
Thank you, Stefan..

> > using namespace std;
> >
> > int WelcomeAndInit(ofstream& send2file, ifstream& readfile)
> > {
> > cout << "\t\t\tLottery Numbers" << endl << endl
> > << "This program opens the data file \"T8Be08.dat\" and then stores"
> > "groups of" << endl
> > << "lotto numbers that have 6 unique numbers to a group in the data"
> > "file." << endl
> > << "There are 50 groups of numbers. The groups of numbers are"
> > " displayed to the" << endl
> > << "screen - 3 groups per line." << endl << endl;
>
> for my taste there are too much endl's mixed in here. '\n' is not
> too hard to type, may be embedded in a string and doesn't flush
> the stream unnessesarily.

You are right,
cout << "\t\t\tLottery Numbers\n\n"
"This program opens the data file \"T8Be08.dat\" and then...
I had just reformatted the wrapping string literals in the first version to
make the whole thing compile and to avoid wrapping in my reply, and after
that did not pay any more attention to it.
Thanks.

>
> > srand(time(NULL));
> > send2file.open("T8Be08.dat", ios::out);
>
> 2nd argument nessesary?
>
> > if(send2file.fail())
>
> if (!send2file)
>
> is much more idiomatic
>
> > {
> > return OPEN_OUT_FAILED;
> > }
> > readfile.open("T8Be08.dat", ios::in);
>
> Er ...
>
> ... do we open the same file twice, for reading and writing?
>
> What shall be the result of this? Well-defined behaviour?
>
> Open, write, close. Open again, read, close. So just drop this
> initilisation function. Initialisation of streams can be done
> at the declaration point, error checking is easy enough.

Brain fart..
Of course that's silly. And one of the nasty cases where UB produces the
expected result on some implementations :(

>
> > if(readfile.fail())
> > {
> > send2file.close();
> > return OPEN_IN_FAILED;
> > }
>
> Why do we close send2file here? It will be closed automagically
> when beeing destructed.

Hm.. (generally, besides the silly opening the same file twice)
send2file is defined in main(), so as I understand, it will stay alive until
main() returns. In the actual code here I think this makes no difference.
But what if in main() an attempt is made to recover from this situation -
not in the actual code as I said, but in general. Wouldn't it be better to
return in a "clean" state without any open stream hanging around?

>
> > return SUCCESS;
>
> I dont like this C-style error handling code here.
>
> > }/* WelcomeAndInit */
> >
> >
> >
> > int WriteNumberFile(ofstream& send2file)
> > {
> > short group_count = 0;
> > short lower_bound = 1;
> > short upper_bound = 9;
> > short one_tip[6] = {0};
> > while(group_count < NUM_GROUPS)
> > {
> > /*we generate a group of numbers*/
> > for(short x = 0; x < GROUP_SIZE; x++)
>
> I dont understand why you use shorts all the time. An int
> will probably be faster and doesn't has to be thought over
> all the time.

Agreed :)
I also don't like the x, y and z in the loops, I'd prefer the usual i, j, k.
I left that, because I did not want to change the original too much.

>
> > {
> > one_tip[x] = lower_bound + rand() % (upper_bound - lower_bound +
> > 1);
>
> Hmm. Probably you better take a look at
> http://www.eskimo.com/~scs/C-faq/q13.16.html

I know. I did not change that, but I wrote a note that the randomness is
questionable after the first reformatted version.

>
> > lower_bound = upper_bound + 1;
> > upper_bound = upper_bound + 9;
> > if(x == GROUP_SIZE - 1)
> > {
>
> Hmm ...
>
> > /*we are done with one group, so we write it to the file*/
> > for(short y = 0; y < GROUP_SIZE; y++)
> > {
> > if(y == GROUP_SIZE - 1)
> > {
> > send2file << one_tip[y] << endl;
> > }
> > else
> > {
> > send2file << one_tip[y] << '#';
> > }
> > if(send2file.fail())
> > {
> > return WRITE_FAILED;
> > }
> >
> > }
>
> I would write that
>
> send2file << one_tip[0];
>
> for (int tipNo=1;tipNo < GROUP_SIZE; ++tipNo)
> send2file << '#' << one_tip[tipNo];
> send2file << endl;
>
> if (!send2file) return WRITE_FAILED;

Yes. Again, I tried to stick to the original as much as possible.
One little thing: I am not really happy with the name of the index you
choose - looping through the array one_tip[] with an index tipNo seems a bit
misleading to me

>
> > }
> > }
> > lower_bound = 1;
> > upper_bound = 9;
> > group_count++;
> > }
>
> > send2file.close();
> > return SUCCESS;
> > } /* WriteNumberFile */
> >
> >
> >
> > int ReadAndDisplayFile(ifstream& readfile)
> > {
> > short column_count = 0;
> > short group_count = 0;
> > short one_tip[6] = {0};
> >
> > while(group_count < NUM_GROUPS)
> > {
> > for(short z = 0; z < GROUP_SIZE; z++)
> > {
> > readfile >> one_tip[z];
> > readfile.ignore(1);
>
> This#the#same#file#format#we#just#wrote?
> Oh#yes#it#is.
> Difficulties#in#parsing#this?#Guess#why?#Weird#whitespace!

Not my decision :)

>
> > if(readfile.fail())
>
> Er ...
>
> You'll be on the safe side with if (!readfile), believe me.

I do :)

>

[....]

> >
> > readfile.close();
>
> I really dont know why so many people use open() and
> close() on streams.

C strikes back :)

>
> > return SUCCESS;
> > } /* ReadAndDisplayFile */
> >
> >
> >
>
> > void OnError(int err_code)
> > {
[....]
> > } /* OnError */
>
> All functions printing something somewhere should take an
> ostream& - argument telling it where to print. This makes them
> much more versatile.
>
> Error messages should be better stored in constant character arrays,
> this would save you the switch statement (and make localisation as well
> as error message maintenance easier - one point where you have them
> all ...)

Good points, thank you.

>
> > int main()
> > {
> > ofstream send2file;
> > ifstream readfile;
> > int result;
> >
> > if((result = WelcomeAndInit(send2file, readfile)) == SUCCESS)
> > {
> > WriteNumberFile(send2file);
> > ReadAndDisplayFile(readfile);
> > return EXIT_SUCCESS;
> > }
> > else
>
> Useless else.
>
> I usually do it the other way round:
> if (somethingWentWrong) throw SomeException();
> proceed_with_processing();
>
> Handling the error usually takes only a few lines, so you can jump
> over it with your eye withou loosing context. Handling the error in
> the else-clause means error-handling occuring at the end of the
> function, where we don't know anymore where this "else" belongs to.
>
> I additionally like avoiding the extra indentation needed for putting
> everything into an else clause, especially if I have multiple error
> conditions to be checked.

I agree, that this approach is a good one, however, with well chosen error
constants and restricting the size of functions to not more than one screen
of lines it does not make much difference I think.
And isn't, if we use exceptions, the catch() block after the try block as
well?

>
> > {
> > OnError(result);
> > return EXIT_FAILURE;
> > }
> > return EXIT_SUCCESS;
>
> Useless statement, will never be reached.

Oops..

>
> > }/* main */

Btw, I used spaces for indentation, but here the code shows up unindented -
is that now my newsreader or yours, or did I mess up something with the
spaces?

kind regards
Robert

Robert

Relevant Pages

  • Re: F2003 access=stream question
    ... >>I imagine that for most implementations, ... >>standard gives you no guarantees. ... >>opens the file for sequential access, ... >>statement opens it for stream access. ...
    (comp.lang.fortran)
  • Re: convenient way to read text file multiple times without reopening it
    ... my program opens the file ... but not when I create InputStreamReader from that FileInputStream. ... BufferedReader provides the decoded char buffer, but the InputStreamReader is still responsible for the binary reading, and thus it does maintain it's own private bytebuffer. ... When it comes to streams, once you wrap one stream with another stream, any further direct changes to the underling stream usually result in "undefined" behavior of the outer stream. ...
    (comp.lang.java.programmer)
  • Re: Windows media services isues
    ... (mean i can`t open a connection with another windows media player ... after that it opens further ports to actually stream the media from. ...
    (microsoft.public.windowsmedia)
  • Re: Regina seems to have issues
    ... This section describes some implementations of stream I/O in REXX. ... Regina implements a set of functions ... Regina implicitly opens any file whenever it is first used. ...
    (comp.lang.rexx)
  • Re: Changing font size in a web part?
    ... > clicked it opens as a parent. ... >> browser where to open the link. ... >> _parent means in our case the actual browser window. ... >> I dont know the contents of your web part, so i cannot solve the problem. ...
    (microsoft.public.sharepoint.portalserver)