8
\$\begingroup\$

I have three text fields (DevExpress ButtonEdits set to read only) on a Windows Forms app. They look like this:

enter image description here

They're used by dragging and dropping a single file on each field.

  • If a dropped file belongs on a particular field (correct file type with legal contents), the file is parsed and its contents are used to populate the corresponding place in the applications object model. The text field changes to reflect the location of the loaded file.
  • If a dropped file does not belong on a particular text field (illegal, incorrect, or corrupt contents), an alert message is shown and the application is unaffected. The text field remains blank/unchanged.

I currently achieve this with the following code:

public partial class MainForm : DevExpress.XtraEditors.XtraForm
{

    private void MainForm_Load(object sender, EventArgs e)
    {
        SetDragDropHandlers();
    }

    // Here the drag drop and drag enter handlers are assigned at most once to each field.
    private void SetDragDropHandlers()
    {

        // REVIEWER: SEE REPETITION 1
        /////////////////////////////

        // Using -= then += on a handler ensures only one is ever added.

        this.TextCalibrationFile.AllowDrop = true;
        this.TextCalibrationFile.DragEnter -= new DragEventHandler(ActivateIfSingleFileDrag);
        this.TextCalibrationFile.DragEnter += new DragEventHandler(ActivateIfSingleFileDrag);
        this.TextCalibrationFile.DragDrop -= new DragEventHandler(TryExtractCalibration);
        this.TextCalibrationFile.DragDrop += new DragEventHandler(TryExtractCalibration);

        this.TextWhiteBalance.AllowDrop = true;
        this.TextWhiteBalance.DragEnter -= new DragEventHandler(ActivateIfSingleFileDrag);
        this.TextWhiteBalance.DragEnter += new DragEventHandler(ActivateIfSingleFileDrag);
        this.TextWhiteBalance.DragDrop -= new DragEventHandler(TryExtractWhiteBalance);
        this.TextWhiteBalance.DragDrop += new DragEventHandler(TryExtractWhiteBalance);

        this.TextDarkBalance.AllowDrop = true;
        this.TextDarkBalance.DragEnter -= new DragEventHandler(ActivateIfSingleFileDrag);
        this.TextDarkBalance.DragEnter += new DragEventHandler(ActivateIfSingleFileDrag);
        this.TextDarkBalance.DragDrop -= new DragEventHandler(TryExtractDarkBalance);
        this.TextDarkBalance.DragDrop += new DragEventHandler(TryExtractDarkBalance);
    }

    // This is registered to the DragEnter event to ensure a single file is being dragged.
    private void ActivateIfSingleFileDrag(object sender, DragEventArgs evt)
    {
        if (evt.Data.GetDataPresent(DataFormats.FileDrop) && ((string[])evt.Data.GetData(DataFormats.FileDrop)).Length == 1)
            evt.Effect = DragDropEffects.Copy;
    }

    // This is registered to the calibration-specific DragDrop event to load a calibration into the app.
    private void TryExtractCalibration(object sender, DragEventArgs evt)
    {
        string[] files = (string[])evt.Data.GetData(DataFormats.FileDrop);
        if (files.Length > 0)
        {
            try
            {
                // TODO attempt to load a ChannelSetCalibration from file contents


                // load was successful, so set the UI text to the source file's path
                ((ButtonEdit)sender).EditValue = files[0];
            } catch (ArgumentException ex)
            {
                DisplayAlert("Could not load Calibration from file!\n" + ex.Message);
            }
        }
    }

    // This is registered to the white balance-specific DragDrop event to load a white balance into the app.
    private void TryExtractWhiteBalance(object sender, DragEventArgs evt)
    {

        // REVIEWER: SEE REPETITION 2
        /////////////////////////////

        string[] files = (string[])evt.Data.GetData(DataFormats.FileDrop);
        if (files.Length > 0)
        {
            try
            {
                // TODO attempt to load a white balance ImageCube from file contents


                // load was successful, so set the UI text to the source file's path
                ((ButtonEdit)sender).EditValue = files[0];
            }
            catch (ArgumentException ex)
            {
                DisplayAlert("Could not load White Balance from file!\n" + ex.Message);
            }
        }
    }

    // This is registered to the dark balance-specific DragDrop event to load a dark balance into the app.
    private void TryExtractDarkBalance(object sender, DragEventArgs evt)
    {

        // REVIEWER: SEE REPETITION 2
        /////////////////////////////

        string[] files = (string[])evt.Data.GetData(DataFormats.FileDrop);
        if (files.Length > 0)
        {
            try
            {
                // TODO attempt to load a dark balance ImageCube from file contents


                // load was successful, so set the UI text to the source file's path
                ((ButtonEdit)sender).EditValue = files[0];
            }
            catch (ArgumentException ex)
            {
                DisplayAlert("Could not load Dark Balance from file!\n" + ex.Message);
            }
        }
    }

    private void DisplayAlert(string message)
    {
        var alertInfo = new AlertInfo("Warning", message);
        this.AlertWarning.Show(this, alertInfo);
    }

    public MainForm()
    {
        Load += MainForm_Load;
        InitializeComponent();
    }
}

This works just fine, but my event handling logic looks like it should be much more DRY. Or, to summarize...

My handlers are all wet.

Repetition 1

The three chunks that look like

this.TextCalibrationFile.AllowDrop = true;
this.TextCalibrationFile.DragEnter -= new DragEventHandler(ActivateIfSingleFileDrag);
this.TextCalibrationFile.DragEnter += new DragEventHandler(ActivateIfSingleFileDrag);
this.TextCalibrationFile.DragDrop -= new DragEventHandler(TryExtractCalibration);
this.TextCalibrationFile.DragDrop += new DragEventHandler(TryExtractCalibration);

