Re: shell32: SHFileOperation improvements

[Mike McCormack]

Rolf,

Thanks for cleaning up SHFileOperation.

Can you please try restructuring your code further to get rid of the 
excessive number of branching points in that function?  Last time I 
counted, SHFileOperation had more than 60 if/while/for/case statements 
in the one +500 line function... that makes it hard to read.

If you were to make use of helper functions more often, you'd improve 
your code's readability, eliminate the need for 2-space indent, the 
pointless /* end-if */ comments, reduce the excessive number of local 
variables and reduce side effects, thus making it easier for other 
people to understand and fix the code.

Try make small functions that fit in ~50 lines.  If a function gets over 
50 lines, cut it up into two new functions.

Mike

Rolf Kalbermatter wrote:
> -
>      if (b_Multi || IsAttribFile(FromAttr))
> -
>        continue;
> -
>      if (IsAttribDir(FromAttr))
> -
>      {
> -
>        /* in w98/w2k/wXp endless recursive move until it aborts due to filename getting to
> -
>    long, we don't do this stupidity but return the according error anyhow here */
> -
>        retCode = ERROR_FILENAME_EXCED_RANGE;
> -
>        break;
> -
>      }
>  }
> -
>    }
> -
>    if (( b_SameRoot_MOVE &&  (b_Multi) && !(b_MultiFrom) && IsAttribFile(FromAttr) && IsAttribDir(ToAttr))
> -
>     || (!(b_MultiFrom) && IsAttribFile(FromAttr) && IsAttribFile(ToAttr))
> -
>     || ( (b_Multi) && IsAttribDir(FromAttr)  && IsAttribDir(ToAttr))
> -
>     || (!b_SameRoot_MOVE && IsAttribDir(FromAttr) && IsAttribFile(ToAttr))
> -
>       )
> -
>    {
> -
>      ToAttr = SHRenameOnCollision(&nFileOp, pToFile + 1, pFromFile + 1);
> -
>    } /* endif */
> -
>  } /* endif */
> -
>  if (!(b_MultiFrom) && (!FlagRenameOnCollision(&nFileOp) || b_SameRoot_MOVE) &&
> -
>      ((SFSC_IDENTIICAL | SFSC_SOURCE_CONTAINS_TARGET) & f_SameDrive))
> -
>  {
> -
>    if (!FlagRenameOnCollision(&nFileOp) && (IsAttribFile(FromAttr)))
> -
>    {
> -
>      /* target is the same as source */
> -
>      retCode = 0x71;
> -
>    }
> -
>    break;
> -
>  } /* endif */
> -
> -
>  /* Analyzing for moving SourceName to TargetName */
> -
>  if ((b_MultiFrom || !b_Multi) && ((IsAttribFile(FromAttr) && IsAttribDir(ToAttr)) ||
> -
>      (!b_MultiTo && IsAttribDir(ToAttr)) ||
> -
>      (!b_MultiTo && IsAttribDir(FromAttr) && b_MultiFrom && !b_SameRoot_MOVE)))
> -
>  {
> -
>    SHFileStrCpyCatW(pTempTo, NULL, pFromFile);
> -
>    /* without FOF_MULTIDESTFILES shlfileop would create dir's recursively */
> -
>    nFileOp.fFlags |= FOF_MULTIDESTFILES;
> -
>    retCode = SHFileOperationW(&nFileOp);
> -
>    continue;
> -
>  }
> -
>  /* What can we do with one pair and FO_MOVE/FO_COPY ? */
> -
>  /* w98se, nt40 does create recursive dirs, W2K not! wMe ist not tested */
> -
>  if (IsAttribDir(ToPathAttr) || !b_SameRoot_MOVE)
> -
>  {
> -
>    if (!b_MultiFrom)
> -
>    {
> -
>      if (IsAttribFile(FromAttr))
> -
>      {
> -
>        if (b_SameRoot_MOVE &&
> -
>          /* windows-bug, MOVE for File also with pToTailSlash, COPY not for this */
> -
>           (!(IsAttribFile(ToAttr)) || (!(FlagAskOverwrite(&nFileOp)) && FlagNotOverwrite(&nFileOp))) &&
> -
>           (IsAttribDir(ToPathAttr) || b_ToMask))
> -
>        {
> -
>          /* With the same drive, we can move for FO_MOVE, dir to dir is solved later */
> -
>          retCode = SHNotifyMoveFileW(nFileOp.pFrom, nFileOp.pTo);
> -
>          continue;
> -
>        } /* endif */
> -
>        if (IsAttribDir(ToPathAttr) && !pToTailSlash && !IsAttribDir(ToAttr))
> -
>        {
> -
>          if (!(FOI_NeverOverwrite & nFileOp.fAnyOperationsAborted))
> -
>          {
> -
>            if (!FlagNotOverwrite(&nFileOp))
> -
>            {
> -
>              ToAttr = INVALID_FILE_ATTRIBUTES;
> -
>            }
> -
>            if (IsAttribFile(ToAttr) && FlagNotOverwrite(&nFileOp))
> -
>            {
> -
>              if (FlagAskOverwrite(&nFileOp))
> -
>              {
> -
>                /* FIXME: we must change the dialog in the future to allow 3-4 cases,
> -
>                 * 'Yes','No','Yes for all','Never ?' */
> -
>                if ((INVALID_FILE_ATTRIBUTES != ToAttr) && !SHELL_ConfirmDialogW(ASK_OVERWRITE_FILE, pTempTo))
> -
>                {
> -
>                  retCode = 0x10008;
> -
>                  nFileOp.fAnyOperationsAborted |= TRUE;
> -
>                  break;
> -
>                }
> -
>                ToAttr = INVALID_FILE_ATTRIBUTES;
> -
>              } /* endif */
> -
>            } /* endif */
> -
>          } /* endif */
> -
>          if (INVALID_FILE_ATTRIBUTES == ToAttr)
> -
>          {
> -
>            if (SHNotifyCopyFileW(pTempFrom, pTempTo, FlagRenameOnCollision(&nFileOp)))
> -
>            {
> -
>              /* the retcode for this case is unknown */
> -
>              retCode = 0x10009;
> -
>            }
> -
>            if ((FO_COPY != FuncSwitch) && !retCode)
> -
>            {
> -
>              nFileOp.wFunc = ((level + 1) << FO_LevelShift) + FO_DELETE;
> -
>              retCode = SHFileOperationW(&nFileOp);
> -
>            }
> -
>            continue;
> -
>          }
> -
>        }
> -
>      }
> -
>      if (IsAttribDir(FromAttr))
> -
>      {
> -
>        if (INVALID_FILE_ATTRIBUTES == ToAttr)
> -
>        {
> -
>          SetIfPointer(pToTailSlash, '\0');
> -
>          ToAttr = SHRenameOnCollision(&nFileOp, pToFile + 1, pFromFile + 1);
> -
>          if (INVALID_FILE_ATTRIBUTES != ToAttr)
> -
>          {
> -
>            /* w2k returns ERROR_ALREADY_EXISTS, if it found the target with mask */
>
> +#if 1
> -
>            retCode = ERROR_ALREADY_EXISTS;
>
> +#else
> -
>            retCode = ERROR_INVALID_NAME;
>
> +#endif
> -
>            nFileOp.fAnyOperationsAborted |= TRUE;
> -
>            break;
> -
>          }
> -
>          lpFileName = &pToFile[lstrlenW(pToFile)];
> -
>          if (pToTailSlash)
> -
>          {
> -
>            pToTailSlash = lpFileName;
> -
>            lpFileName++;
> -
>          }
> -
>          ((DWORD*)lpFileName)[0] = '\0';
> -
>          retCode = SHNotifyCreateDirectoryW(pTempTo, NULL);
> -
> -
>          if (retCode)
> -
>          {
> -
>            retCode = (ERROR_PATH_NOT_FOUND | 0x10000); /* nt40,w98se,w2k,wxp */
> -
>            break;
> -
>          }
> -
>          ToAttr = GetFileAttributesW(pTempTo);
> -
>          SetIfPointer(pToTailSlash,'\\');
> -
>          nFileOp.fFlags &= ~FOF_RENAMEONCOLLISION;
> -
>        }
> -
>        if (IsAttribDir(ToAttr))
> -
>        {
> -
>          /* both are existing dirs, we (FO_)MOVE/COPY the content */
> -
>          pToFile = SHFileStrCpyCatW(pTempFrom, NULL, wWildcardFile);
> -
>          nFileOp.fFlags &= ~FOF_MULTIDESTFILES;
> -
>          retCode = SHFileOperationW(&nFileOp);
> -
>          if ((FO_COPY != FuncSwitch) && !retCode)
> -
>          {
> -
>            ((DWORD*)pToFile)[0] = '\0';
> -
>            nFileOp.wFunc =  ((level + 1) << FO_LevelShift) + FO_DELETE;
> -
>            retCode = SHFileOperationW(&nFileOp);
> -
>          }
> -
>          continue;
> -
>        }
> -
>      } /* endif IsAttribDir(from) */
> -
>    } /* endif !b_MultiFrom */
> -
>  }
> -
>  if (!b_SameRoot_MOVE)
> -
>  {
> -
>    nFileOp.fAnyOperationsAborted |= TRUE;
> -
>  } /* endif */
> -
>  retCode = ERROR_CANCELLED;
> - } /* end-while */