4
\$\begingroup\$

I have around 150 lines of code that I managed to optimize.

But, because my two methods and the six if-statements almost are identical, could there be room for improvement? Maybe it could be done with only one button and one listbox?

As I'm out of ideas and could learn more myself, I'd like to know what could be done to improve this code.

public Form1()
    { 
      InitializeComponent();
      timer1.Interval = 100;
      timer1.Start();
    }

    private void timer1_Tick(object sender, EventArgs e)
    {
      lblClock.Text = DateTime.Now.ToString("HH:mm:ss");
    }
        // ADD values BUTTON
    private void button8_Click(object sender, EventArgs e)
    {
        AddValues();
    }

        // REMOVE values BUTTON
    private void button7_Click(object sender, EventArgs e)
    {
        RemoveValues();
    }

    private void AddValues()
    {
            // ADD hours
        if (!box_Hour.Items.Contains(DateTime.Now.Hour))
        {
            box_Hour.Items.Add(DateTime.Now.Hour);    
        }
            // ADD minutes
        if (!box_Minute.Items.Contains(DateTime.Now.Minute))
        {
            box_Minute.Items.Add(DateTime.Now.Minute);
        }
            // ADD seconds
        if (!box_Second.Items.Contains(DateTime.Now.Second))
        {
            box_Second.Items.Add(DateTime.Now.Second);
        }

    }

    private void RemoveValues()
    {
            // REMOVE hours
        if (box_Hour.Items.Contains(DateTime.Now.Hour))
        {
            box_Hour.Items.Remove(DateTime.Now.Hour);
        }
            // REMOVE minutes
        if (box_Minute.Items.Contains(DateTime.Now.Minute))
        {
            box_Minute.Items.Remove(DateTime.Now.Minute);
        }
            // REMOVE seconds
        if (box_Second.Items.Contains(DateTime.Now.Second))
        {
            box_Second.Items.Remove(DateTime.Now.Second);
        }
    }
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Please, don't call your controls like button7, change that to descriptive names. \$\endgroup\$
    – svick
    Aug 31 '13 at 11:09
  • \$\begingroup\$ I will keep that in mind . ^^ \$\endgroup\$
    – xoxo_tw
    Aug 31 '13 at 12:00
1
\$\begingroup\$

Yeah, you can simplify your code. To have a single method for both of them, you would need to specify whether the check was negated and what action to perform (that could be done using a delegate).

To simplify it even more, use a loop over the three boxes (possibly using a Dictionary to connect each box to the date part).

Also, when working with DateTime.Now, you should make sure to access that property only once, then use its value. If you don't, you could get inconsistent results if the time changes while your method is executing.

private void AddValues()
{
    ProcessValues(true, (items, value) => items.Add(value));
}

private void RemoveValues()
{
    ProcessValues(false, (items, value) => items.Remove(value));
}

private void ProcessValues(
    bool negateContains, Action<ListBox.ObjectCollection, int> successAction)
{
    var now = DateTime.Now;

    var pairs = new Dictionary<ListBox, int>
    {
        { box_Hour, now.Hour },
        { box_Minute, now.Minute },
        { box_Second, now.Second }
    };

    foreach (var pair in pairs)
    {
        bool condition = pair.Key.Items.Contains(pair.Value);

        if (negateContains)
            condition = !condition;

        if (condition)
            successAction(pair.Key.Items, pair.Value);
    }
}

This makes your code less repetitive, but also more complicated. It's up to you to decide if that's worth it.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, I will try to play around with your example, Hopefully I learn something new ^_^, great ! \$\endgroup\$
    – xoxo_tw
    Aug 31 '13 at 12:03

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.