1
\$\begingroup\$

I'm building a program that allows the user to monitor files on their local system. To display the files, I created a TreeView using System.Windows.Controls from the WPF framework. The files are added recursively.

  1. What suggestions would you give to improve this code for cleanliness, structure, or efficiency?

  2. Could I have made better use of the C# and WPF libraries?

  3. Could this code be made more scalable, in the event of having to display hundreds or thousands of files?

Any guidance from the Code Review community would be welcomed and appreciated.

using System.Collections.Generic;
using System.Linq;
using System.Windows.Controls;

namespace WpfApp1
{
    /// <summary>
    /// A class for creating a Windows File Explorer tree view.
    /// </summary>
    public class FileExplorerTreeView
    {
        /// <summary>
        /// The public <see cref="TreeView"/> property to bind to the UI.
        /// </summary>
        public TreeView FileTree { get; }

        /// <summary>
        /// The <see cref="FileExplorerTreeView"/> class constructor. 
        /// </summary>
        public FileExplorerTreeView()
        {
            FileTree = new TreeView();
        }

        /// <summary>
        /// Add a single path to the <see cref="TreeView"/>.
        /// </summary>
        public void AddPath(string path)
        {
            // Split the file path components then add them to a Queue.
            var pathElements = new Queue<string>(path.Split(Path.DirectorySeparatorChar).ToList());
            AddNodes(pathElements, FileTree.Items);
        }

        /// <summary>
        /// Add multiple paths to the <see cref="TreeView"/>.
        /// </summary>
        public void AddPaths(IEnumerable<string> paths)
        {
            foreach (var path in paths) AddPath(path);
        }

        // Add each file path element recursively to the TreeView.
        private void AddNodes(Queue<string> pathElements, ItemCollection childItems)
        {
            if (pathElements.Count == 0) return;
            var first = new TreeViewItem();
            TreeViewItem? match;
            first.Header = pathElements.Dequeue();

            if (HasItem(childItems, first, out match))
            {
                AddNodes(pathElements, match.Items);
            }
            else
            {
                childItems.Add(first);
                AddNodes(pathElements, first.Items);
            }
        }

        // If the TreeViewItem is contained in childItems, return true and return the "item" object as an out
        // parameter.
        private bool HasItem(ItemCollection childItems, TreeViewItem item, out TreeViewItem? match)
        {
            foreach (var childItem in childItems)
            {
                var cast = childItem as TreeViewItem;
                if (cast == null)
                {
                    match = null;
                    return false;
                }
                if (item.Header.Equals(cast.Header))
                {
                    match = cast;
                    return true;
                }
            }
            match = null;
            return false;
        }
    }
}

Here is the MainWindow class along with some client code to test FileExplorerTreeView with:

using System.Collections.Generic;
using System.Windows;

namespace WpfApp1
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        public MainWindow()
        {
            InitializeComponent();

            var paths = new List<string>()
            { 
                "C:\\DIR\\ANOTHERDIR\\FOO.TXT",
                "C:\\DIR\\FOO.TXT",
                "C:\\ANOTHERDIR\\FOO.TXT",
                "C:\\DIR\\ANOTHERDIR\\BAR.TXT",
                "C:\\DIR\\DIR\\DIR\\FOO.TXT",
            };

            var tree = new FileExplorerTreeView();
            tree.AddPaths(paths);
            DataContext = tree;
        }
    }
}

And here is the XAML:

<Window x:Class="WpfApp1.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:WpfApp1"
        mc:Ignorable="d"
        Title="MainWindow" Height="450" Width="800">
    
    <Grid>
        <TreeView 
            x:Name="FileTreeDisplayed" 
            ItemsSource="{Binding FileTree.Items}"/>
    </Grid>

</Window>
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Comments

I am not a really big fan of adding documentation comments to trivial things (like this is the constructor). On the other hand I do appreciate consistency (every public member is annotated).

I would like to encourage you try to avoid echoing comments (the comment tells exactly the same thing as the code), like this

// Split the file path components then add them to a Queue.
var pathElements = new Queue<string>(path.Split(Path.DirectorySeparatorChar).ToList());

I try to follow this guideline: use comments to document the whys and why nots. In other words, you should capture the reasoning why did you prefer this solution over other viable alternatives. This information is known at the time of code writing but it will be lost if it is not captured as a comment. This could help future code maintainers to better understand your intent.

Readonly

At the first glace this code seems to be fine to expose the TreeView as read-only:

public TreeView FileTree { get; }