only differ by the parameter to the DragDrop handler (TryExtractCalibration here). I say my handlers are all wet because these three blocks should be DRYed out into three method calls that look something like

RegisterFileDropHandler(this.TextCalibrationFile, TryExtractCalibration);
RegisterFileDropHandler(this.TextWhiteBalance, TryExtractWhiteBalance);
RegisterFileDropHandler(this.TextDarkBalance, TryExtractDarkBalance);

but I don't know haw to pass methods like parameters in C#!

Repetition 2

Another related problem is the similarity between the three event handler methods themselves. They all contain a body like

string[] files = (string[])evt.Data.GetData(DataFormats.FileDrop);
if (files.Length > 0)
{
    try
    {
        // TODO attempt to load a ChannelSetCalibration from file contents


        // load was successful, so set the UI text to the source file's path
        ((ButtonEdit)sender).EditValue = files[0];
    } catch (ArgumentException ex)
    {
        DisplayAlert("Could not load Calibration from file!\n" + ex.Message);
    }
}

They only differ by their try block bodies and their alert messages. Passing an alert message string into a method is simple enough, but if I try adding one

private void TryExtractCalibration(object sender, DragEventArgs evt, string errorMsg)

then I can no longer use the method to create a DragEventHandler. That is, new DragEventHandler(TryExtractCalibration); becomes illegal.

Even if I knew how to resolve that, the try block body differences still confound me.


Can someone wiser than I am please help me get this code more maintainable? The two blatant code repetitions I mentioned are my main concerns, but I'm open to all criticisms large and small. I'm only a few months old in the worlds of C#, .NET, and Windows development.

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

You actually don't need to do this:

this.SomeControl.SomeEvent += new SomeEventHandler(SomeDelegate);

You can simply do this:

this.SomeControl.SomeEvent += SomeDelegate;

Armed with this knowledge you can create that method you were craving:

private static void RegisterFileDropHandler(ButtonEdit control, DragEventHandler handler)
{
    control.AllowDrop = true;
    control.DragEnter -= ActivateIfSingleFileDrag;
    control.DragEnter += ActivateIfSingleFileDrag;
    control.DragDrop -= handler;
    control.DragDrop += handler;
}

I'll come back to your other point later if I have time :)

Update

As your second bit of replication:

Create another method:

private static void DoSomethingWithFile(ButtonEdit sender, DragEventArgs evt, string errorMessage)
{
    string[] files = (string[])evt.Data.GetData(DataFormats.FileDrop);
    if (!files.Any())
    {
        return;
    }
    try
    {
        // Figure out which button raised the event - something like:
        // sender.Id
        // Then execute a method specific to that button.
    } 
    catch (ArgumentException ex)
    {
        DisplayAlert(string.Format("{0}\n{1}", errorMessage ,ex.Message);
    }
}

Now all of your handlers just call that method:

private void TryExtractCalibration(object sender, DragEventArgs evt)
{
    DoSomethingWithFile((ButtonEdit)sender, evt, "Could not load Calibration from file!");
}
\$\endgroup\$
2
  • \$\begingroup\$ That worked perfectly! Thank you. I hope you can help me DRY out those TryExtract* methods too :) \$\endgroup\$
    – kdbanman
    Aug 12, 2015 at 19:33
  • \$\begingroup\$ @kdbanman - added some more details for you. \$\endgroup\$
    – RobH
    Aug 13, 2015 at 8:47
2
\$\begingroup\$
// Using -= then += on a handler ensures only one is ever added.  

This is basically true, but in your szenario superfluous, because the SetDragDropHandlers() method is only called from the Load event of the form. You just can skip the removing of the handler.


In addition to @RobH's answer:

If you are using C# 6 you could use some syntax sugar by using string interpolation by replacing this

catch (ArgumentException ex)
{
    DisplayAlert(string.Format("{0}\n{1}", errorMessage ,ex.Message);
}  

with

catch (ArgumentException ex)
{
    DisplayAlert($"{errorMessage}\n{ex.Message}");
}

taking into account @RobH comment we should change it to

catch (ArgumentException ex)
{
    DisplayAlert($"{errorMessage}{Environment.NewLine}{ex.Message}");
}
\$\endgroup\$
6
  • \$\begingroup\$ Good comment on the string interpolation! I keep forgetting I have that feature now. But the -= then += are not superfluous. I have no guarantees that the Load event is only fired once. In fact, it usually fires twice. \$\endgroup\$
    – kdbanman
    Aug 13, 2015 at 15:17
  • \$\begingroup\$ About Load: Then you have assigned the handler twice. One time in the constructor and one time in the form designer. Check the InitializeComponents() for a sign of it. \$\endgroup\$
    – Heslacher
    Aug 13, 2015 at 15:20
  • 2
    \$\begingroup\$ +1 Never sure when to suggest C#6.0 featutres... Even better (IMO): $"{errorMessage}{Environment.NewLine}{ex.Message}" \$\endgroup\$
    – RobH
    Aug 13, 2015 at 15:23
  • \$\begingroup\$ I just looked - you're right, it's also assigned in the InitializeComponents() method. However, that's an autogenerated method. If I remove the assignment there, can I be sure it won't pop up again? \$\endgroup\$
    – kdbanman
    Aug 13, 2015 at 15:29
  • 2
    \$\begingroup\$ You should select the form in the designer. In the properties click on the thunderbolt sign select the Load event and delete its content. Now it is safely removed from the InitializedComponents method. \$\endgroup\$
    – Heslacher
    Aug 13, 2015 at 15:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.