Should I refactor these nested foreach statements?

Question

I have some functions that allow a user to search through multiple directories for files of a certain type, and then just the path of those files is added to a listbox. Right now it's done through some nested foreach statements. It's going to be retrieving hundreds of thousands of filepaths, so I was curious what other efficient ways there would be to go about this?

Also, I know it sounds dumb to add that many items to a listbox. I'm only doing what I was told to do. I have a feeling in the future it will be asked to get rid of, but the filepaths will still have to be stored in a list somewhere.

Note: I'm using the WindowsAPICodePack to get a dialogue box that allows multiple directory selection.

List<string> selectedDirectories = new List<string>();

/// <summary>
/// Adds the paths of the directories chosen by the user into a list
/// </summary>
public void AddFilesToList()
{
    selectedDirectories.Clear(); //make sure list is empty

    var dlg = new CommonOpenFileDialog();
    dlg.IsFolderPicker = true;
    dlg.AddToMostRecentlyUsedList = false;
    dlg.AllowNonFileSystemItems = false;
    dlg.EnsureFileExists = true;
    dlg.EnsurePathExists = true;
    dlg.EnsureReadOnly = false;
    dlg.EnsureValidNames = true;
    dlg.Multiselect = true;
    dlg.ShowPlacesList = true;

    if (dlg.ShowDialog() == CommonFileDialogResult.Ok)
    {
        selectedDirectories = dlg.FileNames.ToList(); //add paths of selected directories to list
    }
}

/// <summary>
/// Populates a listbox with all the filepaths of the selected type of file the user has chosen
/// </summary>
public void PopulateListBox()
{
    foreach (string directoryPath in selectedDirectories) //for each directory in list
    {
        foreach (string ext in (dynamic)ImageCB.SelectedValue) //for each file type selected in dropdown
        {
            foreach (string imagePath in Directory.GetFiles(directoryPath, ext, SearchOption.AllDirectories)) //for each file in specified directory w/ specified format(s)
            {
                ListBox1.Items.Add(imagePath); //add file path to listbox
            }
        }
    }
}

Edit: Not sure if it makes a difference, but I'm using the WPF listbox, not winforms.

One word... [Linq](https://msdn.microsoft.com/en-us/library/bb397933.aspx) — David Pine, May 26 '16 at 13:19
.. are you going to be adding "hundreds of thousands of filepaths" to the listbox? — stuartd, May 26 '16 at 13:21
Yes, as of now that's what the person I'm doing this for wants. There's a chance in the future that they won't be displayed in the listbox, but will still have to be saved in a list somewhere. — pfinferno, May 26 '16 at 13:23
@DavidPine Haven't used much Linq before, I'll read into it. I did read somewhere that underneath the hood it does the same basic operations as a foreach loop though. — pfinferno, May 26 '16 at 13:24

score 1 · Answer 1 · edited May 23 '17 at 12:24

1

One way to begin refactoring this outside of learning Linq would be to use the AddRange method. A good explanation as to its performance advantages over a for loop: https://stackoverflow.com/a/9836512/4846465

There's probably no one answer to this question however.

foreach (var directoryPath in selectedDirectories) 
{
    foreach (string ext in (dynamic)ImageCB) 
    {
        ListBox1.Items.AddRange(Directory.GetFiles(directoryPath, ext, SearchOption.AllDirectories).ToArray());
    }
}

edited May 23 '17 at 12:24

Community

1
1

answered May 26 '16 at 13:52

Ryan Peters

180
9

Good idea but I'm using WPF's listbox which doesn't have an `AddRange` property. – pfinferno May 26 '16 at 14:07
1

Perhaps a slight alteration to the above code snippet would be add the results of Directory.GetFiles to a local List then set that List as the ListBoxs.ItemsSource outside the outer most for loop. Good luck, I know little about WPF. – Ryan Peters May 26 '16 at 14:15
Hmm yeah that's a good idea. I'm going to do some performance tests on that vs some other ways. The best bet would probably be to make a view model to store the data and bind the `listbox` to that, but unfortunately the rest of this program isn't written that way. – pfinferno May 26 '16 at 14:26

score -3 · Answer 2 · answered May 26 '16 at 13:21

-3

You can refactor it, or you can leave it how it is.

If you refactor it;

Your code will be more readable, understandable and reusable. You need to write just a couple methods.

And your methods can be usable for another things like your current method.

And works.

If you leave it how it is;

Your code works. But hard to understand and read. Hard to debug in case of bug. But works.

answered May 26 '16 at 13:21

Vecihi Baltacı

352
4
20

Why is the code "hard to understand and read" and "hard to debug"? – stuartd May 26 '16 at 14:26
This code is readable enough, but with LINQ, you can do this more efficiently and with less code – Curious May 26 '16 at 14:38
this code contains multiple nested foreach, but if you split foreaches to methods, you can read more easily. and when you debug, you can understand easily what is wrong. – Vecihi Baltacı May 27 '16 at 06:38

Should I refactor these nested foreach statements?

2 Answers2