6
\$\begingroup\$

I recently started C# and I've got the feedback that I still repeat too much. How can I make it less repetitive?

namespace ExtraOefLabo
{
/// <summary>
/// Interaction logic for MainWindow.xaml
/// </summary>
public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
    }

    string click = "";
    private void btnOk_Click(object sender, RoutedEventArgs e)
    {
        //Input

        //Proccess

        //Output
        if (click == "9382")
        {
            MessageBox.Show("Cracked the code!");

        }
    }

    private void btnNr1_Click(object sender, RoutedEventArgs e)
    {
        click+= "1";
    }

    private void btnNr2_Click(object sender, RoutedEventArgs e)
    {
        click +=  "2";
    }

    private void btnNr3_Click(object sender, RoutedEventArgs e)
    {
        click +=  "3";
    }

    private void btnNr4_Click(object sender, RoutedEventArgs e)
    {
        click +="4";
    }

    private void btnNr5_Click(object sender, RoutedEventArgs e)
    {
        click += "5";
    }

    private void btnNr6_Click(object sender, RoutedEventArgs e)
    {
        click += "6";
    }

    private void btnNr7_Click(object sender, RoutedEventArgs e)
    {
        click += "7";
    }

    private void btnNr8_Click(object sender, RoutedEventArgs e)
    {
        click += "8";
    }

    private void btnNr9_Click(object sender, RoutedEventArgs e) 
    {
        click += "9";
    }

    private void BtnNr0_Click(object sender, RoutedEventArgs e)
    {
        click += "0";
    }

    private void btnEnd_Click(object sender, RoutedEventArgs e)
    {
        Application.Current.Shutdown();
    }
}
}

XAML

    <Window x:Class="ExtraOefLabo.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:ExtraOefLabo"
        mc:Ignorable="d"
        Title="MainWindow" Height="350" Width="525">
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition/>
        </Grid.RowDefinitions>
        <Button x:Name="btnNr1" Content="1" HorizontalAlignment="Left" Height="25" Margin="115,51,0,0" VerticalAlignment="Top" Width="81" Click="btnNr1_Click"/>
        <Button x:Name="btnNr2" Content="2" HorizontalAlignment="Left" Height="25" Margin="201,51,0,0" VerticalAlignment="Top" Width="81" Click="btnNr2_Click"/>
        <Button x:Name="btnNr3" Content="3" HorizontalAlignment="Left" Height="25" Margin="287,51,0,0" VerticalAlignment="Top" Width="81" Click="btnNr3_Click"/>
        <Button x:Name="btnNr4" Content="4" HorizontalAlignment="Left" Height="25" Margin="115,81,0,0" VerticalAlignment="Top" Width="81" Click="btnNr4_Click"/>
        <Button x:Name="btnNr5" Content="5" HorizontalAlignment="Left" Height="25" Margin="201,81,0,0" VerticalAlignment="Top" Width="81" Click="btnNr5_Click"/>
        <Button x:Name="btnNr6" Content="6" HorizontalAlignment="Left" Height="25" Margin="287,81,0,0" VerticalAlignment="Top" Width="81" Click="btnNr6_Click"/>
        <Button x:Name="btnNr7" Content="7" HorizontalAlignment="Left" Height="25" Margin="115,111,0,0" VerticalAlignment="Top" Width="81" Click="btnNr7_Click"/>
        <Button x:Name="btnNr8" Content="8" HorizontalAlignment="Left" Height="25" Margin="201,111,0,0" VerticalAlignment="Top" Width="81" Click="btnNr8_Click"/>
        <Button x:Name="btnNr9" Content="9" HorizontalAlignment="Left" Height="25" Margin="287,111,0,0" VerticalAlignment="Top" Width="81" Click="btnNr9_Click"/>
        <Button x:Name="BtnNr0" Content="0" HorizontalAlignment="Left" Height="25" Margin="201,141,0,0" VerticalAlignment="Top" Width="81" Click="BtnNr0_Click"/>
        <Button x:Name="btnEnd" Content="End" HorizontalAlignment="Left" Height="25" Margin="115,141,0,0" VerticalAlignment="Top" Width="81" Click="btnEnd_Click"/>
        <Button x:Name="btnOk" Content="OK" HorizontalAlignment="Left" Height="25" Margin="287,141,0,0" VerticalAlignment="Top" Width="81" Click="btnOk_Click"/>

    </Grid>
</Window>
\$\endgroup\$
0
8
\$\begingroup\$

XAML:

