8
\$\begingroup\$

I have a model called WorkOrder which I want to be able to manage from a single window. The required operations (for now) are only Insert and view, so this is what I came up with:
My Model:

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; }
}

My View:

<Window.DataContext>
    <vm:WorkOrderManagerViewModel/>
</Window.DataContext>
<StackPanel>
    <!--This textbox is incharge of searching the work order in the DB.
    If the value was found, a variable of type WorkOrder is set in the viewmodel,
    otherwise the boolean IsNewWorkOrder is set to true and the rest of the window gets enabled-->
    <TextBlock Text="Work Order Name"/>
    <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>

    <StackPanel IsEnabled="{Binding IsNewWorkOrder}">
        <TextBlock Text="Part Number"/>
        <TextBox Text="{Binding SelectedWorkOrder.PartNumber}"/>
        <TextBlock Text="Created"/>
        <TextBox Text="{Binding SelectedWorkOrder.Created}"/>
        <TextBlock Text="Closed"/>
        <TextBox Text="{Binding SelectedWorkOrder.Closed}"/>
        <TextBlock Text="First Number"/>
        <TextBox Text="{Binding SelectedWorkOrder.FirstNumber}"/>
        <TextBlock Text="Last Number"/>
        <TextBox Text="{Binding SelectedWorkOrder.LastNumber}"/>

        <Button Content="Save"/>
    </StackPanel>
</StackPanel>

The ViewModel:

public class WorkOrderManagerViewModel : ObservableObject
{
    private ApplicationDbContext db = new ApplicationDbContext();

    //bound to the work order name textbox
    private string _workOrderName;
    public string WorkOrderName
    {
        get { return _workOrderName; }
        set
        {
            _workOrderName = value;

            //on text changed - clear all values
            SelectedWorkOrder = null;
        }
    }

    private WorkOrder _selectedWorkOrder;
    public WorkOrder SelectedWorkOrder
    {
        get { return _selectedWorkOrder; }
        set
        {
            _selectedWorkOrder = value;
            RaisePropertyChanged();
        }
    }

    //set to true if in "new work order mode"
    private bool _isNewWorkOrder;
    public bool IsNewWorkOrder
    {
        get { return _isNewWorkOrder; }
        set
        {
            _isNewWorkOrder = value;
            RaisePropertyChanged();
        }
    }

    //after enter was pressed - used to search if exists or not
    #region GetWorkOrderCommand
    private ICommand _getWorkOrderCommand;
    public ICommand GetWorkOrderCommand
    {
        get
        {
            if (_getWorkOrderCommand == null)
            {
                _getWorkOrderCommand = new CommandBase(GetWorkOrder, CanGetWorkOrder);
            }
            return _getWorkOrderCommand;
        }
    }

    private bool CanGetWorkOrder(object arg)
    {
        string name = (arg ?? "").ToString();
        return !string.IsNullOrWhiteSpace(name);
    }

    private void GetWorkOrder(object parameter)
    {
        IsNewWorkOrder = false;
        string name = WorkOrderName;

        var wo = db.WorkOrders.Where(x => x.WorkOrderName.ToUpper() == name.ToUpper()).FirstOrDefault();
        if (wo != null)
        {
            SelectedWorkOrder = wo;
        }
        else
        {
            SelectedWorkOrder = new WorkOrder()
            {
                WorkOrderName = parameter.ToString(),
                Created = DateTime.Now,
                FirstNumber = 0,
                LastNumber = 0
            };
            IsNewWorkOrder = true;
        }
    }
    #endregion


    #region SaveChanges Command
    private ICommand _saveChangesCommand;
    public ICommand SaveChangesCommand
    {
        get
        {
            if (_saveChangesCommand == null)
            {
                _saveChangesCommand = new CommandBase(SaveChanges, CanSaveChanges);
            }
            return _saveChangesCommand;
        }
    }

    private bool CanSaveChanges(object arg)
    {
        if (SelectedWorkOrder == null)
            return false;

        return true;
    }

    private void SaveChanges(object obj)
    {
        if (SelectedWorkOrder.FirstNumber >= SelectedWorkOrder.LastNumber)
        {
            MessageBox.Show("Numeric Error");
            return;
        }
        if (IsNewWorkOrder)
        {
            #region check for overlaps

            var lowerOverlap = db.WorkOrders.Where(x =>
                x.FirstNumber <= SelectedWorkOrder.FirstNumber
             && x.LastNumber >= SelectedWorkOrder.FirstNumber).FirstOrDefault();

            if (lowerOverlap != null)
            {
                MessageBox.Show("Overlap Error");
                return;
            }

            var upperOverlap = db.WorkOrders.Where(x =>
                 x.FirstNumber <= SelectedWorkOrder.LastNumber
              && x.LastNumber >= SelectedWorkOrder.LastNumber).FirstOrDefault();

            if (upperOverlap != null)
            {
                MessageBox.Show("Overlap Error");
                return;
            }
            #endregion

            SelectedWorkOrder.Created = DateTime.Now;

            db.WorkOrders.Add(SelectedWorkOrder);
            int insertResult = db.SaveChanges();

            if (insertResult > 0)
                MessageBox.Show("New Work Order Created");
        }
    }
    #endregion
}

