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.

I have written some code for inserting a label at runtime that has its content set to a string array into a datagrid row. All of this will initiate when certain radio buttons are checked. The code is working perfectly fine, but I need to improve this code as I am learning C#, WPF and datagrid. I know there can be a certain way to improve this code. This code will be a nightmare when there are 50 radio buttons. Can it be improved?

XAML Code:

<Grid>
<RadioButton x:Name="rb_1" Content="RadioButton" HorizontalAlignment="Left" Margin="351,85,0,0" VerticalAlignment="Top" GroupName="1" />
<RadioButton x:Name="rb_2" Content="RadioButton" HorizontalAlignment="Left" Margin="351,105,0,0" VerticalAlignment="Top" GroupName="1"/>
<RadioButton x:Name="rb_3" Content="RadioButton" HorizontalAlignment="Left" Margin="351,120,0,0" VerticalAlignment="Top" GroupName="1"  />
<RadioButton Content="RadioButton" HorizontalAlignment="Left" Margin="351,159,0,0" VerticalAlignment="Top" GroupName="2" />
<RadioButton x:Name="rb_4" Content="RadioButton" HorizontalAlignment="Left" Margin="351,179,0,0" VerticalAlignment="Top" GroupName="2"/>
<RadioButton Content="RadioButton" HorizontalAlignment="Left" Margin="351,199,0,0" VerticalAlignment="Top" GroupName="2" />
<Button Content="Button" HorizontalAlignment="Left" Margin="713,60,0,0" VerticalAlignment="Top" Width="75" Click="Button_Click_2"/>
<DataGrid x:Name="datagrid_" HorizontalAlignment="Left" Margin="549,85,0,0" VerticalAlignment="Top" Height="253" Width="399" />
<RadioButton Content="RadioButton" HorizontalAlignment="Left" Margin="351,226,0,0" VerticalAlignment="Top" GroupName="3" />
<RadioButton x:Name="rb_6" Content="RadioButton" HorizontalAlignment="Left" Margin="351,246,0,0" VerticalAlignment="Top" GroupName="3"/>
<RadioButton Content="RadioButton" HorizontalAlignment="Left" Margin="351,266,0,0" VerticalAlignment="Top" GroupName="3" />
<RadioButton Content="RadioButton" HorizontalAlignment="Left" Margin="351,298,0,0" VerticalAlignment="Top" GroupName="4" />
<RadioButton x:Name="rb_8" Content="RadioButton" HorizontalAlignment="Left" Margin="351,318,0,0" VerticalAlignment="Top" GroupName="4"/>
<RadioButton Content="RadioButton" HorizontalAlignment="Left" Margin="351,338,0,0" VerticalAlignment="Top" GroupName="4" />

