5
\$\begingroup\$

I have this code:

private void numWidth_ValueChanged(object sender, EventArgs e)
{
    if (ignoreChange) return; // Sometimes I only want to change the value without the event firing (like when undo-ing)

    PushUndoStack();

    int width = (int)numWidth.Value;

    foreach (Layer layer in doc.TileLayers)
        layer.Width = width;

    hscDoc.Maximum = doc.WidthInPixels;
    ClampScrollbarValue(hscDoc);
}

private void numHeight_ValueChanged(object sender, EventArgs e)
{
    if (ignoreChange) return;

    PushUndoStack();

    int height = (int)numHeight.Value;

    foreach (Layer layer in doc.TileLayers)
        layer.Height = height;

    vscDoc.Maximum = doc.HeightInPixels;
    ClampScrollbarValue(vscDoc);
}

They are both very similar yet I can't really think of a good way to combine the two. This happens several times in my program and the lines of code are really adding up. I suppose I could use reflection but is it overkill? Can anyone think of a better way?

This is just an example. This happens quite a few times in my program where functions are very similar but cannot be easily refactored in to a generic method.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ There is a site for code-reviews on SE, check it out ;) Apart from that, you could in that case (adapt where necessary) create a dimensionChanged method that takes sender, e and a (boolean? string? int? I favor int...) variable indicating direction. \$\endgroup\$
    – Kheldar
    Aug 31 '11 at 22:53
5
\$\begingroup\$

Something along these lines:

private void ValueChangedImp(object sender, EventArgs e, Func<int> get_value, Action<Layer, int> set_value, Func<int> get_pixels)
{
    if (ignoreChange) return; // Sometimes I only want to change the value without the event firing (like when undo-ing)

    PushUndoStack();

    int value = get_value();

    foreach (Layer layer in doc.TileLayers)
        set_value(laywe, value);

    hscDoc.Maximum = get_pixels();
    ClampScrollbarValue(hscDoc);
}

private void numWidth_ValueChanged(object sender, EventArgs e)
{
    ValueChangedImp(sender, e, () => (int)numWidth.Value, (layer, value) => layer.Width = value, () => doc.WidthInPixels);
}

private void numHeight_ValueChanged(object sender, EventArgs e)
{
    ValueChangedImp(sender, e, () => (int)numHeight.Value, (layer, value) => layer.Height = value, () => doc.HeightInPixels);
}
\$\endgroup\$
1
  • \$\begingroup\$ The scrollbar is not always hscDoc but I get the idea. Forgot all about these features in C# \$\endgroup\$
    – Andrew Godfrey
    Aug 31 '11 at 23:47
2
\$\begingroup\$

You can try the idea below.

    private void numWidth_ValueChanged(object sender, EventArgs e)
    {
        if (OnValueChanged((layer, value) => layer.Width = value, (int)numWidth.Value))
        {
            // Can be refactored in a similar
            // hscDoc.Maximum = doc.WidthInPixels;
            // ClampScrollbarValue(hscMap);
        }
    }

    private void numHeight_ValueChanged(object sender, EventArgs e)
    {
        if (OnValueChanged((layer, value) => layer.Height = value, (int)numHeight.Value));
        {
            // Can be refactored in a similar way
            // vscDoc.Maximum = doc.HeightInPixels;
            // ClampScrollbarValue(vscMap);
        }
    }

    private bool OnValueChanged<TValue>(Action<Layer, TValue> setter, TValue value)
    {
        if (ignoreChange) return false;

        PushUndoStack();

        foreach (Layer layer in doc.TileLayers)
            setter(layer, value);

        return true;
    }
\$\endgroup\$
2
\$\begingroup\$

while the other solutions show ways of injecting code into a templated function...which is useful. But can lead to 'coincidental coupling'. Its a good approach in some situations.

But potentially I'd opt for...

I'd also probably work to move UpdateDimensions to a view model and for it to take the height and width as a parameter.

private void numWidth_ValueChanged(object sender, EventArgs e)
{
    UpdateDimensions();
}
private void numHeight_ValueChanged(object sender, EventArgs e)
{
    UpdateDimensions();
}

private void UpdateDimensions()
{
    if (ignoreChange) return; 
    PushUndoStack();

    int width = (int)numWidth.Value;
    int height = (int)numHeight.Value;

    foreach (Layer layer in doc.TileLayers)
    {
        layer.Width = width;
        layer.Height = height;
    }
    hscDoc.Maximum = doc.WidthInPixels;
    ClampScrollbarValue(hscDoc);
    vscDoc.Maximum = doc.HeightInPixels;
    ClampScrollbarValue(vscDoc);
}
\$\endgroup\$

Your Answer

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