CODEWITHSUNDEEP
c#
loamobn
loamobn

Reputation: 43

Return values in recursion

My question "how do modern languages handle local variables and recursion?" comes from writing a file search method. Basically,

public ArrayList getMusicFiles(string directory){
    ArrayList songpaths = new ArrayList();

    string[] localFiles = System.IO.Directory.GetFiles(directory);
    for(int i=0; i<localFiles.Length-1; i++)
        if(isMusicFile(localFiles[i]))
            songpaths.add(localFiles[i]);

    string[] localFolders = System.IO.Directory.GetDirectories(directory);
    for(int i=0; i<localFolder.length-1; i++)
        getMusicFiles(localFolder[i]);
}

So, the problem is this will redeclare "songpaths" with every recursion. In VB you can declare songpaths as static, which I think would solve this. Is there a good C# way for me to not overwrite the ArrayList?

Upvotes: 0

Views: 165

Answers (5)

cdhowie
cdhowie

Reputation: 168988

Note that I am operating under the assumption that you meant to use the directory parameter in place of the rootDir class-level variable.

You have two options here.

The hacky, but more-efficient way

In this case, you pass the list object through. I am going to use List<string> instead of ArrayList.

public List<string> getMusicFiles(string directory) {
  var list = new List<string>();
  getMusicFiles(list, directory);
  return list;
}

private void getMusicFilesInternal(List<string> songpaths, string directory)
{
  string[] localFiles= System.IO.Directory.GetFiles(directory);
  for(int i=0; i<localFiles.Length-1; i++) {
    if(isMusicFile(localFiles[i])) {
      songpaths.add(localFiles[i]);
    }
  }
  string[] localFolders= System.IO.Directory.GetDirectories(directory);
  for(int i=0; i<localFolder.Length-1; i++) {
    getMusicFiles(songpaths, localFolder[i]);
  }
}

The less-efficient, but functional way

Return the list and aggregate the results each time you recurse:

public IList<string> getMusicFiles(string directory)
{
  List<string> songpaths = new List<string>();
  string[] localFiles= System.IO.Directory.GetFiles(directory);
  for(int i=0; i<localFiles.Length-1; i++) {
    if(isMusicFile(localFiles[i])) {
      songpaths.add(localFiles[i]);
    }
  }
  string[] localFolders= System.IO.Directory.GetDirectories(directory);
  for(int i=0; i<localFolder.Length-1; i++) {
    songpaths.AddRange(getMusicFiles(localFolder[i]));
  }
  return songpaths;
}

You can also implement this using delayed execution, which is still not as efficient as the first example, but gives you much more flexibility with how you use the result:

public IEnumerable<string> getMusicFiles(string directory)
{
  string[] localFiles= System.IO.Directory.GetFiles(directory);
  for(int i=0; i<localFiles.Length-1; i++) {
    if(isMusicFile(localFiles[i])) {
      yield return localFiles[i];
    }
  }
  string[] localFolders= System.IO.Directory.GetDirectories(directory);
  for(int i=0; i<localFolder.Length-1; i++) {
    foreach (var j in getMusicFiles(localFolder[i])) {
      yield return j;
    }
  }
}

This will return an enumerable that will perform the search operation each time you enumerate it, similar to how Linq queries work. You can call ToList() on the result to execute the query and store the results in a list that you can enumerate several times without executing the query again.

If I clean up all of the code, the following is the variant I would probably choose to go with. Your original code has several issues (you subtract one from the arrays' Length properties, even though this will cause you to skip the last element, and there are a few other typos).

public IEnumerable<string> getMusicFiles(string directory)
{
  foreach (var file in System.IO.Directory.GetFiles(directory)) {
    if (isMusicFile(file)) {
      yield return file;
    }
  }

  foreach (var dir in System.IO.Directory.GetDirectories(directory)) {
    foreach (var musicFile in getMusicFiles(dir)) {
      yield return musicFile;
    }
  }
}

If you are worried about the performance of foreach, don't. First of all, you should be coding for readability first and performance second, optimizing only when you find a bottleneck. Second, when you use foreach on an array type, the compiler will turn it into an equivalent Length-based iteration rather than access the array through an IEnumerator<T>.

Upvotes: 8

user7116
user7116

Reputation: 64068

You could opt to use an accumulator strategy, and foist the work to a helper function which requires the array be passed to it:

public List<string> GetMusicFiles(string directory)
{
    List<string> songPaths = new List<string>();

    GetMusicFilesHelper(directory, songPaths);

    return songPaths;
}

private void GetMusicFilesHelper(string directory, List<string> paths)
{
    string[] localFiles = Directory.GetFiles(directory);
    for(int i = 0; i < localFiles.Length; i++) 
    {
        if(isMusicFile(localFiles[i])) paths.Add(localFiles[i]);
    }

    string[] localFolders = Directory.GetDirectories(directory);
    for(int i = 0; i < localFolder.length; i++)
    {
        GetMusicFilesHelper(localFolder[i], paths);
    }
}

Another option would be to skip recursion all together and let Directory.GetFiles do the work for you:

public List<string> GetMusicFiles(string directory)
{
    List<string> songPaths = new List<string>();

    // TODO: pick a better search pattern
    string[] paths = Directory.GetFiles(directory, "*.*", SearchOption.AllDirectories);
    foreach (string path in paths)
    {
        if (IsMusicFile(path))
        {
            songPaths.Add(path);
        }
    }
}

If you're on .Net 4.0+, this becomes a cakewalk with Directory.EnumerateFiles:

public IEnumerable<string> GetMusicFiles(string directory)
{
    return Directory.EnumerateFiles(directory, "*.*", SearchOption.AllDirectories)
                    .Where(ff => IsMusicFile(ff));
}

Upvotes: 2

Jasmine
Jasmine

Reputation: 4029

You can do it by passing in the current list - that way you can append the list in your recursion. Something like this...

public ArrayList getMusicFiles(string directory, ArrayList data, string rootDir) {
    string[] localFiles = System.IO.Directory.GetFiles(rootDir);
    for (int i = 0; i < localFiles.Length - 1; i++) 
        if (isMusicFile(localFiles[i])) 
            songpaths.add(localFiles[i]);

    string[] localFolders = System.IO.Directory.GetDirectories(rootDir);
    for (int i = 0; i < localFolder.length - 1; i++) 
        data.AddRange(getMusicFiles(localFolder[i]));
    return data;
}

Upvotes: 0

David Cummins
David Cummins

Reputation: 988

The code's a bit hard to read, but anyway... I'd suggest writing a private version of the method which is actually used for recursion, and pass the ArrayList as a parameter.

Upvotes: 1

nullptr
nullptr

Reputation: 2274

public ArrayList getMusicFiles(string directory, ArrayList songpaths){
  string[] localFiles= System.Io.Directory.GetFiles(rootDir);

  for(int i=0; i<localFiles.Length-1; i++) if(isMusicFile(localFiles[i]))
      songpaths.add(localFiles[i]);

  string[] localFolders= System.IO.Directory.GetDirectories(rootDir);
for(int i=0; i<localFolder.length-1; i++) getMusicFiles(localFolder[i]);

And then you pass in a new ArrayList() when you first call the method.

Upvotes: -1

Related Questions