# Refactoring my windowsFormApp code

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");
}
private void button8_Click(object sender, EventArgs e)
{
}

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

{
if (!box_Hour.Items.Contains(DateTime.Now.Hour))
{
}
if (!box_Minute.Items.Contains(DateTime.Now.Minute))
{
}
if (!box_Second.Items.Contains(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);
}
}

• Please, don't call your controls like button7, change that to descriptive names. Aug 31 '13 at 11:09
• I will keep that in mind . ^^ Aug 31 '13 at 12:00

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()
{
}

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.

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