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've spent a few days making the transition from WinForms to WPF, and do not have much time for tutorials as the work needs to be done quickly. I was wondering if anyone could take a look at a sample of my code and point out any conventions that I'm missing (and that are commonplace in modern WPF usage). I've picked a random sample of some XAML code which I've written:

<Window x:Class=""
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
         Width="300" Height="300">
    <Window.Resources>
        <Style x:Key="buttonsStyle" TargetType="Button">
            <Setter Property="Height" Value="25"/>
            <Setter Property="Width" Value="80"/>
        </Style>
        <Style x:Key="componentsStyle" TargetType="Control">
            <Setter Property="Height" Value="20"/>
            <Setter Property="Width" Value="190"/>
        </Style>
        <Style x:Key="labelStyle" TargetType="Label">
            <Setter Property="VerticalAlignment" Value="Center"/>
        </Style>
    </Window.Resources>
    <StackPanel Orientation="Vertical">
        <Grid>
            <Grid.RowDefinitions>
                <RowDefinition>
                </RowDefinition>
                <RowDefinition>
                </RowDefinition>
                <RowDefinition>
                </RowDefinition>
                <RowDefinition>
                </RowDefinition>
                <RowDefinition>
                </RowDefinition>
            </Grid.RowDefinitions>
            <Grid.ColumnDefinitions>
                <ColumnDefinition Width="70">
                </ColumnDefinition>
                <ColumnDefinition Width="200">
                </ColumnDefinition>
            </Grid.ColumnDefinitions>
            <Label Grid.Column="0" Grid.Row="0" Style="{StaticResource labelStyle}">Server:</Label>
            <ComboBox Grid.Column="1" Grid.Row="0" Name="cmbServer" Style="{StaticResource componentsStyle}"/>
            <CheckBox Name="chkWindowsAuthentication" Grid.Column="0" Grid.Row="1" IsChecked="False"/>
            <Label Grid.Column="1" Grid.Row="1" Style="{StaticResource labelStyle}">Use Windows Authentication</Label>
            <Label Grid.Column="0" Grid.Row="2" Style="{StaticResource labelStyle}">Username:</Label>
            <TextBox Grid.Column="1" Grid.Row="2" Name="txtUsername" Style="{StaticResource componentsStyle}"/>
            <Label Grid.Column="0" Grid.Row="3" Style="{StaticResource labelStyle}">Password:</Label>
            <PasswordBox Grid.Column="1" Grid.Row="3" Name="txtPassword" Style="{StaticResource componentsStyle}"/>
            <Label Grid.Column="0" Grid.Row="4" Style="{StaticResource labelStyle}">Database:</Label>
            <ComboBox Grid.Column="1" Grid.Row="4" Name="cmbDatabase" Style="{StaticResource componentsStyle}"/>
        </Grid>
        <StackPanel Orientation="Horizontal" Margin="10,30,10,10">
            <Button Margin="0,0,10,0" Style="{StaticResource buttonsStyle}">Connect</Button>
            <Button Style="{StaticResource buttonsStyle}">Reset</Button>
        </StackPanel>
    </StackPanel>
</Window>

The code is functional and displays the proper result, however I am worried that I am not using the right techniques. Any input at all is greatly appreciated.

share|improve this question
add comment

1 Answer 1

up vote 14 down vote accepted

All in all, your code is quite well-structured. Here are a couple of suggestions for improvement:

  1. Consider moving your styles to a resource dictionary:

    <Window.Resources>
        <ResourceDictionary Source="Resources.xaml" />
    </Window.Resources>
    

    This will de-clutter your file and allow you to re-use the styles in other windows.

  2. Collapse empty tags (use the self-closing syntax):

    <Grid.RowDefinitions>
        <RowDefinition />
        <RowDefinition />
        <RowDefinition />
        <RowDefinition />
        <RowDefinition />
    </Grid.RowDefinitions>
    

    (the same goes for the ColumnDefinitions and any other empty tags)

  3. Declare styles that affect all controls of one type without a key:

    <Style TargetType="Button"> ... </Style>
    <Style TargetType="Label"> ... </Style>
    

    That way, you don't seperately have to set all the labels to labelStyle and all the buttons to buttonsStyle, just remove all occurences of the following two attributes:

    Style="{StaticResource labelStyle}"
    Style="{StaticResource buttonsStyle}"
    
  4. The default value of Grid.Column and Grid.Row is 0, you can get away without setting it explicitly. And CheckBox.Checked is false by default. So you can remove all occurences of the following:

    Grid.Column="0" 
    Grid.Row="0" 
    IsChecked="False"
    
  5. Generally, in WPF, widths and heights are not explicitly stated. You should try to use relative and automatic dimensions where possible (which often requires choosing the correct container). For instance, you could use relative sizing for your grid columns:

    <Grid.ColumnDefinitions>
        <ColumnDefinition Width="0.35*" />
        <ColumnDefinition Width="*" />
    </Grid.ColumnDefinitions>
    

    In other situations, working with Margin can be a replacement for explicit pixel sizes. Remember that hard-coded sizes are an obstacle to localization (where long words in foreign languages are cropped) and less flexible.

These suggestions shrink your Window .xaml file from 54 to 37 lines (Resources.xaml has 14).

share|improve this answer
    
Thank you very much for the detailed reply. So if I basically had to replace widths and heights with margin values, that would solve the last problem? –  Dot NET Oct 11 '12 at 17:57
    
You don't have to, but yes, margins are normally used where possible instead of heights and widths. –  codesparkle Oct 11 '12 at 17:59
    
Ok thanks, I'll see what I can do :) –  Dot NET Oct 11 '12 at 18:05
    
If I may also ask, what would be the convention for setting the window's height and width? (since I should try to avoid explicitly specifying them) –  Dot NET Oct 11 '12 at 18:16
1  
Personally, I wouldn't remove all instances of Grid.Column="0", because I like to have the code regular. But it makes sense to omit it if you have a grid with only one column. –  svick Oct 11 '12 at 18:18
show 3 more comments

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.