First I would recommend that you use the Grid.RowDefinitions and Grid.ColumnDefinitions more / better. Then all the mess with VerticalAlignment, Height etc. can be deleted. But maybe you want a specified Height then you still can do it:

<RowDefinition Height="50"/>

Then the XAML code looks much cleaner and e.g. like this (but it is a bit longer):

<Window x:Class="Safe.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="MainWindow" Height="350" Width="525">
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="*" />
            <RowDefinition Height="50" />
        </Grid.RowDefinitions>
        <Grid>
            <Grid.RowDefinitions>
                <RowDefinition Height="*"/>
                <RowDefinition Height="*"/>
                <RowDefinition Height="*"/>
                <RowDefinition Height="*"/>
            </Grid.RowDefinitions>
            <Grid.ColumnDefinitions>
                <ColumnDefinition Width="*"/>
                <ColumnDefinition Width="*"/>
                <ColumnDefinition Width="*"/>
            </Grid.ColumnDefinitions>
            <Button Grid.Row="0" Grid.Column="0" Content="1"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="0" Grid.Column="1" Content="2"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="0" Grid.Column="2" Content="3"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="1" Grid.Column="0" Content="4"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="1" Grid.Column="1" Content="5"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="1" Grid.Column="2" Content="6"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="2" Grid.Column="0" Content="7"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="2" Grid.Column="1" Content="8"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="2" Grid.Column="2" Content="9"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="3" Grid.Column="1" Content="0"  Click="ButtonNumberClicked"/>
            <Button Grid.Row="3" Grid.Column="2" Content="End" Click="ButtonEndClicked"/>
        </Grid>
        <Button Grid.Row="1" x:Name="btnOk" Content="OK" Click="ButtonOkClicked"/>
    </Grid>
</Window>

In the image below you can see that the whole grid fills the GUI and will resize if you drag the window smaller / bigger.

Application GUI

Code behind (.cs):

As you can see in the code above i have deleted all the event handler, because they are unnecessary and can be implemented better. This was not a recursive programming fault. It was only a little bit too much code.

Recursion is e.g. calling a function Foo within the function Foo. The Recursion Wikipedia article may be a good first look into the chapter.

My C# code looks like this:

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
    }

    string click = "";

    private void ButtonOkClicked(object sender, RoutedEventArgs e)
    {
        if (this.click == "9382")
        {
            MessageBox.Show("Cracked the code!");

        }
    }

    private void ButtonEndClicked(object sender, RoutedEventArgs e)
    {
        Application.Current.Shutdown();
    }

    private void ButtonNumberClicked(object sender, RoutedEventArgs e)
    {
        var button = (Button)sender;
        this.click += (string)button.Content;
    }
}

I only have one Eventhandler for all the Buttons and I am reading the number from the Button which was clicked. The number is saved in its Content Property. You can read the clicked button with the sender parameter of the Eventhandler Method ButtonNumberClicked, but you have cast the sender to a Button because otherwise you are not able to access the Content property.

Hint: Casting objects

In your special example it is very clear that the sender from the ButtonNumberClicked must be a Button so you can cast it safe. But if it could be also a TextBox then you should cast it with the as operator and check for null:

