Is it OK to overload ShowDialog() so that a child form returns information as an out parameter?

Question

In an earlier question about how to return a string from a dialog window, yapiskan suggested overloading the child form's ShowDialog() method to include an out parameter.

My question is whether or not this is a good approach in C#.

Here is some example code, based on yapiskan's suggestion. In the child form (in this example, it's a form with a textbox on it), you just need to add the ShowDialog overload and assign UI values to the out parameter:

public DialogResult ShowDialog(out string s)
{
    DialogResult result = this.ShowDialog();
    s = this.textBox1.Text;
    return result;
}

And to show the form and retrieve the entered text, you do this:

using (CustomDialog frm = new CustomDialog())
{
    string s;
    if (frm.ShowDialog(out s) == DialogResult.OK)
    {
        // do something with s
    }
}

One advantage I can think of is that this approach forces the user of the CustomDialog form to get the information it contains through the form's ShowDialog method (rather than from a who-knows-what-it's-called method like GetMyData() or something).

Answer 1

Better to have a Public property/method and get the information.

What would you do if you would need 3..4..5 informations, having 5 parameters out? More clean to have accessors to get your information from the Dialog.

Answer 2

Personally I try to avoid out parameters wherever possible, although I understand that like GoTo they are sometimes a necessary evil. I would say that it would be much better to use properties or methods to return the information.

Answer 3

I prefer this one because I don't like the approach of getting result from a property or a method after you have done with the class. After dialog form was shown and closed I think the object should not be used any more because logically you have done with the dialog then why should I use its property or method to get the result?

Answer 4

In my experience, a custom modal dialog that collects only one piece of information is a pretty extreme outlier. Much more common are zero and many.

And a dialog that collects many pieces of data is almost certain to be modified at some point to collect just one more. I'd much rather fix only the code that uses that one new piece of data than every single piece of code that uses the modified dialog.

Also, think about how a developer uses IntelliSense to use your class. He's going to type this:

MyDialog d = new MyDialog();
d.ShowDialog(

...and at that last keystroke, IntelliSense will pop up telling him that he now has to declare three new string variables to hold the out parameters. So he moves the cursor up, and starts typing:

string foo;
string

...and, what was the name of the second parameter again? So it's back down to the open paren, hit CTRL+SPACE, oh yeah, it's bar, back up to the previous line, etc.

The problem with using properties on a custom dialog is that the Form class already has a million properties, and the three or four special ones that you're creating are going to get lost in the mix. To fix this, create a class for the dialog parameters and a Parameters property of that type on the custom dialog. That makes code like this easy to write:

MyDialog d = new MyDialog();
d.Parameters.Foo = "foo";
d.Parameters.Bar = "bar";
d.Parameters.Baz = "baz";

because the parameter names pop up in IntelliSense, and you don't need to declare any variables to hold their values.

Answer 5

My approach is typically to write a method that internally calls ShowDialog, then formats the output data appropriately. For (contrived) example:

public string GetFolderName(){
    if(this.ShowDialog() == DialogResult.OK) {
        return this.FolderName.Text;
    }
    return String.Empty;
}

In most cases I make this method static, and instantiate the dialog itself from within the body of the method - that way the caller doesn't have to deal with form references, or the notion of having to chose which 'show' method to call.

In the non-edge cases of having multiple output values, I typically construct a struct that holds these values, then have my 'Get' function return that struct.

public struct FolderData {
    public static FolderData Empty = new FolderData();

    public string FolderName {get; set;}
    public int FilesInFolder {get; set;}
}

public FolderData GetFolderData(){
    if(this.ShowDialog() == DialogResult.OK) {
        return new FolderData {
            FolderName = this.FolderName.Text;
            FilesInFolder = int.Parse(this.FilesInFolder.Text);
        }
    }
    return FolderData.Empty;
}

Answer 6

@Musigenesis, you really don't want to force client code to break when you change your dialog, and using an out parameter that is only sometimes valid isn't a good design. As @Daok says, when you have more than 1 value returned this starts to get messy and ugly fast.

You also can't force client code to use the result any more than the .net framework ensures that you call properties on a file dialog. You're also not forcing the caller to do anything with the out parameter, all you've forced them to do is to accept a variable that they may not want to use.

If the dialog is very generic this may not be applicable, but instead of chucking all sorts of properties to the dialog itself, have a single method that you use consistently throughout your application, and have that return a specific class that holds the relevant data.

public sealed class MySaveDialogResult
{
    public static MySaveDialogResult NonOkResult(); // Null Object pattern
    public MySaveDialogResult( string filePath ) { ... }

    // encapsulate the dialog result
    public DialogResult DialogResult { get; private set; } 
    // some property that was set in the dialog
    public string FilePath { get; private set; }
    // another property set in the dialog
    public bool AllowOVerwrite { get; private set; }
}

and your dialog is

public MySaveDialog ...
{
    public MySaveDialogResult GetDialogResult() { .... }
}

The essence is a small immutable utility class that also implements null object pattern. The null object is returned whenever the dialog result wasn't OK. Obviously the above is a shot in the dark for your needs, so alter it at will, make inheritance hierarchies, etc.

The main point is to have GetDialogResult(), a single method on the dialog, to returned a class that encapsulates all the relevant dialog data.

---

edit:

@yapiskan wonders why not just 'out' the MyDialogResult versus calling GetDialogResult().

IMO - The points are simply:

1. That's not the convention
2. A method call is trivially easy, and made easier when you follow the 'convention' argument as made above.
3. out is awkward to use. GetDialogResult() is not forcing the caller to write awkward code, and it doesn't force the user to consume the dialog result at the point of invoking the dialog.
4. Normally the dialog isn't re-instantiated or re-shown to get the result, it's already there. Show() and Hide() do just that.

The reality is you're trading a method call for an awkward ShowDialog() syntax. Method calls are cheap, and you can't guarantee the caller will use your out parameter any more than you can guarantee they will call GetDialogResult(). So why bother. Make the thing easy to use, or don't overload ShowDialog in the first place.

Maybe whatever you're sub-classing is funky and acts differently and it's not applicable to your situation, but general design is forms don't go away when you click OK, they go away when they are Disposed() of.

Answer 7

It should not be OK since .net framework does not use this design. In the case of OpenFileDialog class, it has a parameterless ShowDialog() method returning a DialogResult. Once this method called, user is supposed to get the selected files by using the FileName, FileNames, SafeFileName and SafeFileNames methods.

Let's assume that this implented in the "out parameter" way. I would have to write code like this just to get the SafeFileName:

string dummyFileName;
string[] dummyFileNames;
string safeFileName;
string[] dummySafeFileNames;

myDialog.ShowDialog(out dummyFileName, out dummyFileNames, out safeFileName, out dummySafeFileNames);