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?

share|improve this question
1  
And what should happen if it is coming from MyCustomViewModel ? – Heslacher 17 hours ago
    
Some other stuff (which always happens no matter from which View I'm navigating from, so I didn't include it here for readability). – Zure 17 hours ago
3  
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; – Heslacher 17 hours ago
1  
@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? – Zure 17 hours ago
up vote 5 down vote accepted

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);
share|improve this answer
    
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' – Zure 17 hours ago
    
@Zure yup, you just need to add another condition, I edited the answer. – t3chb0t 16 hours ago

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.

share|improve this answer
1  
yeah, using generics is a good idea – t3chb0t 16 hours ago

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.

share|improve this answer
    
Couple this with storing to a self-explaining helper variable (t3chb0t) and generics (Greg Burghardt), and I think this would be the best answer. – Patrick Roberts 11 hours ago
    
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. – Cody Gray 48 mins ago

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);
} 
share|improve this answer

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.