Skip to main content
2 of 4
added 1011 characters in body
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463
<TextBox Width="100" Text="{Binding Last, Mode=OneWay}"/>

This strikes me as a confusing UX. If the content isn't meant to be editable, then you shouldn't use a TextBox control; users expect to be able to edit a text box - and yours is editable, but modifying the content won't do anything to the view model... which is awkward. Either leave it a two-way binding and work out the VM logic for it, or make it a Label.

I like that your view has zero code-behind. However I'd find another way to do this:

DataContext = new MainViewModel();

The view itself shouldn't be responsible for actually instantiating its view model; this tight coupling violates the code against abstractions guideline. Instead of directly instantiating the MainViewModel instance, you could leave that responsibility up to the calling code:

var view = new MainWindow { DataContext = new MainViewModel() };

Now, if you're leaving it up to WPF to create the MainWindow instance in the App class, it's not too bad. All I'm saying is, there are other ways - the xaml designer can work off a DesignInstance:

xmlns:vm="clr-namespace:TestHumanName.ViewModel"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
mc:Ignorable="d" 
d:DesignHeight="392" d:DesignWidth="392" 
d:DataContext="{d:DesignInstance vm:MainWindowViewModel}">

The d: xml namespace is ignored at runtime, but the xaml designer will use it - by doing that you no longer need to set the view model in the constructor.

<Button Content="Go"/>

It's pretty rare that a simple string suffices as the Content of a Button control. Even when all I need is a caption, I find it easier to maintain when the content is expressed in a more verbose manner:

<Button>
    <StackPanel Orientation="Horizontal">
        <TextBlock Text="Go" />
    </StackPanel>
</Button>

That way if you change your mind and decide to add an icon beside the "Go" caption, all you have to do is, well, add an icon.

Hard-coding strings is fine for prototyping, but makes it quite painful to localize afterward: it's better to have the strings in a resource file from the start, so if/when you localize all that's left to do is, well, localize the strings. Then you can use a value converter or a markup extension to support the easy-to-use .resx resource format. The problem with localization is that it's often an afterthought, something that's "I don't need this"... until you do. And if you haven't set yourself up for it from the start, you're in for a lot of pain. The solution is simple: don't hard-code string captions into your markup - put your strings in a resource file... where they belong anyway.


Your naming scheme for private fields is terrible.

private string __sFull;
readonly ObservableAsPropertyHelper<string> __oapTitle;

There is no need for two underscores. Ever. Don't get me wrong - I use an underscore prefix all the time, for every single one of my private fields. I like how it avoids clashes with locals and parameters, and thus avoids being forced to use an otherwise perfectly redundant this qualifier. But two underscores just doesn't look right; there's no reason for having two of them.

Speaking of the this qualifier, its usage is confusing here:

public string Full
{
    get { return __sFull; }
    set { this.RaiseAndSetIfChanged(ref __sFull, value); }
}

If RaiseAndSetIfChanged is a member of the base class (I'm not familiar at all with reactive stuff), then base would have been a more appropriate and useful qualifier to use; this is simply redundant and uninformative here, and since the member isn't in the same class, it's outright misleading.

But the double-underscores and misplaced this qualifiers aren't the worst offender. Hungarian Notation is. I didn't think there still existed programmers writing C# code in 2016 using Hungarian Notation. Seriously - drop that. There is no need to use type-hinting prefixes for any identifier in any .net code (see why).


IMO the Name class shouldn't be a nested type under the ViewModel. It's your Model, and it's a completely separate thing from the ViewModel, as hinted by the pattern's name, Model-View-Viewmodel.

As for your concern with calling ParseName 4 times, it looks like your updated code is what doing that with might look like - although again, I've never used that namespace before.

public MainViewModel()
{
    this.WhenAnyValue(x => x.Full).Where(x => x != null).Select(x => ParseName(x))
        .ToProperty(this, x => x.NameObject, out __oapName);
}

Seems legit. It might read a little easier like this though:

public MainViewModel()
{
    WhenAnyValue(name => name.Full).Where(name => name != null)
                                   .Select(name => ParseName(name))
                                   .ToProperty(this, vm => vm.Name, out _name);
}

(ignore the name parsing bit!)

Why? It's a really interesting bit of code!

You're populating these arrays every time you call the function, but you shouldn't have to:

// Might want to add more to this list
string[] prefixes = { "mr", "mrs", "ms", "dr", "miss", "sir", "madam", "mayor", "president" };
// Might want to add more to this list, or use code/regex for roman-numeral detection
string[] suffixes = { "jr", "sr", "i", "ii", "iii", "iv", "v", "vi", "vii", "viii", "ix", "x", "xi", "xii", "xiii", "xiv", "xv" };

Make them private static IReadOnlyList<string> fields, that will create them only once for the type instead of per call.

Then, I'd try to extract a private method for each member I'm trying to parse out of that string, to up the abstraction level a bit.

Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463