3
\$\begingroup\$

I think that my code isn't perfect and I need a help to understand how to optimize it.

In my application I have a main form and several forms for data editing. The user can:

  • Add a new item
  • Edit the selected item
  • Delete the selected item
  • Duplicate an item with a new name

You can see an example of my code below. I think that the approach to trigger editing mode isn't good.

An alternative way is to create a new form without parameters and then run methods AddItem(), EditItem(), etc., but I don't think that's a good idea either.

Editor modes:

public enum EditMode { Add, Edit, Delete, Copy };

Main form has several buttons:

    private void AddItemBarButton_ItemClick(object sender, ItemClickEventArgs e)
    {
        ItemForm IF = new ItemForm(EditMode.Add);
        IF.ShowDialog();
        IF.Dispose(); 
    }

    private void EditItemBarButton_ItemClick(object sender, ItemClickEventArgs e)
    {
        if (ActiveItem != null)
        {
            ItemForm IF = new ItemForm(EditMode.Edit, ActiveItem);
            MIF.ShowDialog();
            MIF.Dispose();
        }
    }

    private void DeleteItemBarButton_ItemClick(object sender, ItemClickEventArgs e)
    {
        if (ActiveItem != null)
        {
            ItemForm IF = new ItemForm(EditMode.Delete, ActiveItem);
            MIF.Dispose();
        }
    }

    private void CopyItemBarButton_ItemClick(object sender, ItemClickEventArgs e)
    {
        if (ActiveItem != null)
        {
            ItemForm IF = new ItemForm(EditMode.Delete, ActiveItem, "somenewname");
            MIF.Dispose();
        }
    }

Editor form:

    private Item T = null;
    private EditMode Mode;

    private ItemForm()
    {
        InitializeComponent();    
    }

    public ItemForm(EditMode mode) : this()  
    {
        Mode = mode;
        switch (Mode)
        {
            case EditMode.Add:
                T = new Item();
                T.Sort_Order = DateTime.Now;
                break;
        }
    }

    public ItemForm(EditMode mode, Item item) : this()
    {
        Mode = mode;
        T = item;
        switch (Mode)
        {
            case EditMode.Edit:
                NameEdit.Text = T.Name;
                CountEdit.Text = T.Countnum.ToString();
                break;

            case EditMode.Delete:
                cDB.Item.DeleteOnSubmit(T);
                cDB.SubmitChanges();
                break;
        }
    }

    public ItemForm(EditMode mode, Item item, string new_name) : this()
    {
        Mode = mode;
        switch (Mode)
        {
            case EditMode.Copy:
                T = new Item();
                T.Name = new_name;
                T.Countnum = item.Countnum;
                T.Sort_Order = item.Sort_Order;
                cDB.Item.InsertOnSubmit(T);
                cDB.SubmitChanges();
                break;
        }
    }

    private void OKButton_Click(object sender, EventArgs e)
    {
            T.Name = NameEdit.Text;
            T.Countnum = Convert.ToInt(CountEdit.Text);

            if (Mode == EditMode.Add) cDB.Item.InsertOnSubmit(T);
            cDB.SubmitChanges();

            this.Close();
        }
    }
\$\endgroup\$
6
\$\begingroup\$

A small note on naming:

Being new to your code base, I have no idea what MIF represents and worse, IF looks a lot like if and it confuses my eyes. As a rule of thumb, don't abbreviate names. Spell them out so that the maintainer coming behind you understands what everything is.

\$\endgroup\$
5
\$\begingroup\$

Your constructors don't handle all possible situations. What if I call public ItemForm(EditMode mode) with mode set to Edit? When you're overloading constructors, you have to reduce it to one constructor where you handle everything, if this is possible of course.

Small example with a regular method:

public int Add(int a, int b)
{
    return a + b;
}

public int Add(int a, int b, int c)
{
    return a + b + c;
}

The above is not incorrect, but it should be done like following code:

public int Add(int a, int b)
{
    return Add(a, b, 0);
}

public int Add(int a, int b, int c)
{
    return a + b + c;
}

As you can see, all the logic is kept in one place. This can also be applied to your code:

public ItemForm(EditMode mode) : this(mode, new Item(), "")  
{
}

public ItemForm(EditMode mode, Item item) : this(mode, item, "")
{
}

public ItemForm(EditMode mode, Item item, string new_name) : this()
{
    Mode = mode;

    switch (Mode)
    {
        case EditMode.Add:
            T = item;
            T.Sort_Order = DateTime.Now;
            break;
        case EditMode.Edit:
            T = item;
            NameEdit.Text = T.Name;
            CountEdit.Text = T.Countnum.ToString();
            break;
        case EditMode.Delete:
            cDB.Item.DeleteOnSubmit(T);
            cDB.SubmitChanges();
            break;
        case EditMode.Copy:
            T = new Item();
            T.Name = new_name;
            T.Countnum = item.Countnum;
            T.Sort_Order = item.Sort_Order;
            cDB.Item.InsertOnSubmit(T);
            cDB.SubmitChanges();
            break;
    }
}

As you can see everything is handled in the constructor with all possible parameters. The other ones just call this one with the right parameters and the necessary action is performed.

Now you only have to handle situations correctly. If I pass EditMode.Edit and the item is null you'll get a NullReferenceException. This was also the case with your code though.

Update: (tip from Morten Christiansen)

When dealing with lots of code in a switch statement, you can extract that logic into a method and call that method from the case. Example:

case EditMode.Copy: CopyItem(item, new_name); break;

public void CopyItem(Item item, string newName)
{
    T = new Item();
    T.Name = newName;
    T.Countnum = item.Countnum;
    T.Sort_Order = item.Sort_Order;
    cDB.Item.InsertOnSubmit(T);
    cDB.SubmitChanges();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This kind of code (one switch with a lot of logic) can quickly become very hard to read and I prefer to refactor the content of each case out into a method. Something like this: case EditMode.Add: AddItem(); break;. \$\endgroup\$ – Morten Christiansen Nov 12 '14 at 12:42
  • \$\begingroup\$ @MortenChristiansen Good point, I mentioned it in my answer. \$\endgroup\$ – Abbas Nov 12 '14 at 12:52
  • \$\begingroup\$ But what are the advantages comparing to different constructors? Number of constructors will be equal to number of different methods (add, edit, ...). \$\endgroup\$ – John4 Nov 12 '14 at 13:07
  • \$\begingroup\$ I'm not saying this method of working is the best and I wouldn't write this kind of code. This is just a 'better' way when working with multiple constructors. \$\endgroup\$ – Abbas Nov 12 '14 at 13:09
2
\$\begingroup\$

Debugging the following code to delete an item will lead to digging into ItemForm class, because it is a confusing call.

if (ActiveItem != null)
{
    ItemForm IF = new ItemForm(EditMode.Delete, ActiveItem);
    MIF.Dispose();
}  

A better idea would be to add a static method to ItemForm like

public static void DeleteItem(Item item)
{
    if (item == null){ return; }
    using(ItemForm IF = new ItemForm(EditMode.Delete, item)){}
}

this will change you calling code to

ItemForm.DeleteItem(ActiveItem);

an it is clear what is going on.

Basically, a constructor is for constructing the object. So this approach isn't the best either, but it is at least better.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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