Topic: best practice when using dialog result OK

[David Hall]

This code is really ugly, but I'm basically doing a proof of concept. 

I know the error checking is whacked, so I'm looking for pointers on the best way to use either the TRY or the DialogResult.  In my mind I know I need to check to see if it makes it all the way through, and somehow tell the user it finished, thus the MessageBoxes.  Any ideas?

Code: [Select]

        private void btnOK_Click(object sender, EventArgs e)
        {
            string[] filestoCopy;
            openFileDialog1.InitialDirectory = "c:\\";
            openFileDialog1.Filter = "dwg files (*.dwg)|*.dwg|All files (*.*)|*.*";
            openFileDialog1.FilterIndex = 1;
            openFileDialog1.RestoreDirectory = true;
            openFileDialog1.Multiselect = true;
            try
            {
                if (openFileDialog1.ShowDialog() == DialogResult.OK)
                {
                    filestoCopy = openFileDialog1.FileNames;
                    string DirTo = txtFolderTo.Text + "\\";
                    foreach (string fl in filestoCopy)
                    {
                        string destFile;
                        destFile = fl.Substring(fl.LastIndexOf("\\") + 1);
                        destFile = DirTo + destFile;
                        File.Copy(fl, destFile, true);
                        destFile = string.Empty;
                    }
                }
                MessageBox.Show("Files Copied");
            }
            finally
            {
                MessageBox.Show("Process Completed");
            }
        }

[sinc]

Well, the biggest thing I see is that you don't have a "catch" statement.  In general, you want to have one of those, which will print some sort of error message and/or rethrow the exception.  In some cases, you may want to use compiler directives so that you only get an error message in debug builds, and make it silent in your official build, but that approach should be used sparingly and with caution, only when necessary.

Otherwise, the missing catch can "hide" problems from you, and make debugging much more difficult.

As for the rest, it would probably be better to move your "if" statement outside the "try" statement.  Right now, you print "Files Copied" even if the user hits cancel in the dialog box (i.e., if DialogResult is not OK).

In fact, you may even want the "try" block inside your "foreach", so that if one file fails, it will still try to process all the other files.

[David Hall]

