Локальная переменная внутри рекурсивной функции не назначена - PullRequest
1 голос
/ 14 февраля 2010

У меня есть следующий код, есть идеи, как решить эту проблему, вместо объявления переменной int вне функции? Я получаю следующую ошибку компилятора: Использование неназначенной локальной переменной '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;
  }

Ответы [ 5 ]

8 голосов
/ 14 февраля 2010

Кажется, много вещей неправильно с этой функцией.

  1. У вас есть рекурсивная функция, которая создает новый контекст сущности - и рекурсивно перед удалением контекста! Таким образом, это не только создает тонну избыточных ObjectContext экземпляров, но и все они используются одновременно . Все это должно быть полностью переписано, чтобы разделить контекст между вызовами функций.

  2. Вы создаете ObjectContext в статическом методе . Это действительно плохой дизайн. Особенно учитывая название этого метода, может показаться, что вы злоупотребляете статическими методами, чтобы реализовать эффективный процедурный код. Это должен быть метод экземпляра, и класс, вероятно, должен быть тем, который на самом деле поддерживает ObjectContext.

  3. У вас есть такие строки: GetNumberOfDevicesForManagementGroup(managementGroupId, firstTime);. Они ничего не делают, кроме ненужных циклов ЦП и времени базы данных. Вы отбрасываете результаты, которые вы получаете от них. Похоже, вы думаете, что последовательные исполнения GetNumberOfDevicesForManagementGroup будут использовать одну и ту же переменную counter; это не то, как работает рекурсия, это не то, как подпрограммы работают, и создание counter глобальной переменной для компенсации неправильно неправильно неправильно .

  4. Вместо того, чтобы фактически получать счет в каждом случае, вы просто загружаете все «устройства» и подсчитываете их один за другим. Опять же, это огромная трата времени процессора и базы данных.

  5. Вы выполняете запрос к базе данных в цикле. Хлоп.

  6. Две строки firstTime = false; и counter = 0; в первом блоке if вообще ничего не делают. Вы присваиваете аргументы функции. Это не-операции.

  7. На самом деле вы никогда не инициализируете counter для блока else, поэтому ошибка компилятора на самом деле не удивительна. Если вы хотите увеличить переменную, например counter++, она должна где-то начинаться.

Честно говоря, похоже, что это возникло как какой-то грубый процедурный код, который был случайно "преобразован" в C #. Вам нужно полностью переписать этот метод. Вам, вероятно, нужно переделать большую часть вашего дизайна.


Вот пример переписанного класса, который, если я правильно понял ваш код, выполнит ту же задачу (получение количества устройств для одной группы управления и всех групп управления в ее поддереве):

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;
    }
}

Обратите внимание, что это все еще не будет работать очень хорошо, потому что это забивает базу данных новым запросом для каждой дочерней группы; для того, чтобы довести это до нуля, вам нужно реализовать рекурсивный запрос в самой базе данных.

4 голосов
/ 14 февраля 2010

Вы, похоже, неправильно поняли, как работают рекурсивные функции: вы не возвращаете результат (, хотя в конце функции return!). Вы, кажется, думаете, что counter является общим для рекурсивных вызовов - но дело обстоит наоборот. Фактически, сам принцип рекурсии основан на том, что нет совместного использования.

Каждый рекурсивный вызов получает новую counter переменную. Ваша задача - сложить все эти результаты вместе. Например, возьмите только якорь рекурсии:

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

Это неправильно; это должно действительно выглядеть так:

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

Здесь важно вернуть результат. Но настройка firstTime не нужна (и необычна), и настройка counter вообще также не нужна.

Остальная часть тела метода должна быть соответственно изменена.

(Кроме того, этот рекурсивный якорь кажется бессмысленным. Его также можно было бы опустить.)

0 голосов
/ 14 февраля 2010

напишите так:

Вы можете вставить значение счетчика в качестве параметра.

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;
  }

Вы можете вставить значение счетчика в качестве параметра.

0 голосов
/ 14 февраля 2010

Как и предлагали другие, если вы инициализируете counter нулем при объявлении, вы решаете проблему компиляции. Но только этот.

Но ... Почему вы используете аргумент firstTime в качестве аккумулятора? Почему бы не удалить его полностью и не использовать аккумуляторы вообще?

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;
  }
0 голосов
/ 14 февраля 2010

Инициализируйте counter на ноль, чтобы заставить компилятор замолчать!На самом деле Почему бы не сделать его статичным вне функции?

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

Редактировать: Кажется, что ОП хочет иметь переменную внутри области действия функции, а другойспособ сделать это состоит в том, чтобы изменить сигнатуру функции, чтобы включить параметр для переменной counter, и вместо этого сделать ее параметром out ...

Надеюсь, это поможет, С уважением, Том.1013 *

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...