The problem with this: its Items collection is not exposed as read-only. In other words, both the FileExplorerTreeView class and its consumer(s) can manipulate concurrently the Items collection. Without proper locking this can cause problems.

Ease of usage

The AddPaths provides a way to specify multiple paths at the same time. But as you have showed in the MainWindow code you had to create a List and pass that to the AddPaths methods.

In C# you could use the params keyword to ease the usage of the method:

void AddPaths(params string[] paths)
...
tree.AddPaths("C:\\DIR\\ANOTHERDIR\\FOO.TXT", ..., "C:\\DIR\\DIR\\DIR\\FOO.TXT");

Naming convention

Your HasItem method follows the tester-doer pattern (sometimes referred as error hiding pattern). Most of the cases in .NET these methods are prefixed with Try, for example TryParse, TryCreate, TryAdd or TryGetValue. The TryGetMatch IMHO would be a more convenient name for this method.

Single return

In your code you have used 3 return statements. 2 out of 3 are used as early exits. As an alternative you could take advantage of break:

private bool TryGetMatch(ItemCollection childItems, TreeViewItem item, out TreeViewItem? match)
{
    bool result = false;
    match = default; 
    
    foreach (var childItem in childItems)
    {
        var cast = childItem as TreeViewItem;
        if (cast == null)
            break;

        if (item.Header.Equals(cast.Header))
        {
            match = cast;
            result = true;
            break;
        }
    }

    return result;
}

Depending on your C# you could take advantage of the is pattern matching:

private bool TryGetMatch(ItemCollection childItems, TreeViewItem item, out TreeViewItem? match)
{
    bool result = false;
    match = default;
    
    foreach (var childItem in childItems)
    {
        if (childItem is TreeViewItem viewItem)
        {
            if (!item.Header.Equals(viewItem.Header))
                continue;
            
            match = viewItem;
            result = true;
        }
        break;
    }

    return result;
}

UPDATE #1

  1. You pointed out that the TreeView is readonly, but the Items collection is not. Is it even possible to make Items readonly since this is a built in class?

Well you don't need to expose the entire TreeView component. It is enough to expose only its Items property. This property's data type is ItemCollection which implements the IList. So, exposing the ItemCollection as readonly prevents you to overwrite the property with other ItemCollection instance but it still allows you to add/remove items from the collection.

private TreeView FileTree { get; }
public ItemCollection Items => FileTree.Items;

So, what can you do? Gladly it is enough (in this particular case) to use the AsEnumerable to prevent modification on the collection.

private TreeView FileTree { get; }
public IEnumerable Items => FileTree.Items.AsEnumerable();
<TreeView x:Name="FileTreeDisplayed" ItemsSource="{Binding Items}"/>
  1. Are there any notable benefits to using the is pattern matching over the as operator?

The as operator can be used only with reference types and nullable types.
So, for instance the following code won't event compile:

object o = 1.0;
var x = o as double;

The is operator and expression do not have this constraint.

var x = o is double; //operator
if(o is double y) //expression
{
}

Another notable difference is that you can define a lots pattern with is whereas as can be used only with types. Here are some examples:

object o = TimeSpan.FromSeconds(1);
if (o is not null) 
{
  //...
}

if (o is TimeSpan { Days: 0, TotalSeconds: > 0 } ts)
{
  // You can use o and ts
}

if (o is TimeSpan { Minutes: 0 or 1 } ts)
{
  // You can use o and ts
}

if (o is TimeSpan { TotalMilliseconds: var milliseconds } ts) 
{
   // You can use o, ts and milliseconds
}
...
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Hi, thanks for the review! Two follow up questions: 1. You pointed out that the TreeView is readonly, but the Items collection is not. Is it even possible to make Items readonly since this is a built in class? 2. Are there any notable benefits to using the is pattern matching over the as operator? \$\endgroup\$
    – Ramza
    Sep 29, 2023 at 1:23
  • 1
    \$\begingroup\$ @Ramza I've updated my post to reflect to your questions. Please check it out :) \$\endgroup\$ Sep 29, 2023 at 7:38
  • 1
    \$\begingroup\$ I'm not sure if AsEnumerable can be called on an ItemCollection because it requires the generic IEnumerable, but ItemCollection inherits from the non-generic IEnumerable. Have I missed something here? \$\endgroup\$
    – Ramza
    Oct 10, 2023 at 12:15
  • \$\begingroup\$ @Ramza Yes, you are right. public IEnumerable Items => FileTree.Items; would be enough. \$\endgroup\$ Oct 10, 2023 at 14:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.