CODEWITHSUNDEEP
c#refactoring
Leron
Leron

Reputation: 9886

C# converting a long if statement to something more appropriate

I have a MDI form where some of the children forms need a message box showing before being closed and others could be closed without asking. Because of a problem when calling application.Exit() from a close event of a child form I handle to close event of the parent and check where it was fired. If it was fired in a form that need a message box I call it otherwise just close the application. All this is implemented in this code:

private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
        {
            SEdit se       = this.ActiveMdiChild as SEdit;
            SoEdit soleEdit = this.ActiveControl as SoEdit;
            UppEdit ue      = this.ActiveControl as UpEdit;
            MEdit mat  = this.ActiveControl as MEdit;
            LEdit lse      = this.ActiveControl as LEdit;
            CEdit cle    = this.ActiveControl as CEdit;

            if (se != null || soleEdit != null || ue != null || mat != null || lse != null || cle != null)
            {
                if (MessageBox.Show("Do you want to save before exit?", "Closing",
                      MessageBoxButtons.YesNo,
                      MessageBoxIcon.Information) == DialogResult.Yes)
                {
                    MessageBox.Show("To Do saved.", "Status",
                              MessageBoxButtons.OK,
                              MessageBoxIcon.Information);
                }
            }
         }

I'm still learning but I know that such a long if-statement is a sign for bad code but i don't know how to improve it. What is the proper way to handle this situation?

Upvotes: 1

Views: 668

Answers (5)

Sergey Berezovskiy
Sergey Berezovskiy

Reputation: 236308

Extract condition to separate method:

private bool AnyChildAlive()
{
   return (this.ActiveMdiChild is SEdit) ||
          (this.ActiveControl is SoEdit) ||
          ...
          (this.ActiveControl is CEdit);
}

Then call this method (also use guard conditions to avoid nested if statements):

private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
{          
    if (!AnyChildAlive())
       return;

    if (MessageBox.Show("Do you want to save before exit?", "Closing",
           MessageBoxButtons.YesNo, 
           MessageBoxIcon.Information) != DialogResult.Yes)
       return;

   MessageBox.Show("To Do saved.", "Status",
        MessageBoxButtons.OK, MessageBoxIcon.Information);     

}

Upvotes: 6

Khan
Khan

Reputation: 18172

For the sake of making it more appealing on the eye or re-use, you may want to think about moving your validation to a different method.

private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
{
    if (FormIsValid())
    {
        if (MessageBox.Show("Do you want to save before exit?", "Closing",
              MessageBoxButtons.YesNo,
              MessageBoxIcon.Information) == DialogResult.Yes)
        {
            MessageBox.Show("To Do saved.", "Status",
                      MessageBoxButtons.OK,
                      MessageBoxIcon.Information);
        }
    }
}

private bool FormIsValid()
{
    return 
    (this.ActiveMdiChild as SEdit) != null ||
    (this.ActiveControl as SoEdit) != null ||
    (this.ActiveControl as UpEdit) != null ||
    (this.ActiveControl as MEdit)  != null ||
    (this.ActiveControl as LEdit)  != null ||
    (this.ActiveControl as CEdit)  != null;
}

Upvotes: 1

Abdusalam Ben Haj
Abdusalam Ben Haj

Reputation: 5433

Not the best way but you could set the Tag property of the forms to 1 or 0 and check :

          if (this.ActiveControl.Tag == 1)
            {
                if (MessageBox.Show("Do you want to save before exit?", "Closing",
                      MessageBoxButtons.YesNo,
                      MessageBoxIcon.Information) == DialogResult.Yes)
                {
                    MessageBox.Show("To Do saved.", "Status",
                              MessageBoxButtons.OK,
                              MessageBoxIcon.Information);
                }
            }

Upvotes: 0

Fede
Fede

Reputation: 44068

private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
{
  var objects = new List<object>
                {
                    this.ActiveMdiChild as SEdit,
                    this.ActiveControl as SoEdit,
                    this.ActiveControl as UpEdit,
                    this.ActiveControl as LEdit,
                    this.ActiveControl as CEdit
                };

        if (objects.Any(x => x != null))
        {
            if (MessageBox.Show("Do you want to save before exit?", "Closing",
                  MessageBoxButtons.YesNo,
                  MessageBoxIcon.Information) == DialogResult.Yes)
            {
                MessageBox.Show("To Do saved.", "Status",
                          MessageBoxButtons.OK,
                          MessageBoxIcon.Information);
            }
        }
     }

Upvotes: 0

ChrisLively
ChrisLively

Reputation: 88092

Probably the best way would be to create an Interface like:

public interface IFormActions {
  bool AskBeforeClosing();
  void SaveData();
}

Then, implement that interface for each of your forms and in the MainForm_FormClosing method do this:

private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
{
  IFormActions se = this.ActiveControl as IFormActions;
  if ((se != null) && se.AskBeforeClosing()) {
    if (MessageBox.Show("Do you want to save before exit?", "Closing", MessageBoxButtons.YesNo,  MessageBoxIcon.Information) == DialogResult.Yes) {
      se.SaveData();
      MessageBox.Show("Saved", "Status", MessageBoxButtons.OK, MessageBoxIcon.Information);
    }
  }
}

Because of how this is written, you wouldn't have to implement the interface for all of the forms, just the ones you actually want to ask the closing question for.

Upvotes: 2

Related Questions