C# Duplicated usage of an if/else condition and a ternary operator. A good practice?

Question

I had a debate with a work mate regarding the following code as to which one of it would be the better practice:

My code (in pseudo):

var A = <precondition either TRUE or FALSE>;
var B;
if(A)
{
    // some other multi-line, complex computation based on A=TRUE
    B = <value fetched from JsonElement .GetProperty("C");>
}
else
{
    B = <value fetched from JsonElement .GetProperty("D");>
}

var E = B; // B is carried forward based on the condition

Their code:

var A = <precondition either TRUE or FALSE>;
var B = A ? <value fetched from JsonElement .GetProperty("C") 
        : <value fetched from JsonElement .GetProperty("D");
if(A)
{
    // some other multi-line, complex computation based on A=TRUE
}

var E = B;

My view was their usage of the ternary operator is essentially a shorthand of the IF condition that followed and that it is pretty much a duplication where we could consolidate the logic we need to work out into a single conditional operator, but they were persistent it was not and that the usage of ternary operator is better for the sake of brevity and readability.

Could anyone shed some light here as to what the best practice would look like? Duplicating a conditional operator for cleaner code or having a simple consolidated if/else condition?

Thanks!

Answer 1

I don't like both versions. Instead, I would prefer some code like

   B = <value fetched from JsonElement .GetProperty(A ? "C" :"D");>

or maybe

key = A ? "C" :"D"`;
B = <value fetched from JsonElement .GetProperty(key);

The duplication I would avoid here is not the duplicate test for A. The real problem shows up when one duplicates lines like this one:

  B = <some complex expression which differs only slightly on A>

Having a complex expression twice in code is far more error prone than having a test for a boolean twice.

In fact a complex calculation which is required twice leads straightforward to a separate function, which solves the issue for both snippets in your question. So let's see what happens when you try to refactor the two snippets by extracting the expression for B into a function of its own. This gives you either

if(A)
{
    // some complex computation 

    B = CalculateB(true);
}
else
{
    B = CalculateB(false);
}

or

if(A)
{
    // some complex computation 
}
B = CalculateB(A);

I would usually prefer the second alternative, simply because it is shorter and saves the duplicate call to CalculateB.

Answer 2

Doc's asnwer is pretty good already.

I, personally, would do something different, however, using Local Functions:

    var A = getValueForA();

    string doThingieIfA()
    {
        doProcessA();
        return FetchFromJSon("C");
    }

    string doThingieIfB()
    {
        return FetchFromJSon("D");
    }

    var B = A ? doThingieIfA() :  doThingieIfB());

Answer 3

I'm not at all a fan of the conditional operator being spread over multiple lines. I think you either keep it on one line, or you use the proper if-block.

So in that respect, I prefer your version to your colleague's version.

But there are cases where it can be useful to repeat the test, as a lesser evil than conflating two pieces of disparate logic:

var A = <precondition either TRUE or FALSE>;
var B;

if (A) {
    // some other multi-line, complex computation based on A=TRUE
}

if (A) {
    B = <value fetched from JsonElement>.GetProperty("C");
} else {
    B = <value fetched from JsonElement>.GetProperty("D");
}

var E = B; // B is carried forward based on the condition

I think @DocBrown also has something good with his second example (applied example):

var A = <precondition either TRUE or FALSE>;

if (A) {
    // some other multi-line, complex computation based on A=TRUE
}

var B = <value fetched from JsonElement>.GetProperty(A ? "C" : "D");

var E = B; // B is carried forward based on the condition

Always remember though that opinions and preferences can differ (after all, you've already got more opinions than answerers here), and it is best to defer to the person actually writing the code.

Now, let's get to the real point of conflict: how about my brace style!