2
\$\begingroup\$
  private static void OnSundayChangedCallBack(DependencyObject sender, DependencyPropertyChangedEventArgs e)
    {
        BinaryDataGrid c = sender as BinaryDataGrid;
        if (c != null)
        {

            if (_shouldCallBack)
            {
                if (c.Sunday.Length == 4)
                    return;
                Days day = (Days)c.dataGrid.Items[0];
                byte[] newArray = new byte[c.Sunday.Length + 1];
                newArray[0] = Convert.ToByte(0);
                c.Sunday.CopyTo(newArray, 1);
                day.Value = BitConverter.ToInt32(newArray.Reverse().ToArray(), 0);
                _sun = day.Value;
            }
            else
            {
                _shouldCallBack = true;
                c.Sunday = BitConverter.GetBytes(_sun).Reverse().ToArray().Skip(1).ToArray();
            }
        }
    }

    private static void OnMondayChangedCallBack(DependencyObject sender, DependencyPropertyChangedEventArgs e)
    {

        BinaryDataGrid c = sender as BinaryDataGrid;
        if (c != null)
        {
            if (_shouldCallBack)
            {
                if (c.Monday.Length == 4)
                    return;
                Days day = (Days)c.dataGrid.Items[1];
                byte[] newArray = new byte[c.Monday.Length + 1];
                newArray[0] = Convert.ToByte(0);
                c.Monday.CopyTo(newArray, 1);
                day.Value = BitConverter.ToInt32(newArray.Reverse().ToArray(), 0);
                _mon = day.Value;
            }
            else
            {
                _shouldCallBack = true;
                c.Monday = BitConverter.GetBytes(_mon).Reverse().ToArray().Skip(1).ToArray();
            }
        }
    }

I've got the same code for every week day, how can I generalize this so I don't have 6 duplicates of the code above?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

What you need to do is:

  1. Add a parameter of type System.DayOfWeek called, say, day_of_week.

  2. Replace the fields BinaryDataGrid.Sunday, .Monday, etc. with a Dictionary<K,V> that has a key of type System.DayOfWeek and use day_of_week to access values within that dictionary.

  3. Do the same for your _sun, _mon etc. global variables. (For which you will burn in hell, incidentally.)

If you cannot modify BinaryDataGrid, then write two helper methods, one for getting and one for setting the value of the day member of BinaryDataGrid: Each method has a switch( day_of_week ) and modifies the appropriate member of BinaryDataGrid.

Good luck and have fun.

\$\endgroup\$
5
  • \$\begingroup\$ Hi thanks for your suggestion, i'll definately improve that, but kindly enlighten me on 3rd point, (For which you will burn in hell, incidentally.) If this is that bad, guide me towards something that will help me fix that and also why this is so bad. \$\endgroup\$
    – MegaMind
    Dec 19, 2011 at 15:43
  • \$\begingroup\$ From wikipedia (en.wikipedia.org/wiki/Global_variable): "They are usually considered bad practice precisely because of their non-locality: a global variable can potentially be modified from anywhere (unless they reside in protected memory or are otherwise rendered read-only), and any part of the program may depend on it. A global variable therefore has an unlimited potential for creating mutual dependencies, and adding mutual dependencies increases complexity." If you want to read more, here: "Global Variables are Bad" c2.com/cgi/wiki?GlobalVariablesAreBad \$\endgroup\$
    – Mike Nakis
    Dec 19, 2011 at 15:56
  • \$\begingroup\$ I do not know why you had to use them, and what you are trying to achieve with them, so I cannot help you get rid of them. \$\endgroup\$
    – Mike Nakis
    Dec 19, 2011 at 15:57
  • \$\begingroup\$ Actually i believe i could replace them with private properties, Thanks anyways \$\endgroup\$
    – MegaMind
    Dec 19, 2011 at 16:10
  • \$\begingroup\$ Thanks for the accept. In order to replace them with private properties you will first need to make your methods non-static, instead of static which they are now. And that will probably open up another can of worms. So if you can just get away by leaving them as they are, just swipe them under the carpet and move on. \$\endgroup\$
    – Mike Nakis
    Dec 19, 2011 at 16:27
1
\$\begingroup\$

Mike Nakis has covered most of the design issues, so I'm going to cover some of the smaller points.

Var

Use the var keyword when defining method-scope variables for which the right hand side of the definition makes the type obvious. This saves you time when you come to refactor, and is neater.

e.g.

BinaryDataGrid c = sender as BinaryDataGrid;

should be

var c = sender as BinaryDataGrid;

Naming

Don't use single-character variable names. These do nothing but make the code more difficult to comprehend. Extra characters are free, so be descriptive.

var dateGrid = sender as BinaryDataGrid;

Braces

Always prefer to use braces for your if statements, it's cleaner, spaces things out better, and it's a StyleCop violation not to. Additionally, prefer to surround any statement involving braces with empty lines, this improves readability. Additionally, try to give your statements some space. Group together related statements, but leave a gap between unrelated ones.

So

if (_shouldCallBack)
{
    if (c.Sunday.Length == 4)
        return;
    Days day = (Days)c.dataGrid.Items[0];
    byte[] newArray = new byte[c.Sunday.Length + 1];
    newArray[0] = Convert.ToByte(0);
    c.Sunday.CopyTo(newArray, 1);
    day.Value = BitConverter.ToInt32(newArray.Reverse().ToArray(), 0);
    _sun = day.Value;
}
else
{
    _shouldCallBack = true;
    c.Sunday = BitConverter.GetBytes(_sun).Reverse().ToArray().Skip(1).ToArray();
}

Looks better as

if (_shouldCallBack)
{
    if (c.Sunday.Length == 4)
    {
        return;
    }

    Days day = (Days)c.dataGrid.Items[0];

    byte[] newArray = new byte[c.Sunday.Length + 1];
    newArray[0] = Convert.ToByte(0);

    c.Sunday.CopyTo(newArray, 1);

    day.Value = BitConverter.ToInt32(newArray.Reverse().ToArray(), 0);

    _sun = day.Value;
}
else
{
    _shouldCallBack = true;

    c.Sunday = BitConverter.GetBytes(_sun).Reverse().ToArray().Skip(1).ToArray();
}

ToArray

You have a duplicate call to ToArray that is wasted execution time here:

c.Sunday = BitConverter.GetBytes(_sun).Reverse().ToArray().Skip(1).ToArray();

Instead simply call:

c.Sunday = BitConverter.GetBytes(_sun).Reverse().Skip(1).ToArray();

Design

This is quite a lot of logic for what looks to be code-behind. I would recommend looking into some more well-structured patterns such as MVVM to keep your code separate from your view.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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