private void ButtonNumberClicked(object sender, RoutedEventArgs e)
{
    var button = sender as Button;
    if (button != null)
    {
        this.click += (string)button.Content;                
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ It looks very good! I have one question though. Wich button is the ButtonNumberClicked and why is it reocurring in the xaml? \$\endgroup\$
    – Maartenw
    Nov 16 '15 at 13:00
  • 1
    \$\begingroup\$ @MaartenWachters ButtonNumberClicked is no Button. It is the Event Handler. A Button implements different Events like the Click event. So when you click the Button the event is fired and the EventHandler Method - which is ButtonNumberClicked - calls the code you want to run when the Click event is fired \$\endgroup\$
    – Jens
    Nov 16 '15 at 13:05
  • 1
    \$\begingroup\$ So the code is like this: You define 10 Button with content 0-9. Then you say when you click this button the code should run the ButtonNumberClicked method and in this method you extract the clicked button, which is button 1, 2, 3 ... 10 and get the number this button has - which is saved in the Content property. \$\endgroup\$
    – Jens
    Nov 16 '15 at 13:07
  • \$\begingroup\$ Thank you for your very good explenation. I have another question though. Right now we use the event click and it checks if click is 9382. However, if I'd like to check after each button press and not on OK, do I add my if statement to ButtonNumberClicked? \$\endgroup\$
    – Maartenw
    Nov 16 '15 at 13:11
  • 1
    \$\begingroup\$ @MaartenWachters yes. then you add the if to the ButtonNumberClicked event handler. \$\endgroup\$
    – Jens
    Nov 16 '15 at 13:12
4
\$\begingroup\$

Building on Jens Horstmann's answer, I think we can get more compact using an ItemsControl. One of the nice things about ItemsControls is that you can back them with any panel. In this case we want a UniformGrid.

<Window x:Class="WpfApplication2.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="MainWindow" Height="350" Width="525"
        x:Name="WindowRoot">
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="*" />
            <RowDefinition Height="50" />
        </Grid.RowDefinitions>
        <ItemsControl ItemsSource="{Binding Buttons, ElementName=WindowRoot}">
            <ItemsControl.ItemsPanel>
                <ItemsPanelTemplate>
                    <UniformGrid Columns="3"/>
                </ItemsPanelTemplate>
            </ItemsControl.ItemsPanel>
            <ItemsControl.ItemTemplate>
                <DataTemplate>
                    <Button x:Name="Button" Content="{Binding}" Click="ButtonNumberClicked"/>
                    <DataTemplate.Triggers>
                        <DataTrigger Binding="{Binding}" Value="{x:Null}">
                            <Setter TargetName="Button" Property="Visibility" Value="Collapsed"/>
                        </DataTrigger>
                    </DataTemplate.Triggers>
                </DataTemplate>
            </ItemsControl.ItemTemplate>
        </ItemsControl>
        <Button Grid.Row="1" x:Name="btnOk" Content="OK" Click="ButtonOkClicked"/>
    </Grid>
</Window>

(Note the datatrigger to hide the button in the bottom left.)

And the code behind:

public partial class MainWindow : Window
{
    public static readonly DependencyProperty ButtonsProperty =
        DependencyProperty.Register("Buttons", typeof(IEnumerable<string>), typeof(MainWindow), new PropertyMetadata(null));

    public IEnumerable<string> Buttons
    {
        get { return (IEnumerable<string>)GetValue(ButtonsProperty); }
        set { SetValue(ButtonsProperty, value); }
    }

    public MainWindow()
    {
        InitializeComponent();

        Buttons = new[] {"1", "2", "3", "4", "5", "6", "7", "8", "9", null, "0", "End"};
    }

    private void ButtonNumberClicked(object sender, RoutedEventArgs e)
    {
        // See Jens Horstmann's answer (still need to account for "End"
    }

    private void ButtonOkClicked(object sender, RoutedEventArgs e)
    {
        // See Jens Horstmann's answer
    }
}

This allows us to let WPF handle the layout a bit more. All we need to do is add buttons. This is extensible to using letters (or any other symbol) if you wanted without having to change the XAML (except to alter the number of columns).

\$\endgroup\$
2
\$\begingroup\$

Regarding the XAML:

  • A UniformGrid is a decent fit for this.
  • Using routed events some more duplication can be removed.

Sample:

<Grid>
    <Grid.RowDefinitions>
        <RowDefinition />
        <RowDefinition Height="Auto" />
    </Grid.RowDefinitions>
    <UniformGrid Columns="3" Button.Click="OnButtonClicked">
        <Button Content="1"/>
        <Button Content="2"/>
        <Button Content="3"/>
        <Button Content="4"/>
        <Button Content="5"/>
        <Button Content="6"/>
        <Button Content="7"/>
        <Button Content="8"/>
        <Button Content="9"/>
        <FrameworkElement>
            <!-- We use a this empty element as a placeholder to offset the 0-button.
                 This is a slight hack, hence this comment -->
        </FrameworkElement>
        <Button Content="0"/>
        <Button Content="End"/>
    </UniformGrid>
    <Button Grid.Row="1" Content="OK" Click="OnButtonOkClicked" Height="50"/>
</Grid>

Regarding the code:

If you are learning WPF you want to learn about:

  • Bindings and why they are nice. Create viewmodels and bind to them.
  • Layout using panels, here are the most useful ones (ordered): { Grid, StackPanel, Canvas, Dockpanel, WrapPanel }

I'm not saying MVVM because it tends to paralyzes beginners. When you use bindings fluently and love them good things will happen in your code.

Three reasons to not use code behind for logic like in your sample.

  • A view and viewmodel is usually a nice separation of concerns.
  • It is harder to test and reuse.
  • Code behind for logicis not idiomatic WPF, this is an important reason when learning.
\$\endgroup\$

Your Answer

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

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