Local variable inside recursive function unassigned

I have the following code, any ideas on how to resolve this issue, instead of declaring a int variable outside the function? I get the following 开发者_开发技巧compiler error: Use of unassigned local variable 'counter'

public static int GetNumberOfDevicesForManagementGroup(Guid managementGroupId, bool firstTime)
  {
     int counter;
     using (var ctx = new DeviceManagerEntities())
     {
        if (firstTime)
        {
           firstTime = false;
           counter = 0;
           GetNumberOfDevicesForManagementGroup(managementGroupId, firstTime);
        }
        else
        {
           var groups = ctx.ManagementGroups
              .Where(x => x.ParentId == managementGroupId)
              .ToList();
           if (groups.Count != 0)
           {
              foreach (ManagementGroups group in groups)
              {
                 var devices = ctx.Devices
                    .Where(x => x.ManagementGroups.ManagementGroupId == group.ManagementGroupId)
                    .ToList();
                 foreach (Devices device in devices)
                 {
                    counter++;
                 }
                 GetNumberOfDevicesForManagementGroup(group.ManagementGroupId, firstTime);
              }
           }
           else
           {
              var devices = ctx.Devices
                    .Where(x => x.ManagementGroups.ManagementGroupId == managementGroupId)
                    .ToList();
              foreach (Devices device in devices)
              {
                 counter++;
              }
           }
        }
     }
     return counter;
  }

There seem to be many things wrong with this function.

1. You have a recursive function that creates a new entity context - and recurses before the context is disposed! So not only does this create a ton of redundant ObjectContext instances, but they are all used at the same time. This whole thing should be totally rewritten to share a context across function calls.

2. You create a ObjectContext in a static method. This is really poor design. Especially given the name of this method, it would appear that you are abusing static methods in order to implement what is effectively procedural code. This should be an instance method, and the class should probably be the thing that actually maintains the ObjectContext.

3. You have a bunch of lines like this: GetNumberOfDevicesForManagementGroup(managementGroupId, firstTime);. These do nothing whatsoever except waste CPU cycles and database time. You're throwing away the results you get from them. It looks like you think that successive executions of GetNumberOfDevicesForManagementGroup are going to share the same counter variable; that's not how recursion works, that's not how subroutines work, and making counter a global variable to compensate is wrong wrong wrong.

4. Instead of actually getting a count in each instances, you just download all of the "devices" and count them one-by-one. This is, again, a huge waste of CPU and database time.

5. You're running a database query in a loop. Yikes.

6. The two lines firstTime = false; and counter = 0; in the first if block don't do anything at all. You're assigning to the function arguments. These are no-ops.

7. You never actually do initialize counter for the else block, so the compiler error really is no surprise. If you want to increment a variable, such as counter++, it has to start somewhere.

Honestly, it looks like this originated as some crufty procedural code that has been haphazardly "converted" to C#. You need to rewrite this method completely. You probably need to redo a lot of your design.

Here is an example of a rewritten class that, if I've understood your code correctly, will accomplish the same task (getting the count of devices for a single management group and all of the management groups in its subtree):

public class DeviceRepository
{
    private DeviceManagerEntities context;

    public DeviceRepository(DeviceManagerEntities context)
    {
        if (context == null)
            throw new ArgumentNullException("context");
        this.context = context;
    }

    public int GetDeviceCount(Guid managementGroupID)
    {
        return GetDeviceCount(new Guid[] { managementGroupID });
    }

    public int GetDeviceCount(IEnumerable<Guid> managementGroupIDs)
    {
        int deviceCount = context.Devices
            .Where(d => managementGroupIDs.Contains(
                d.ManagementGroups.ManagementGroupID))
            .Count();
        var childGroupIDs = context.ManagementGroups
            .Where(g => managementGroupIDs.Contains(g.ParentId))
            .Select(g => g.ManagementGroupID);
        deviceCount += GetDeviceCount(childGroupIDs);
        return deviceCount;
    }
}

Note that this is still not going to perform very well because it's hammering the database with a new query for every child group; in order to get this up to snuff, you would need to implement a recursive query in the database itself.

You seem to have misunderstood how recursive functions work: You don’t return a result (even though there’s a return at the end of the function!). You seem to think that counter is shared between the recursive calls – but the opposite is the case. In fact, the very principle of recursion is based on the fact that no sharing occurs.

Every recursive call gets a new counter variable. It is your job to add all these results together. For example, take just the recursion anchor:

if (firstTime)
{
    firstTime = false;
    counter = 0;
    GetNumberOfDevicesForManagementGroup(managementGroupId, firstTime);
}

This is wrong; it should really look like this:

if (firstTime)
{
    return GetNumberOfDevicesForManagementGroup(managementGroupId, false);
}

The important thing here is to return the result. But setting firstTime is unnecessary (and unusual), and setting counter at all is also unnecessary.

The rest of the method body has to be changed accordingly.

(Additionally, this recursion anchor seems to be pointless. It could as well be omitted.)

write like this:

you can insert your counter value as a parameter.

public static int GetNumberOfDevicesForManagementGroup(Guid managementGroupId, bool firstTime, int counterValue)
  {
     int counter = 0;
     counter = counterValue;

     using (var ctx = new DeviceManagerEntities())
     {
        if (firstTime)
        {
           firstTime = false;
           counter = 0;
           GetNumberOfDevicesForManagementGroup(managementGroupId, firstTime);
        }
        else
        {
           var groups = ctx.ManagementGroups
              .Where(x => x.ParentId == managementGroupId)
              .ToList();
           if (groups.Count != 0)
           {
              foreach (ManagementGroups group in groups)
              {
                 var devices = ctx.Devices
                    .Where(x => x.ManagementGroups.ManagementGroupId == group.ManagementGroupId)
                    .ToList();
                 foreach (Devices device in devices)
                 {
                    counter++;
                 }
                 GetNumberOfDevicesForManagementGroup(group.ManagementGroupId, firstTime);
              }
           }
           else
           {
              var devices = ctx.Devices
                    .Where(x => x.ManagementGroups.ManagementGroupId == managementGroupId)
                    .ToList();
              foreach (Devices device in devices)
              {
                 counter++;
              }
           }
        }
     }
     return counter;
  }

you can insert your counter value as a parameter.

Initialize counter to zero to silence the compiler! In fact Why not make it static outside of the function?

static int counter = 0;
public static int GetNumberOfDevicesForManagementGroup(Guid managementGroupId, bool firstTime)
{
 ....
}

Edit: It seems the OP want to have a variable within the scope of the function, the other way of doing it is to modify the signature of the function to include a parameter for the variable counter and make it an out parameter instead...

Hope this helps, Best regards, Tom.

As others suggested, if you initialize counter to zero upon declaration you solve your compilation problem. But only that one.

But... Why do you use the firstTime argument as an accumulator? Why not remove it entirely and use no accumulators at all?

public static int GetNumberOfDevicesForManagementGroup(Guid managementGroupId)
{
     // *** Initialization
     int counter = 0;
     using (/* ... */)
     {
       // *** No first time special case
       var groups = // ...
       if (groups.Count != 0)
       {
          foreach (ManagementGroups group in groups)
          {
             // *** No need to call ToList() to count
             counter += ctx.Devices
                .Count(x => x.ManagementGroups.ManagementGroupId == group.ManagementGroupId)
             // *** Add recursive result
             counter += GetNumberOfDevicesForManagementGroup(group.ManagementGroupId);
          }
       }
       else
       {
          // *** Use LINQ to count
          counter = devices.Count(x => x.ManagementGroups.ManagementGroupId == group.ManagementGroupId);
       }
     }
     return counter;
  }