Code Behind:

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
    }
    DataTable dt;
    DataRow dr;
    string[] str = new string[4];
    int location = 0;
    int count = 0;

    private void Window_Loaded(object sender, RoutedEventArgs e)
    {
        dt = new DataTable("emp");
        DataColumn dc1 = new DataColumn("Factors", typeof(string));
        DataColumn dc2 = new DataColumn("Non_Compliant", typeof(string));

        dt.Columns.Add(dc1);
        dt.Columns.Add(dc2);

        datagrid_.ItemsSource = dt.DefaultView;
    }
    private void Button_Click_2(object sender, RoutedEventArgs e)
    {
        if (count >= 1)
        {
            datagrid_.ItemsSource = dt.DefaultView;
            dt.Clear();
        }

        str[0] = "Load Path1";
        str[1] = "Load Path2";
        str[2] = "Load Path3";
        str[3] = "Load Path4";
        int j = 0;

        if (rb_2.IsChecked == true)
        {
            j = 0;
            int k = 0;
            dr = dt.NewRow();
            Label label = new Label();
            label.Height = 28;
            label.Width = 100;
            label.HorizontalAlignment = HorizontalAlignment.Center;
            label.VerticalAlignment = VerticalAlignment.Center;
            label.Content = str[j];
            dr[k] = label.Content;
            dt.Rows.Add(dr);
            datagrid_.ItemsSource = dt.DefaultView;
            location += 34;

        }
        if (rb_4.IsChecked == true)
        {
            j = 1;
            int k = 0;
            dr = dt.NewRow();
            Label label = new Label();
            label.Height = 28;
            label.Width = 100;
            label.HorizontalAlignment = HorizontalAlignment.Center;
            label.VerticalAlignment = VerticalAlignment.Center;
            label.Content = str[j];
            dr[k] = label.Content;
            dt.Rows.Add(dr);
            datagrid_.ItemsSource = dt.DefaultView;
            location += 34;

        }
        if (rb_6.IsChecked == true)
        {
            j = 2;
            int k = 0;
            dr = dt.NewRow();
            Label label = new Label();
            label.Height = 28;
            label.Width = 100;
            label.HorizontalAlignment = HorizontalAlignment.Center;
            label.VerticalAlignment = VerticalAlignment.Center;
            label.Content = str[j];
            dr[k] = label.Content;
            dt.Rows.Add(dr);
            datagrid_.ItemsSource = dt.DefaultView;
            location += 34;

        }
        if (rb_8.IsChecked == true)
        {
            j = 3;
            int k = 0;
            dr = dt.NewRow();
            Label label = new Label();
            label.Height = 28;
            label.Width = 100;
            label.HorizontalAlignment = HorizontalAlignment.Center;
            label.VerticalAlignment = VerticalAlignment.Center;
            label.Content = str[j];
            dr[k] = label.Content;
            dt.Rows.Add(dr);
            datagrid_.ItemsSource = dt.DefaultView;
            location += 34;
        }
        count++;

    }


}
share|improve this question
    
Welcome to CodeReview, Vanadium90. Hope you get some fine answers. –  Legato Apr 12 at 15:51
    
looking forward :) –  Vanadium90 Apr 12 at 15:54

1 Answer 1

up vote 3 down vote accepted

Don't do this:

DataTable dt;
DataRow dr;
string[] str = new string[4];
int location = 0;
int count = 0;

Always use explicit access modifiers.

Also, do any of those fields really need to be global?


Now, the real problem is that you don't follow MVVM. A lot of my other issues follow from that.

For instance:

<Button Content="Button" HorizontalAlignment="Left" Margin="713,60,0,0" VerticalAlignment="Top" Width="75" Click="Button_Click_2"/>

You shouldn't use the Click of a button; instead you should bind it to a command.

You also use the name of your DataGrid in your code behind, datagrid_. That is a meaningless name that doesn't follow any naming guideline. Quite frankly, I don't think I've used the Name of a XAML object more than a handful of times in the years I did Silverlight development: whenever possible you should bind to a source.

I also see a lot of Margin properties being used in very specific ways. I would urge you to look into the various layout possibilities of XAML and apply those. Don't work pixel-perfect, it's pointless IMHO and just a maintenance nightmare.

You XAML code looks like it's drag & drop. Which is easy to use I guess, but I'd urge you to abandon the visual editor and code your XAML "by hand".


A quick solution for now: look at the code that you repeat over and over, i.e. most of this:

        int k = 0;
        dr = dt.NewRow();
        Label label = new Label();
        label.Height = 28;
        label.Width = 100;
        label.HorizontalAlignment = HorizontalAlignment.Center;
        label.VerticalAlignment = VerticalAlignment.Center;
        label.Content = str[j];
        dr[k] = label.Content;
        dt.Rows.Add(dr);
        datagrid_.ItemsSource = dt.DefaultView;
        location += 34;

Once you start to copy-paste code and simply change one or two things, you know that's a sign you need to move it to a method that will accept the necessary parameters.

share|improve this answer
    
you know what sir. . I love u :) thanks and thanks alot for the helpful guideline and review. I will improve it in sha ALLAH :) any good book or tutorial on databinding that u recommend please?Eager to learn. –  Vanadium90 Apr 12 at 17:49

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.