Skip to content

Code Quality: Copy and Move collision processing can be improved #18210

@KrisVandermotten

Description

@KrisVandermotten

Description

Copying and moving files are key functionality of any file manager, and detecting and correctly handling collisions is an important step in that process. There are several code quality issues with the collision processing in Files.

  • The method FilesystemHelpers.GetCollision should be called FilesystemHelpers.GetCollisions, plural.
  • In that method, we should be building two lists: nonConflictingItems and conflictingItems. The incomingItems list is an unnecessary intermediary.
  • The custom Zip method used to combine source and destination is inefficient: per item it performs an unnecessary delegate invocation and an unnecessary object allocation. We should just use Enumerable.Zip. The index is equally unnecessary. There really is no need to index into a list to retrieve the element that was just added to it.
  • In that first loop, the collisions dictionary is indexed into twice, where once is sufficient. It's standard TryAdd method does the job just fine.
  • After showing the dialog, we're again building the collisions dictionary, now with the user's choices, and again the standard TryAdd method is a better fit.
  • In the final loop of the FilesystemHelpers.GetCollisions method, we're scanning the collisions dictionary, instead of indexing into it. That makes the algorithm quadratic instead of linear.

The FilesystemHelpers.GetCollisions method is called in two places, CopyItemsAsync and MoveItemsAsync. Both have similar code to update the history with the result of the call.

  • Again an inefficient custom Zip method is used. Enumerable.Zip is a better fit.
  • The result of that Zip method is then used to populate a dictionary, completely unnecessary.
  • For readability of the code, we should use meaningful variable names. item and item2 don't really help when trying to understand the code, nor do Key and Value.
  • We should remove the unused Stopwatch in MoveItemsAsync.

Concerned code

  • src\Files.App\Utils\Storage\Operations\FilesystemHelpers.cs
  • src\Files.Shared\Extensions\EnumerableExtensions.cs

Gains

  • Better readability of the code.
  • Better performance, especially when copying or moving large numbers of items.

Requirements

  • Fix the issues described above.

Comments

I have prepared a pull request.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    Status

    🆕 New

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions