> **View**
<!-- language-all: lang-xaml -->
The XAML is rather... simplistic:
![designer rendition of XAML code][1]
Each label and textbox (and the <kbd>Save</kbd> button) stretches all the way across the window.
Instead of just *dumping* controls into a `StackPanel`, you could consider leveraging automagic layout with a `WrapPanel`, and making each label+textbox duet logically regrouped.
I like that you have no WinForms-like absolute positioning, but I would have expected to see margins, or styles. I'd recommend further nesting in your XAML - use a `Grid` to define sections, regroup label+textbox fields in their own `StackPanel`, and use styles to make all fields feel the same, without having to set `MinWidth` and fonts and other things every time.
![Grid, GroupBox, WrapPanel, margins, padding and styles make a prettier UI][2]
Having your controls *logically* regrouped also *visually* regroups them, and your user has a better experience. Here I've removed the "Work Order Name" textblock, and replaced it with a `GroupBox`. Here's the XAML markup for the above screenshot, I'd like to highlight this part:
> <WrapPanel DataContext="{Binding SelectedWorkOrder}">
Setting the container's data context allows everything under it to bind to the `SelectedWorkOrder` properties without having to qualify them, which makes clearer markup.
<Grid Margin="4">
<Grid.RowDefinitions>
<RowDefinition Height="Auto" />
<RowDefinition Height="*" />
<RowDefinition Height="32" />
</Grid.RowDefinitions>
<GroupBox Grid.Row="0"
Padding="4"
Header="Work Order Name">
<TextBox Text="{Binding WorkOrderName}"
AcceptsReturn="False">
<TextBox.InputBindings>
<KeyBinding Key="Enter"
Command="{Binding GetWorkOrderCommand}" />
</TextBox.InputBindings>
</TextBox>
</GroupBox>
<GroupBox Grid.Row="1"
Padding="4"
Header="Work Order Info"
IsEnabled="{Binding IsNewWorkOrder}">
<WrapPanel DataContext="{Binding SelectedWorkOrder}">
<StackPanel Margin="2">
<TextBlock Text="Part Number"/>
<TextBox Style="{StaticResource InputField}"
Text="{Binding PartNumber}"/>
</StackPanel>
<StackPanel Margin="2">
<TextBlock Text="Created"/>
<TextBox Style="{StaticResource InputField}"
Text="{Binding Created}"/>
</StackPanel>
<StackPanel Margin="2">
<TextBlock Text="Closed"/>
<TextBox Style="{StaticResource InputField}"
Text="{Binding Closed}"/>
</StackPanel>
<StackPanel Margin="2">
<TextBlock Text="First Number"/>
<TextBox Style="{StaticResource InputField}"
Text="{Binding FirstNumber}"/>
</StackPanel>
<StackPanel Margin="2">
<TextBlock Text="Last Number"/>
<TextBox Style="{StaticResource InputField}"
Text="{Binding LastNumber}"/>
</StackPanel>
</WrapPanel>
</GroupBox>
<Button Grid.Row="2" Content="Save"/>
</Grid>
Comments in XAML shouldn't try to explain anything:
> <!--This textbox is incharge of searching the work order in the DB. [...]
I hope not. A textbox shouldn't be *in charge of* anything but collecting user input and displaying a value. This comment is misleading at best, and tries to explain the mechanics of the *ViewModel*, which has nothing to do with the textbox itself - help yourself, remove it.
---
I'm not sure why you're passing your command a parameter here:
> <TextBox Text="{Binding WorkOrderName}" AcceptsReturn="False">
> <TextBox.InputBindings>
> <KeyBinding Key="Enter" Command="{Binding GetWorkOrderCommand}"
> CommandParameter="{Binding Path=Text,
> RelativeSource={RelativeSource AncestorType=TextBox}}"/>
> </TextBox.InputBindings>
> </TextBox>
The *ViewModel* already knows what the `WorkOrderName` is, so the command binding can be simplified:
> <TextBox Text="{Binding WorkOrderName}" AcceptsReturn="False">
> <TextBox.InputBindings>
> <KeyBinding Key="Enter" Command="{Binding GetWorkOrderCommand}" />
> </TextBox.InputBindings>
> </TextBox>
---
> **Model**
Not much to say about the `WorkOrder` model, except that I find auto-properties don't really need the whitespace in-between:
<!-- language: lang-csharp -->
public class WorkOrder
{
public int WorkOrderId { get; set; }
public string WorkOrderName { get; set; }
public string PartNumber { get; set; }
public DateTime Created { get; set; }
public DateTime? Closed { get; set; }
public long FirstNumber { get; set; }
public long LastNumber { get; set; }
}
This class is an *entity type* - I don't like that the *View* is binding directly to it. I would create another class, a `WorkOrderViewModel`, that implements the `INotifyPropertyChanged` interface - this class only needs to expose the properties of the *Model* that the *View* is interested in, and can easily know whether it's a "new" work order:
<!-- language: lang-csharp -->
public class WorkOrderViewModel : INotifyPropertyChanged
{
private readonly WorkOrder _entity;
public WorkOrderViewModel()
: this(new WorkOrder())
{ }
public WorkOrderViewMode(WorkOrder entity)
{
_entity = entity;
}
public WorkOrder Model { get { return _entity; } }
public string WorkOrderName
{
get { return _entity.WorkOrderName; }
set
{
_entity.WorkOrderName = value;
NotifyPropertyChanged("WorkOrderName");
}
}
// ...
public bool IsNew { get { return _entity.WorkOrderId == 0; } }
}
---
> **ViewModel**
The `WorkOrderManagerViewModel` can then expose `SelectedWorkOrder` as a `WorkOrderViewModel`.
**Comments**
Again, the comments in the *ViewModel* should explain what's going on *in the ViewModel*, not in the *View* - this is a *waiting-to-become-a-blatant-lie* comment:
<!-- language: lang-csharp -->
> //bound to the work order name textbox
This one is also not clear:
<!-- language: lang-csharp -->
> //on text changed - clear all values
"clearing all values" is the result of setting the `SelectedWorkOrder` to `null`... but only because of things that happen outside the *ViewModel* (specifically, because of the bindings in the *View*) - the comment would be better if reworded like this:
<!-- language: lang-csharp -->
// text has changed, invalidate current selection.
This one is also something that the *ViewModel* has no control over:
<!-- language: lang-csharp -->
> //after enter was pressed - used to search if exists or not
If the command bindings change in the *View*, this commment becomes wrong, or incomplete - what if you added a <kbd>Search</kbd> button next to the text box, for convenience? What if more controls were added, with command bindings to this command?
Other than that, I find the name `GetWorkOrder` a little bit awkward for a method that returns `void`. I like naming my command methods `CanExecuteXxxxx` and `ExecuteXxxxx` - so that would be `CanExecuteGetWorkOrder` and `ExecuteGetWorkOrder` - these names also help justifying the `object` parameter that the `ICommand` interface wants.
---
The `CanSave` logic could be simplified:
<!-- language: lang-csharp -->
> if (SelectedWorkOrder == null)
> return false;
>
> return true;
Is really nothing more than `return SelectedWorkOrder != null;`.
[1]: https://i.sstatic.net/9cOsO.png
[2]: https://i.sstatic.net/2gpuC.png