Рефакторинг вложил заявление IF для ясности [закрыто]

Я хочу провести рефакторинг этого mumbo jumbo метода, чтобы сделать его более читабельным, так как мне нравятся многие вложенные IF.

Как бы вы рефакторинг это?

public static void HandleUploadedFile(string filename)
{
  try
  {
    if(IsValidFileFormat(filename)
    {
      int folderID = GetFolderIDFromFilename(filename);
      if(folderID > 0)
      {
        if(HasNoViruses(filename)
        {
          if(VerifyFileSize(filename)
          {
            // file is OK
            MoveToSafeFolder(filename);
          }
          else
          {
            DeleteFile(filename);
            LogError("file size invalid");
          }
        }
        else
        {
          DeleteFile(filename);
          LogError("failed virus test");
        }
      }
      else
      {
        DeleteFile(filename);
        LogError("invalid folder ID");
      }
    }
    else
    {
      DeleteFile(filename);
      LogError("invalid file format");
    }
  }
  catch (Exception ex)
  {
    LogError("unknown error", ex.Message);
  }
  finally
  {
    // do some things
  }
}
10.12.2008 14:00:32
Что это домашнее задание? Дупе из stackoverflow.com/questions/348562/…
jmucchiello 10.12.2008 14:48:16
Негативная обратная связь только вредит сайту, когда-нибудь они могут понять это. В любом случае, это был вопрос, который я собирался задать. До голосования.
QueueHammer 16.02.2010 21:10:41
Этот вопрос, кажется, не по теме, потому что может принадлежать codereview.stackexchange.com
Prashant Kumar 27.11.2013 15:14:55
9 ОТВЕТОВ
РЕШЕНИЕ

Я бы поменял условия в тесте на « если плохо», то «deleteAndLog», как в примере ниже. Это предотвращает вложение и помещает действие рядом с тестом.

try{
    if(IsValidFileFormat(filename) == false){
        DeleteFile(filename);
        LogError("invalid file format");
        return;
    }

    int folderID = GetFolderIDFromFilename(filename);
    if(folderID <= 0){
        DeleteFile(filename);
        LogError("invalid folder ID");
        return;
    }
    ...

}...
24
10.12.2008 14:06:55
Это более или менее аналогично моему практическому правилу: «Никогда не проверяйте обычный случай, тестируйте исключения», stackoverflow.com/questions/114342/… .
hlovdal 12.08.2009 21:09:37

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

1
10.12.2008 14:05:25

Реорганизовать здесь не так уж много, поскольку вы держите 3 теста отдельно, поскольку сообщения об ошибках относятся к выполненному тесту. Вы можете выбрать, чтобы тестовые методы сообщали об ошибке в журнал, чтобы их не было в дереве if / else, что могло бы упростить abit, так как тогда вы могли бы просто проверить на наличие ошибок и зарегистрировать их + удалить файл. ,

1
10.12.2008 14:07:57

Охранные пункты.

Для каждого условия отмените его, измените блок else на блок then и вернитесь.

таким образом

if(IsValidFileFormat(filename)
{
   // then
}
else
{
   // else
}

становится:

if(!IsValidFileFormat(filename)
{
    // else 
    return;     
}
// then
10
12.12.2008 13:24:20

Если вы не против использования исключений, вы можете обрабатывать проверки без вложенности.

Предупреждение, воздушный код впереди:

public static void HandleUploadedFile(string filename)
{
  try
  {
    int folderID = GetFolderIDFromFilename(filename);

    if (folderID == 0)
      throw new InvalidFolderException("invalid folder ID");

    if (!IsValidFileFormat(filename))
      throw new InvalidFileException("invalid file format!");

    if (!HasNoViruses(filename))
      throw new VirusFoundException("failed virus test!");

    if (!VerifyFileSize(filename))
      throw new InvalidFileSizeException("file size invalid");

    // file is OK
    MoveToSafeFolder(filename);
  }
  catch (Exception ex)
  {
    DeleteFile(filename);
    LogError(ex.message);
  }
  finally
  {
    // do some things
  }
}
2
10.12.2008 14:22:28
Это аккуратнее, но использование исключений таким образом является запахом кода (извините, но это так). Кроме того, зачем выбрасывать специфически типизированные исключения, когда ВЫ ЗНАЕТЕ, что собираетесь их ловить и относиться ко всем одинаково?
Binary Worrier 10.12.2008 14:58:53
Почему это «кодовый запах»? Кстати, означает ли «запах кода» что-то большее, чем «мне это не нравится»? У меня есть Google, но единственный «запах кода», связанный с найденными исключениями, - это использование их для неисключительных обстоятельств.
Tony Andrews 10.12.2008 15:08:51
1) Я знаю о «запахе кода», но здесь я склонен не соглашаться, и я сказал: «Если вы не против». Я знаю, что исключения запускают разматывание стека и работают медленнее, чем флаги - но кого это волнует, для проверки правильности загрузки файла. 2) Нет необходимости в типизированных исключениях, я знаю. Но это всего лишь пример.
Tomalak 10.12.2008 15:27:34
Код запаха часто может быть в уме смотрящего. Но я должен согласиться с тем, что это больше похоже на использование исключений для управления потоком данных, которые в целом кажутся недовольными.
Torlack 10.12.2008 15:28:55
Это зависит от языка, я думаю. Может быть, больше пользователей скомпилированных языков не любят такой вид использования, чем пользователи интерпретируемых языков, я не знаю. Я могу понять , почему такое использование плохо вообще , но я не совсем понимаю , почему это так плохо здесь . (И - разве вирус не стоит исключения?)
Tomalak 10.12.2008 15:47:33

Это остальные, которые бросают мне глаза. Вот альтернатива, внутри попытки {}

Вы можете сделать это еще короче, возвращая после MoveToSafeFolder (даже если вы возвращаете, будет выполнен блок finally.) Тогда вам не нужно назначать пустую строку для errorMessage, и вам не нужно проверять, является ли errorString пустым перед удалением файла и регистрации сообщения). Я не делал этого здесь, потому что многие находят ранние возвраты оскорбительными, и я согласен в этом случае, так как выполнение блока finally после возврата не интуитивно понятно для многих людей.

Надеюсь это поможет

            string errorMessage = "invalid file format";
            if (IsValidFileFormat(filename))
            {
                errorMessage = "invalid folder ID";
                int folderID = GetFolderIDFromFilename(filename);
                if (folderID > 0)
                {
                    errorMessage = "failed virus test";
                    if (HasNoViruses(filename))
                    {
                        errorMessage = "file size invalid";
                        if (VerifyFileSize(filename))
                        {
                            // file is OK                                        
                            MoveToSafeFolder(filename);
                            errorMessage = "";
                        }
                    }
                }
            }
            if (!string.IsNullOrEmpty(errorMessage))
            {
                DeleteFile(filename);
                LogError(errorMessage);
            }
0
10.12.2008 14:39:27

Я бы к чему то так

public enum FileStates {

MoveToSafeFolder = 1,

InvalidFileSize = 2,

FailedVirusTest = 3,

InvalidFolderID = 4,

InvalidFileFormat = 5,
}


public static void HandleUploadedFile(string filename) {
    try {
        switch (Handledoc(filename)) {
            case FileStates.FailedVirusTest:
                deletefile(filename);
                logerror("Virus");
                break;
            case FileStates.InvalidFileFormat:
                deletefile(filename);
                logerror("Invalid File format");
                break;
            case FileStates.InvalidFileSize:
                deletefile(filename);
                logerror("Invalid File Size");
                break;
            case FileStates.InvalidFolderID:
                deletefile(filename);
                logerror("Invalid Folder ID");
                break;
            case FileStates.MoveToSafeFolder:
                MoveToSafeFolder(filename);
                break;
        }
    }
    catch (Exception ex) {
        logerror("unknown error", ex.Message);
    }
}

private static FileStates Handledoc(string filename) {
    if (isvalidfileformat(filename)) {
        return FileStates.InvalidFileFormat;
    }
    if ((getfolderidfromfilename(filename) <= 0)) {
        return FileStates.InvalidFolderID;
    }
    if ((HasNoViruses(filename) == false)) {
        return FileStates.FailedVirusTest;
    }
    if ((VerifyFileSize(filename) == false)) {
        return FileStates.InvalidFileSize;
    }
    return FileStates.MoveToSafeFolder;
}
0
10.12.2008 14:53:26

В ответе Дэвида Уотерса мне не нравится повторяющийся шаблон DeleteFile LogError. Я бы либо написал вспомогательный метод с именем DeleteFileAndLog (строковый файл, строковая ошибка), либо я написал бы такой код:

public static void HandleUploadedFile(string filename)
{
    try
    {
        string errorMessage = TestForInvalidFile(filename);
        if (errorMessage != null)
        {
             LogError(errorMessage);
             DeleteFile(filename);
        }
        else
        {
            MoveToSafeFolder(filename);
        }
    }
    catch (Exception err)
    {
        LogError(err.Message);
        DeleteFile(filename);
    }
    finally { /* */ }
}

private static string TestForInvalidFile(filename)
{
    if (!IsValidFormat(filename))
        return "invalid file format.";
    if (!IsValidFolder(filename))
        return "invalid folder.";
    if (!IsVirusFree(filename))
        return "has viruses";
    if (!IsValidSize(filename))
        return "invalid size.";
    // ... etc ...
    return null;
 }
1
10.12.2008 14:53:56
DeleteFileAndLog не звучит как очень хорошее имя. Удаляет ли он файл и журнал (имя подразумевает это), или он удаляет файл, а затем регистрирует событие (что, я полагаю, подразумевается).
hlovdal 12.08.2009 21:19:17

Как насчет этого?

public static void HandleUploadedFile(string filename)
{  
   try  
   {    
       if(!IsValidFileFormat(filename))        
       { DeleteAndLog(filename, "invalid file format"); return; }
       if(GetFolderIDFromFilename(filename)==0) 
       { DeleteAndLog(filename, "invalid folder ID");   return; }
       if(!HasNoViruses(filename))             
       { DeleteAndLog(filename, "failed virus test");   return; }
       if(!!VerifyFileSize(filename))          
       { DeleteAndLog(filename, "file size invalid");   return; }
       // --------------------------------------------------------
       MoveToSafeFolder(filename); 
   }
   catch (Exception ex)  { LogError("unknown error", ex.Message); throw; }
   finally {    // do some things  }
}     
private void DeleteAndLog(string fileName, string logMessage)
{
    DeleteFile(fileName);
    LogError(logMessage));
}

или, что еще лучше, ... это:

public static void HandleUploadedFile(string filename)
{  
   try  
   {    
       if(ValidateUploadedFile(filename))       
           MoveToSafeFolder(filename); 
   }
   catch (Exception ex)  { LogError("unknown error", ex.Message); throw; }
   finally {    // do some things  }
} 
private bool ValidateUploadedFile(string fileName)
{
   if(!IsValidFileFormat(filename))        
    { DeleteAndLog(filename, "invalid file format"); return false; }
   if(GetFolderIDFromFilename(filename)==0) 
    { DeleteAndLog(filename, "invalid folder ID");   return false; }
   if(!HasNoViruses(filename))             
    { DeleteAndLog(filename, "failed virus test");   return false; }
   if(!!VerifyFileSize(filename))          
    { DeleteAndLog(filename, "file size invalid");   return false; }
   // ---------------------------------------------------------------
   return true;

}    
private void DeleteAndLog(string fileName, string logMessage)
{
    DeleteFile(fileName);
    LogError(logMessage));
}

ПРИМЕЧАНИЕ: вы не должны ловить и глотать общее исключение, не перебрасывая его ...

0
10.12.2008 15:45:50