Thanks Sinc.  I too came across the ESC button gotcha, where it poped up the box for completed.  That was what got me thinking I needed something more.  Question, if the ==OK fails (or isn't ok) I should add an ELSE to tell the user something?  Too many years of programming for myself, I tend to leave out error checking b/c I "know" how to use the proggy, and I dont skip steps.

[sinc]

It's up to you.

But usually, if that dialog does not return OK, it's because the user canceled the operation, and you may just want to end the entire command.

It depends on your precise usage, however.  The best way to see how it's working is to test it, trying various things like a "Curious User", and make sure it looks like your routine is responding the way you want it to.

[David Hall]

this seems to be better

Code: [Select]

        private void btnOK_Click(object sender, EventArgs e)
        {
            string[] filestoCopy;
            openFileDialog1.InitialDirectory = "c:\\";
            openFileDialog1.Filter = "dwg files (*.dwg)|*.dwg|All files (*.*)|*.*";
            openFileDialog1.FilterIndex = 1;
            openFileDialog1.RestoreDirectory = true;
            openFileDialog1.Multiselect = true;
            int fileCount = 0;
            try
            {
           
                if (openFileDialog1.ShowDialog() == DialogResult.OK)
                {
                    filestoCopy = openFileDialog1.FileNames;
                    string DirTo = txtFolderTo.Text + "\\";
                    foreach (string fl in filestoCopy)
                    {

                        try
                        {
                            string destFile;
                            destFile = fl.Substring(fl.LastIndexOf("\\") + 1);
                            destFile = DirTo + destFile;
                            File.Copy(fl, destFile, true);
                            destFile = string.Empty;
                            fileCount += 1;
                        }
                        catch (Exception exp)
                        {
                            MessageBox.Show(exp.ToString());
                        }

                    }
                }
            }
            finally
            {
                MessageBox.Show(fileCount + " files copied");
            }
        }

[sinc]

Yeah, that's cleaner.

But in that code, I would probably omit the outer try/finally block.  I can't think of any exception that would be caught by that try/finally block, so if something DOES get thrown, the best thing would probably be to let it error out.  Either that, or change it to a try/catch/finally block, and put an error message in the "catch" portion, similar to what you do in the inner try/catch block, so you see an error message if any exception ever does get thrown.

[sinc]

Oh, and as another point, you're using a MessageBox in the inner "catch" statement.  That's OK, but it forces the user to hit OK to continue.  Imagine what would happen if the user selected 100 files, and they all start erroring out - the user might have to hit "OK" 100 times before the routine finally exits.

So you might want to rethink that.  One choice would be to simply print a message to the command line, instead of using a MessageBox, so the user doesn't have to hit OK.  Another would be to remember the filenames that failed, then display all of them in a single dialog box at the end.

Amazing how convoluted just a little routine like this can be, eh?  Now imagine how convoluted large programs that Autocad get, and you can understand why it's so difficult to create large software applications that work simply, easily, and cleanly.

[LE3]

i have done something like - very simplistic:

Code: [Select]

        private void openToolStripButton_Click(object sender, EventArgs e)
        {
            System.Windows.Forms.OpenFileDialog dlgOpen = new System.Windows.Forms.OpenFileDialog();
            dlgOpen.Title = "Select one CSV file";
            dlgOpen.ShowReadOnly = false;
            dlgOpen.Multiselect = false;
            dlgOpen.Filter = "CSV files (*.csv)|*.csv|All files (*.*)|*.*"; //"CSV Files (*.csv)|*.csv";           
            if (dlgOpen.ShowDialog() == DialogResult.OK)
                GetData(dlgOpen.FileName);
            dlgOpen.Dispose();
        }

[Glenn R]

It might be just me, but I tend to get my 'can it run' conditionals out of the way first.

Ex:

Code: [Select]

if (openFileDialog1.ShowDialog() != DialogResult.OK)
    return;

Also, I can't remember off the top of my head if that adesk dialog needs to be disposed...

Other than that Duh, looking gooood!

[David Hall]

Thanks everyone for the help.  The more I keep doing this, the easier it seems to be getting.

[Bobby C. Jones]

It might be just me, but I tend to get my 'can it run' conditionals out of the way first.

Ex:

Code: [Select]

if (openFileDialog1.ShowDialog() != DialogResult.OK)
    return;


I'll second Glenn's suggestion.  Sometimes multiple exit points just make sense; they make the intent clearer and they reduce nesting.

[Glenn R]

... they make the intent clearer and they reduce nesting.

The exact 2 reasons I favour them actually...

[sinc]

I wouldn't say that I favor multiple exit points - that can create a whole class of bugs in itself.

But I admit I do the same thing at times, for the same reasons.  In some situations, it seems to be the best course.  But not in all - I recommend using this technique with caution and restraint.

[Glenn R]

I recommend using it with wanton abandon

[It's Alive!]

My C# is a bit rusty but have a look at System.IO.Path  I.e

Code: [Select]

   static void CopyFiles(IEnumerable<string> files, string destPath)
    {
      if (files == null)
        throw new ArgumentNullException("strings");

      if(String.IsNullOrEmpty(destPath))
        throw new ArgumentNullException("destPath");

      foreach(string file in files)
      {
        File.Copy(file, String.Format("{0}{1}{2}",
          destPath, Path.DirectorySeparatorChar,Path.GetFileName(file)));
      }
    }


[Ken Alexander]

Oh, and as another point, you're using a MessageBox in the inner "catch" statement.  That's OK, but it forces the user to hit OK to continue.  Imagine what would happen if the user selected 100 files, and they all start erroring out - the user might have to hit "OK" 100 times before the routine finally exits.

Another point to go along with this is allowing the user to cancel out of the copy.  Depending on the number and size of files, this could take a while.  You might look at copying files/directories using My.Computer.FileSystem namespace.  My.Computer.FileSystem.CopyFile gives options for this.

[It's Alive!]

Don't worry about multiple exit points in .NET... it's got one of them managed stacks 

[Glenn R]

You could also use Path.Combine(string1, string2) as well...

[David Hall]

Thanks eveyone, I will look into the Path options

[Bobby C. Jones]

I recommend using it with wanton abandon

Me too.  But I'm also a huge fan of jump statements.  I do this all the time.

Code: [Select]

   static void Main(string[] args)
    {
      int i = 0;
      loop_begin:
        if (i == 10)
          goto exit;

        Console.WriteLine(i++);
      goto loop_begin;

      exit:
      Console.ReadKey();
    }


[David Hall]