18

I'm making a method to fetch a list of filenames from a server but I have come to a problem that I cannot answer.

The method returns two things:

  • an SftpResult which is an enum with a variety of return codes.
  • the list of filenames.

Of these three signatures:

public static ArrayList GetFileList(string directory, out SftpResult result)

public static SftpResult GetFileList(string directory, out ArrayList fileNames)

public static SftpFileListResult GetFileList(string directory)

(where SftpFileListResult is a composite object of an SftpResult and an ArrayList)

which is preferred and why?

tereško
  • 58,060
  • 25
  • 98
  • 150
Nobody
  • 4,731
  • 7
  • 36
  • 65
  • 1
    What's the purpose of SftpResult? If it's supposed to explain why the call failed, maybe you should throw an exception instead. – Guillaume Sep 10 '10 at 12:55
  • I decided it would be overkill, considering all im doing is reporting the status. It's not a serious problem if it fails and I don't really need to be unwinding the stack just because they typed the wrong directory. I just return a DirectoryNotFound and carry on my business :-) – Nobody Sep 10 '10 at 13:09
  • 1
    Consider abandoning use of ArrayList in favour of IEnumerable. – Eric Lippert Sep 10 '10 at 14:48
  • Thanks Eric, I am trying to do that now. Is there some shorter way of writing this: `return new ReadOnlyCollection((List)(IEnumerable)Connection.SshConnection.GetFileList(directory));`?? The `Connection.SshConnection.GetFileList(directory)` part returns an `ArrayList` – Nobody Sep 10 '10 at 15:34
  • http://stackoverflow.com/questions/810797/which-is-better-return-value-or-out-parameter possibly duplicate :p – Delta76 Sep 13 '10 at 09:43
  • 1
    @rmx Enumerable.Cast and List.AsReadOnly can help here `return ...GetFileList(directory).Cast().ToList().AsReadOnly();` – Gideon Engelberth Sep 13 '10 at 17:33

7 Answers7

32

Personally I prefer the last option (although using a List<T> or ReadOnlyCollection<T> instead of ArrayList). out parameters are basically a way of returning multiple values, and it's generally nicer to encapsulate those.

Another option in .NET 4 would be

Tuple<SftpResult, ArrayList> GetFileList(string directory)

That explicitly says, "this method returns two things... I've packed them together for you for this particular case, but it's not worth further encapsulating them: they're not worth composing in a separate type".

(If you're not using .NET 4 you could always write your own Tuple type.)

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Thanks, I didn't even consider a Tuple. That was my main problem with the third option: I felt like it didnt really warrant a whole new class. – Nobody Sep 10 '10 at 12:38
  • +1 also for tuples, probably one of my most favorite things about .NET 4. – BoltClock Sep 10 '10 at 12:45
  • Can't count how many custom Tuple classes I've created over the years - glad there's finally a real one! Now if I could only get our project to update from 3.4 to 4... – mikemanne Sep 10 '10 at 14:26
  • Man, I want Tuple. :( Never used one before but it looks quite useful. – Dave Sep 10 '10 at 14:32
9

I would prefer to wrap it up in a return object:

class FileResult
{
    public FileResult(SftpResult resultCode, IEnumerable<string> files)
    {
         ResultCode = resultCode;
         FileList = new List<string>(files);
    }
    public SftpResult ResultCode { get; private set; }
    public IEnumerable<string> FileList { get; private set; }
}

It feels a lot cleaner not to use out.

Fredrik Mörk
  • 155,851
  • 29
  • 291
  • 343
4

I prefer the final option. Out parameters are rarely used and can be surprising / confusing to some. Creating a composite result object is a clean solution. The only drawback is that you must create a class solely for this purpose.

Mark Byers
  • 811,555
  • 193
  • 1,581
  • 1,452
3

I would say the third, since it encapsulates the logic that you need in one location. I can only assume that SftpResult and ArrayList return methods should be private and then make up the internal logic of the composite return object.

Woot4Moo
  • 23,987
  • 16
  • 94
  • 151
2

I would make it:

public static bool GetFileList(string directory, out SftpResult result, out ArrayList fileNames)

So there is no confusion about what the function does, and also i would return a bool, if the GetFileList can fail.

Marcus Johansson
  • 2,626
  • 2
  • 24
  • 44
2

If you like designs where one function does two things then I would use a tuple a la Jon or a return object a la Fredrik.

If you wanted to be all OOPy about it you could let the type system do the work:

abstract class FtpResult { ... }
sealed class FileList : FtpResult { ... }
sealed class Error : FtpResult { ... }
...
sealed class FtpService
{
    ...
    public FtpResult GetFileList(string directory) { ... }
    ...
}
...
var result = service.GetFileList(dir);
var error = result as Error;
var list = result as FileList;
if (error != null) { ... }
else if (list != null)
{
    foreach(var name in list.Files) { ... }
}
...
Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
1

Why not provide all three? I prefer not having out parameters in my function calls but I can appreciate that in many times they're needed. If you don't know which signature to support then support all three. Ultimately, the choice is on the side of the caller and the best thing you can do is offer this choice.

Puppy
  • 144,682
  • 38
  • 256
  • 465