2

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!

5
  • 2
    I do not think there is a single correct answer. I tend to prefer the terniary operator for short code, but for anything long or complex it tend to become difficult to read. You might also consider a switch expression as a third option.
    – JonasH
    Commented Dec 1, 2022 at 13:24
  • Thanks JonasH. I figured this is more about individual preference than a 'single best approach'. I didn't opt to consider a switch operation because we were only ever going to have A and A' conditions, nothing further
    – Chams
    Commented Dec 1, 2022 at 14:08
  • Is fetching values from JsonElement a heavy computational task? Couldn't you simply initiate B as <value fetched from JsonElement .GetProperty("D") and remove the else?
    – Laiv
    Commented Dec 1, 2022 at 14:43
  • "We were only ever going to have A and A' conditions, nothing further" -- I've heard that a few times before.
    – GHP
    Commented Dec 1, 2022 at 21:45
  • 2
    This is too vague to make a judgment call on. A more concrete example would be helpful.
    – Flater
    Commented Dec 1, 2022 at 22:59

3 Answers 3

7

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.

2
  • Thank you for the detailed feedback Doc. I have added an update to the initial question that the inside of the A=TRUE is a multi-line, complex computation. I wouldn't prefer to use an inline function to make it even more complex, just wondering how I can tackle it with your suggested one liner
    – Chams
    Commented Dec 1, 2022 at 14:01
  • @Chams: I think you missed my point, so see my edit.
    – Doc Brown
    Commented Dec 1, 2022 at 16:28
0

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());
1
  • People don't like Local Functions.
    – T. Sar
    Commented Dec 2, 2022 at 14:28
0

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!

1
  • 2
    Mixing C# and Egyptian Brackets may lead to the unintended summoning of unspeakable Eldtritch Horrors. Be sure to have an exorcist at hand at all times.
    – T. Sar
    Commented Dec 1, 2022 at 19:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.