up vote 0 down vote favorite
share [g+] share [fb]

For some reason, this code works fine when I don't use a seed in the Random class, but if I try to use DateTime.Now to get a more random number, I get a StackOverflowException! My class is really simple. Could someone tell me what I'm doing wrong here? See MakeUniqueFileName.

public class TempUtil
{
    private int strcmp(string s1, string s2)
    {
        try
        {
            for (int i = 0; i < s1.Length; i++)
                if (s1[i] != s2[i]) return 0;
            return 1;
        }
        catch (IndexOutOfRangeException)
        {
            return 0;
        }
    }
    private int Uniqueness(object randomObj)
    {
        switch (randomObj.ToString())
        {
            case "System.Object":
            case "System.String":
                return randomObj.ToString()[0];
            case "System.Int32":
                return int.Parse(randomObj.ToString());
            case "System.Boolean":
                return strcmp(randomObj.ToString(), "True");
            default:
                return Uniqueness(randomObj.ToString());
        }
    }
    public string MakeUniqueFileName()
    {
        return "C:\\windows\\temp\\" + new Random(Uniqueness(DateTime.Now)).NextDouble() + ".tmp";
    }
}
link|improve this question
Isn't the point about seeding that you seed once and then get the random sequence from that seed? – Martin 19 mins ago
I'd venture that method Uniqueness is a strange method indeed. Can you explain the design principal behind it? Wouldn't it be better to check against Type rather than .ToString. It's probably not doing what you expect in the case of System.String – spender 19 mins ago
2  
Any reason you can't use Path.GetTempFileName msdn.microsoft.com/en-us/library/… – Brian Rasmussen 18 mins ago
First off it is completely unclear what you mean by "more random". Second, Random is already seeded with the current time, so seeding it with the current time isn't "more" or "less" anything. It's the same. – Eric Lippert 17 mins ago
strcmp -> string.CompareTo, MakeUniqueFileName -> Path.GetTempFileName - don't reinvent the wheel – BrokenGlass 16 mins ago
show 7 more comments
feedback

4 Answers

up vote 1 down vote accepted

In short:

It should say 

    switch (randomObj.GetType().ToString())

instead of

    switch (randomObj.ToString())

But even then this isn't very clever.

link|improve this answer
that fixed it thanks! – Raging Dave 7 mins ago
feedback
up vote 4 down vote

You're calling DateTime.Now.ToString(), which doesn't give you one of the strings you're checking for... so you're recursing, calling it with the same string... which still isn't one of the strings you're looking for.

You don't need to use Random to demonstrate the problem. This will do it very easily:

Uniqueness(""); // Tick, tick, tick... stack overflow

What did you expect it to be doing? It's entirely unclear what your code is meant to be doing, but I suggest you ditch the Uniqueness method completely. In fact, I suggest you get rid of the whole class, and use Path.GetTempFileName instead.

link|improve this answer
feedback
up vote 1 down vote

You are passing a DateTime instance to your Uniqueness method.

This falls through and calls itself with ToString - on a DateTime instance this will be a formatted DateTime string (such as "21/01/2011 13:13:01").

Since this string doesn't match any of your switch cases (again), the method calls itself again, but the result of calling ToString on a string is the same string.

You have caused an infinite call stack that results in the StackOverflowException.

There is no need to call Uniquness - when creating a Random instance, it will be based on the current time anyways.

I suggest reading Random numbers from the C# in depth website.

link|improve this answer
feedback
up vote 0 down vote

The parameter-less constructor of Random already uses the current time to as seed value. It uses the time ticks, used internally to represent a DateTime.

A problem with this approach, however, is that the time clock ticks very slowly compared to the CPU clock frequency. If you create a new instance of Random each time you need a random values, it may be, that the clock did not tick between two calls, thus generating the same random number twice.

You can simply solve this problem by creating an instance of Random only once.

public class TempUtil { 
    private static readonly Random random = new Random();

    public string MakeUniqueFileName()
    {
        return @"C:\windows\temp\" + random.NextDouble() + ".tmp";     
    } 
}

This will generate very good random numbers.

By the way

System.IO.Path.GetTempFileName()

automatically creates an empty temporary file with a unique name and returns the full path of that file.

link
feedback

Your Answer

 
or
required, but never shown

Not the answer you're looking for? Browse other questions tagged or ask your own question.