7
\$\begingroup\$

I'm navigating to a view. I have an EventDateTime property which I want to set to DateTime.Today unless the navigation is coming from MyCustomView

This is what I've come with so far. It's working, but I find this really ugly:

public void OnNavigatedTo(NavigationParameters parameters)
{
    if (parameters != null)
    {
        if (!parameters.ContainsKey(PARAM_NAVIGATEDFROM)) // If there is parameter PARAM_NAVIGATEDFROM, then the navigation doesn't come from MyCustomView
        {
            EventDateTime = DateTime.Today;
        }
        else if (parameters[PARAM_NAVIGATEDFROM] != typeof(MyCustomViewModel)) // If there is a parameter PARAM_NAVIGATEDFROM but not with the specific value, the navigation doesn't come from MyCustomView
        {
            EventDateTime = DateTime.Today;
        }
    }
    else // If there are no parameters at all, the navigation is not coming from MyCustomView either
    {
        EventDateTime = DateTime.Today;
    }
}

Any ideas how I can improve this Code? Maybe even with another approach instead of putting the Code into the OnNavigatedTo() method?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ And what should happen if it is coming from MyCustomViewModel ? \$\endgroup\$
    – Heslacher
    Jan 5 '17 at 12:45
  • \$\begingroup\$ Some other stuff (which always happens no matter from which View I'm navigating from, so I didn't include it here for readability). \$\endgroup\$
    – Zure
    Jan 5 '17 at 12:47
  • 3
    \$\begingroup\$ But thats what you should do, otherwise one would suggest only to check if it is a MyCustomViewModel and if it is just call return; \$\endgroup\$
    – Heslacher
    Jan 5 '17 at 12:49
  • 1
    \$\begingroup\$ @Heslacher But that actually sounds like a good solution if I go this check at the end of the OnNavigatedTo() method after the "other stuff" has already been executed? \$\endgroup\$
    – Zure
    Jan 5 '17 at 13:04
6
\$\begingroup\$

I think Bethan has the best approach, but you could take it one step further and use a generic Type:

public void OnNavigatedTo(NavigationParameters parameters)
{
    if (!IsNavigationFrom<MyCustomView>(parameters))
    {
        EventDateTime = DateTime.Today;
    }
}

private bool IsNavigationFrom<T>(NavigationParameters parameters)
{
    return parameters != null
        && parameters.ContainsKey(PARAM_NAVIGATEDFROM)
        && parameters[PARAM_NAVIGATEDFROM] == typeof(T);
}

This makes the method applicable to multiple view models.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ yeah, using generics is a good idea \$\endgroup\$
    – t3chb0t
    Jan 5 '17 at 13:41
5
\$\begingroup\$

I think you can make it shorter, more expressive and easier to understand with C# 6, a helper variable and postive conditions. By using a helper variable with a nice descriptive name you'll always know what this condition means without having to write any comments. It will be self-explaining.

This means you check whether the parameter is comming from the custom-view and then if it is not comming from there you do something.

I find it is easier to negate the entire condition that defines whether a parameter originates from somewhere rather then negating each partial condition to flip the logic.

var navigatedFromMyCustomView =
    parameters?.ContainsKey(PARAM_NAVIGATEDFROM) && 
    parameters[PARAM_NAVIGATEDFROM] == typeof(MyCustomViewModel);

if (!navigatedFromMyCustomView)
{        
    EventDateTime = DateTime.Today;
}

or for C# < 6

var navigatedFromMyCustomView =
    parameters != null &&
    parameters.ContainsKey(PARAM_NAVIGATEDFROM) && 
    parameters[PARAM_NAVIGATEDFROM] == typeof(MyCustomViewModel);
\$\endgroup\$
2
  • \$\begingroup\$ Sounds like a good answer, but I'm using C# 5.0... I guess that's why I'm getting an error Operator '&&' cannot be applied to operands of type 'bool?' and 'bool' \$\endgroup\$
    – Zure
    Jan 5 '17 at 13:14
  • \$\begingroup\$ @Zure yup, you just need to add another condition, I edited the answer. \$\endgroup\$
    – t3chb0t
    Jan 5 '17 at 13:15
4
\$\begingroup\$

One thing that might help is switching to a "fail-fast" approach, where you test error conditions first and handle them immediately if one is detected. Execution in the method only continues if there are no errors, which means one less else block is required. That would give you this:

if (parameters == null)
{
    EventDateTime = DateTime.Today;
}
else if (!parameters.ContainsKey(PARAM_NAVIGATEDFROM))
{
    EventDateTime = DateTime.Today;
}
else if (parameters[PARAM_NAVIGATEDFROM] != typeof(MyCustomViewModel))
{
    EventDateTime = DateTime.Today;
}

Then, since the logic inside of all the blocks is identical, you could combine them into a single expression with a logical OR operator:
(This works because the operator has short-circuiting semantics.)

if ((parameters == null)                           ||
    (!parameters.ContainsKey(PARAM_NAVIGATEDFROM)) ||
    (parameters[PARAM_NAVIGATEDFROM] != typeof(MyCustomViewModel)))
{
    EventDateTime = DateTime.Today;
}

Alternatively, you could flip the logic around, testing to see whether the navigation is not coming from MyCustomView.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Couple this with storing to a self-explaining helper variable (t3chb0t) and generics (Greg Burghardt), and I think this would be the best answer. \$\endgroup\$ Jan 5 '17 at 19:02
  • \$\begingroup\$ Frankly, I'm not sure why everyone is so enamored with a helper variable. If you are only going to use the value stored in the variable once, I can't see why it is clearer to introduce one. Just look directly at the condition being evaluated. If it's confusing enough that it won't be immediately understood, then you need more than a CamelCasedVarName: add a descriptive comment instead. \$\endgroup\$
    – Cody Gray
    Jan 6 '17 at 5:26
4
\$\begingroup\$

Instead of seperate if statements you could combine them into one:

public void OnNavigatedTo(NavigationParameters parameters)
{
    if (parameters == null ||
        !parameters.ContainsKey(PARAM_NAVIGATEDFROM) ||
        parameters[PARAM_NAVIGATEDFROM] != typeof(MyCustomViewModel))
    {
        EventDateTime = DateTime.Today;
    }
}

You could go further and move the check to a function which will make it a bit easier to see what you are checking for at first glance. Plus if you ever need to make the same check again you can re-use the function.

Personally I would invert also the conditions so that the function checks that it is from MyCustomView rather than if it isn't.

public void OnNavigatedTo(NavigationParameters parameters)
{
    if (!IsNavigationFromMyCustomView(parameters))
    {
        EventDateTime = DateTime.Today;
    }
}

private bool IsNavigationFromMyCustomView(NavigationParameters parameters) 
{
    return parameters != null && 
        parameters.ContainsKey(PARAM_NAVIGATEDFROM) && 
        parameters[PARAM_NAVIGATEDFROM] == typeof(MyCustomViewModel);
} 
\$\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.