Is my code correct and propper?

[garyusenet]

Hi I have finally made progresss with my use of the open file dialog
box!

I have now made my own method that opens the dialog box and returns the
file selected.

Can you please have a quick look at the following code and tell me if
what I have done is correct and good code.

Thankyou very much : -

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;

namespace SiteExtract
{
public partial class Form1 : Form
{
public Form1()
{
InitializeComponent();
}

private string OpenFile()

{
string fileName = "";


OpenFileDialog ofd = new OpenFileDialog();
DialogResult dr = ofd.ShowDialog();
if (dr == DialogResult.OK)
{
fileName = ofd.FileName;

}
return fileName;
}


private void Form1_Load(object sender, EventArgs e)
{
string file = OpenFile();
}
}
}

 

[Marc Gravell]

OpenFileDialog is disposable (so use "using"), and no real purpose in the
variable for the dialog result (since only used once in the very next line),
but otherwise looks OK; my suggestion:

string fileName = "";
using (OpenFileDialog ofd = new OpenFileDialog()) {
if (ofd.ShowDialog() == DialogResult.OK) {
fileName = ofd.FileName;
}
return fileName;
}

You could also possibly "return ofd.FileName" (and return "" or return null
at the bottom), but then you get into holy wars of people saying "one
method, one return". Personally in a short snippet like this it wouldn't
bother me as every path is still very clear; I only mention it for
completeness.

Marc

 

[Marc Gravell]

Or even more terse:

using (OpenFileDialog ofd = new OpenFileDialog()) {
return ofd.ShowDialog() == DialogResult.OK ? ofd.FileName : "";
}

As an aside - you may wish to pass in (perhaps as an overload) an
IWin32Window ("owner") so that the dialog knows that it is bound to the
calling form - can make navigation clearer.

Marc

 

[garyusenet]

Thankyou I never knew of this, and have just read of the using
instruction, and learnt that it is basically a way of instructing the
program to do away with the memory space used by the object without
having to wait for the GC to do so? Does that sound correct?

Can you explain to me what 'disposable' means? I know it means it can
be 'done away with' but what is the more accurate meaning of it here?

Also i'm having trouble understanding the code.

using (OpenFileDialog ofd = new OpenFileDialog()) {
return ofd.ShowDialog() == DialogResult.OK ? ofd.FileName : "";
}

Could you please explain what is happening in the above code as
detailed as you can, i would very much appreciate it.

Finally re:
As an aside - you may wish to pass in (perhaps as an overload) an
IWin32Window ("owner") so that the dialog knows that it is bound to the

calling form - can make navigation clearer.

does this mean for instance that the dialog box might display the name
of the form that called it on it, to help the user? Or is there some
other reason for the dialog box to know which form called it?

Thankyou. It's a steep learning curve, but with the help of this group
it's becoming very rewarding.

Thanks again,

Gary.

 

[Jon Skeet [C# MVP]]

Thankyou I never knew of this, and have just read of the using
instruction, and learnt that it is basically a way of instructing the
program to do away with the memory space used by the object without
having to wait for the GC to do so? Does that sound correct?

No. It's a way of calling the Dispose method on an object which
implements IDisposable. It does *not* free up memory.

Can you explain to me what 'disposable' means? I know it means it can
be 'done away with' but what is the more accurate meaning of it here?

IDisposable is typically implemented by classes which hold an unmanaged
resource, directly or indirectly - things like file handles and UI
resources. The Dispose method says, "I'm done with this object now,
please free all the unmanaged resources now rather than waiting for the
garbage collector".

Also i'm having trouble understanding the code.

using (OpenFileDialog ofd = new OpenFileDialog()) {
return ofd.ShowDialog() == DialogResult.OK ? ofd.FileName : "";
}

It opens a file dialog, and either returns the filename from the dialog
box or an empty string, depending on whether OK was clicked or not.
Whatever happens, any unmanaged resources used by the OpenFileDialog
are released at the end of the using statement.

Jon

 

[Marc Gravell]

See http://msdn2.microsoft.com/en-us/library/system.idisposable.aspx

Note the points on not knowing *when* GC will run. In this case, the
unmanaged resource is the Win32 window handles. Note that sometimes
IDisposable relates to *managed* resources as a convenience to always clean
up after use - for instance I've seen this in bespoke performance loggers
(e.g. using(new PerformanceTimer("SomeCode")) {}), impersonation code
(changing the Principal for a period of time), etc.

Marc

 

[Marc Gravell]

// create a new OpenFileDialog(), and capture the reference in the variable
called "ofd";
// when leaving the following code block, call the .Dispose() method (if ofd
is not null)
using (OpenFileDialog ofd = new OpenFileDialog()) {
// modally show the "ofd" dialog; when complete, test the return value; if
// it returned "OK", then return the filename selected, otherwise return an
empty string
return ofd.ShowDialog() == DialogResult.OK ? ofd.FileName : "";
}

Marc

 

[garyusenet]

Thankyou all very much