Is this Implementation correct? Are there any guidelines or design patterns for handling this kind of Insert\Select\Update functionality? The part that I really don't like in this code is the IsNewWorkOrder variable, any way to implement it differently?

\$\endgroup\$
10
\$\begingroup\$

View

The XAML is rather... simplistic:

designer rendition of XAML code

Each label and textbox (and the Save 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, ScrollViewer, margins, padding and styles make a prettier UI

WrapPanel

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.

<Window.Resources>
    <Style x:Key="InputField" TargetType="{x:Type TextBox}">
        <Setter Property="MinWidth" Value="172" />
        <Setter Property="Foreground" Value="Blue" />
    </Style>
    <Style x:Key="Heading" TargetType="{x:Type TextBlock}">
        <Setter Property="FontWeight" Value="Bold" />
    </Style>
</Window.Resources>
<Grid Margin="4">

    <Grid.RowDefinitions>
        <RowDefinition Height="Auto" />
        <RowDefinition Height="*" />
        <RowDefinition Height="32" />
    </Grid.RowDefinitions>

    <!-- search box -->

    <GroupBox Grid.Row="0" Padding="4">
        <GroupBox.Header>
            <TextBlock Style="{StaticResource Heading}" 
                       Text="Work Order Name" />
        </GroupBox.Header>

        <StackPanel Orientation="Horizontal">

            <TextBox Text="{Binding WorkOrderName}"
                     HorizontalAlignment="Left"
                     Width="172" Margin="2"
                     AcceptsReturn="False">
                <TextBox.InputBindings>
                    <KeyBinding Key="Enter" Command="{Binding GetWorkOrderCommand}" />
                </TextBox.InputBindings>
            </TextBox>

            <Button Command="{Binding GetWorkOrderCommand}" 
                    Margin="2" 
                    Padding="2">
                <Button.Content>
                    <StackPanel Orientation="Horizontal">
                        <Image Source="Images/find.png" />
                      <!--  <TextBlock Text="Search" /> -->
                    </StackPanel>
                </Button.Content>
            </Button>

        </StackPanel>

    </GroupBox>

    <!-- form -->

    <GroupBox Grid.Row="1" Padding="2" Margin="2"
              IsEnabled="{Binding IsNewWorkOrder}">
        <GroupBox.Header>
            <TextBlock Style="{StaticResource Heading}" 
                       Text="Work Order Info" />
        </GroupBox.Header>

        <ScrollViewer VerticalScrollBarVisibility="Auto">

            <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>
        </ScrollViewer>

    </GroupBox>

    <!-- bottom panel-->

    <Border Grid.Row="2" Background="WhiteSmoke" />

    <WrapPanel Grid.Row="2" 
               HorizontalAlignment="Right">

        <Button Margin="4" Padding="2"
            Width="128"
            HorizontalAlignment="Right"
            Content="Save"/>

        <Button Margin="4" Padding="2"
            HorizontalAlignment="Right"
            Content="Close"/>

    </WrapPanel>

</Grid>

I noticed the Save button doesn't have a command binding. You may want to fix that ;)

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:

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:

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:

//bound to the work order name textbox

This one is also not clear:

//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:

 // text has changed, invalidate current selection.

This one is also something that the ViewModel has no control over:

//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 Search 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:

    if (SelectedWorkOrder == null)
        return false;

    return true;

Is really nothing more than return SelectedWorkOrder != null;.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for taking the time to go through the entire code (and the xaml) and explaining everything. I did have some styles set in the App.Xaml but your approach with the WrapPanel is preferred. I would like to ask you to elaborate some more about the WorkOrderViewModel part. \$\endgroup\$ – Yoav Sep 8 '14 at 5:41
  • \$\begingroup\$ It's actually best to put resources (/styles) in a ResourceDictionary, so they can be reused in other views. As for the viewmodel, I might edit later & add some more. \$\endgroup\$ – Mathieu Guindon Sep 8 '14 at 11:36
  • \$\begingroup\$ asked for help about the wrapping of the Model in SO: stackoverflow.com/q/25747962/1092181 \$\endgroup\$ – Yoav Sep 9 '14 at 15:56
  • \$\begingroup\$ Been trying to find an explanation for the ViewModel part without any success... any additional info or details would be great \$\endgroup\$ – Yoav Sep 14 '14 at 21:10
  • 1
    \$\begingroup\$ @Yoav I find it's pretty much it; the idea is that the ViewModel contains the command logic; you could move the database operations into some "service" class, and inject/instantiate that instead of a DbContext, but I see nothing blatantly wrong with your pattern/design. \$\endgroup\$ – Mathieu Guindon Sep 15 '14 at 21:39
6
\$\begingroup\$

I think that the IsNewWorkOrder property is fine, but there are a couple of other things you should watch out.

Why do you have the logic to interact with the database in your ViewModel? I think it will be much cleaner if you extract that to another class and then you inject it as a dependency.

You would need something like:

interface IDatabase
{
    bool TryDatabaseLookup(out WorkOrder workOrder);
    void SaveChanges(WorkOrder workOrder);
}

Another thing I noticed is that you invoke MessageBox.Show within the ViewModel. That is conceptually wrong. The ViewModel should only provide data and command bindings but should not directly create UI elements. Their creation should be delegated to the View. You need to notify it (through a method invocation, raising an event or through some other messaging middleware) that you want to show that message and leave it create and display the message box.

\$\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.