current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

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);
        }
    }
share|improve this question
1  
Please, don't call your controls like button7, change that to descriptive names. –  svick Aug 31 '13 at 11:09
    
I will keep that in mind . ^^ –  xoxo_tw Aug 31 '13 at 12:00
add comment

1 Answer

up vote 1 down vote accepted

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.

share|improve this answer
    
Thanks, I will try to play around with your example, Hopefully I learn something new ^_^, great ! –  xoxo_tw Aug 31 '13 at 12:03
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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