recursion function to duplicate value check

Question

0.00/5 (No votes)

See more:

, +

C#

protected void btnSave_Click(object sender, EventArgs e)
        {

            string result = string.Empty;
            string FacilityId = string.Empty;
            int current;



            var chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
           // var random0 = new Random();
            result = new string(
                Enumerable.Repeat(chars, 1)
                          .Select(s => s[random.Next(s.Length)])
                          .ToArray());

            lock (syncLock)
            { // synchronize
                current = random.Next(10000, (99999));
                FacilityId = result + current;
                bool duplicate = DuplicateCheck(FacilityId);
                if (duplicate == true)
                {
                    current = random.Next(10000, (99999));
                    FacilityId = result + current;
                    bool duplicate1 = DuplicateCheck(FacilityId);
                }
            }

        }

C#

private bool DuplicateCheck(string FacilityId)
        {
            bool Flag=false;
            var duplicate = objDB.SelectAll().Where(a => a.FacilityID == FacilityId).ToList();
            if (duplicate.Count > 0)
            {
                current = random.Next(10000, (99999));
                FacilityId = result + current;
                Flag = true;
            }
            return Flag;
        }

Above 1st method is to generate FacilityId and for duplicate check i have used another method named DuplicateCheck.Here i want to use recursion method for duplicate check till DuplicateCheck Method Return false.

If anyone have any idea please share.
Thanks.

Posted 14-May-15 1:00am

itsathere

Add a Solution

2 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Richard Deeming · Accepted Answer · 2015-05-14T02:11:00

Solution 1

You don't need a recursive method - a while loop will do the trick:

C#

current = random.Next(10000, 99999);
FacilityId = result + current;

while (objDB.SelectAll().Any(a => a.FacilityID == FacilityId))
{
    current = random.Next(10000, 99999);
    FacilityId = result + current;
}

Posted 14-May-15 2:11am

Richard Deeming

Comments

itsathere 14-May-15 10:44am

Your generated FacilityId inside while is not getting checked.what if next generated FacilityId inside while loop matches the database FacilityID.

Richard Deeming 14-May-15 10:46am

The condition for the while loop is re-evaluated each time the loop executes, so the value generated within the loop IS being checked.

Milfje 28-May-15 2:51am

You're right. Any() returns a bool. SelectAll() however returns a collection. Wouldn't it be easier to create and more readable to loop through the collection this way with a foreach loop?

Richard Deeming 28-May-15 6:43am

Why would it be more readable? You'd have to replace the one-line condition in the while loop with a separate bool variable and a foreach loop within the while loop. You'd also have to either repeat the foreach loop before the while loop, or switch to a do { ... } while (...) loop.

Furthermore, if SelectAll returns an IQueryable, then the LINQ solution will be translated to an efficient store query, letting the database do the work. A foreach loop would require loading all of the data into the program's memory before searching it, which would be considerably less efficient.

itsathere 14-May-15 10:47am

thanks.

Milfje 27-May-15 9:45am

Wouldn't a foreach loop be even prettier?

Richard Deeming 27-May-15 14:50pm

I don't think so. The code isn't enumerating a collection; it's looping until a condition returns false.

A do { ... } while (...) loop would be shorter, since you wouldn't need to repeat the body of the loop before the start of the loop.

Sascha Lefèvre · Accepted Answer · 2015-05-14T03:10:00

Recursion has its uses but you shouldn't use it in this case.
Here's an article discussing recursive versus iterative solutions:
Rethinking the Practicalities of Recursion[^]
And here's a forum thread discussing the use of recursion in production code:
Don't ever use recursion in production code?[^]

In your case, recursion has no benefit. Instead, you risk a stack overflow in case random.Next(..) not yielding an unused FacilityId while there's space on the stack.

Alternative 1: Neither recursion nor iteration
Generate your FacilityIds sequentially instead of randomly: A10000, A10001, .., A99999, B10000, etc.
Then you can just query for the highest FacilityId in use and calculate the next one like this:

C#

string GetNextFacilityId()
{
  lock (syncLock)
  {
    string highestFacilityIdInUse =
        objDB.SelectAll()    // see note below
             .OrderByDescending(a => a.FacilityID)
             .Select(a => a.FacilityID)
             .Take(1)
             .SingleOrDefault();

    string nextFacilityId;
    if (highestFacilityIdInUse == null)
        nextFacilityId = "A10000";
    else
    {
        // omitted here: checking highestFacilityIdInUse for correct format
        // [A-Z][10000-99999]

        char c = highestFacilityIdInUse[0];
        int n = Int32.Parse(highestFacilityIdInUse.Substring(1));

        if (c == 'Z' && n == 99999)
            throw new InvalidOperationException("ran out of FacilityIds");
        else if (n == 99999)
            nextFacilityId = (char)(c+1) + "10000";
        else
            nextFacilityId = c + (n+1).ToString();
    }

    return nextFacilityId;
  }
}

Regarding SelectAll(): I don't know which kind of database access technology you're using here. In this query, SelectAll() might be required, superfluous or completely wrong. Please try with and without it. (Applies also to the code below.)

Alternative 2: Iteration

C#

string GetNextFacilityId()
{
  lock (syncLock)
  {
    Random rnd = new Random();
    string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

    char c = chars[rnd.Next(chars.Length)]; // simpler than your Enumerable-approach
    int n = rnd.Next(10000, 100000); // 2nd value is EXCLUSIVE upper bound!

    string nextFacilityId = c + n.ToString();
 
    while (objDB.SelectAll().Any(a => a.FacilityID == nextFacilityId))
    {
        c = chars[rnd.Next(chars.Length)];
        n = rnd.Next(10000, 100000);
        nextFacilityId = c + n.ToString();
    }

    return nextFacilityId;
  }
}

Unrelated suggestion: Don't use a method name like "DuplicateCheck". The name doesn't imply what it will do if a duplicate is found or not found. A name like "IsDuplicate" implies that it will return true or false. Likewise (or "worse"), a variable name like "Flag" doesn't say anything (what would Flag == false mean?). It would be more intuitive if it was named "isDuplicate". You will notice that intuitive names for identifiers help a lot when you come back to your code after not looking at it for some weeks :)
Having said that, I would also rename the above identifiers "c" and "n" to something more meaningful like "prefix" and ... well.. "number" or so.. ;)