code sexiness question

[OriginalGriff]

The first relies on the empty delegate: if someone else sees the code and removes it as it clearly does nothing then you are back to a potential failure. Unlikely, yes - but I don't like app failures. :D The second is .NET version dependant: the null conditional operator was introduced at C# 6, and some of my code predates that.

"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!

[11917640 Member]

Very good good code review result. Absolutely meaningless and harmless change. You and code reviewer, and of course, managers, are happy. BTW, after native optimizations both versions may be absolutely identical. But don't tell about this to your manager.

[snorkie]

Are we coding in C# or Perl? I always felt like C# was a nice balance between COBOL and Perl. But the recent changes have it trending towards Perl. I hear the excuse "it saves typing" as if we aren't on a message board or slack typing all day long.

Hogan

[Lost User]

I have problems with both. "var" is a coding shortcut when you're too lazy to type, and should be replaced with the actual type when it resolves (before release); so the next guy doesn't have to "intelli-sense" it. "Object" seems more appropriate in this case. I dislike "long" if's and would have "if not null return x" instead of (if null etc.) Even though it works, I don't declare properties "after" the method that references them. The return "assign" is "different". Then there are the if's with bracketed blocks and some without. Saving keystrokes? etc.

It was only in wine that he laid down no limit for himself, but he did not allow himself to be confused by it. ― Confucian Analects: Rules of Confucius about his food

[Gary R Wheeler]

I would have done things this way:

private IMultipleComponentHandler SelectionHandler
{
get
{
if (m_selectionHandler == null)
{
if (SelectedObject is IMultipleComponentHandler handler)
{
m_selectionHandler = handler;
}

        else
        {
            object\[\] collection;
            
            if (SelectedObject is IEnumerable e
                && !SelectedObject.GetAttributes().Any())
            {
                collection = e as object\[\] ?? e.Cast().ToArray();
            }
            else if (SelectedObject != null)
            {
                collection = new\[\] { SelectedObject };
            }
            else
            {
                collection = Array.Empty();
            }

            m\_selectionHandler = new InspectorMultipleComponentHandler(collection);
        }
    }
    
    return m\_selectionHandler;
}

}
private IMultipleComponentHandler m_selectionHandler;I'm an old fart who prefers single-exit :-D .

Software Zen: delete this;

[Fueled By Decaff]

I would be tempted to extract the contents of the if statement as a function, like this:

    public IMultipleComponentHandler SelectionHandler
    {
        get
        {
            if (m\_selectionHandler == null)
            {
                m\_selectionHandler = InitialiseSelectionHandler(SelectedObject);
            }
            return m\_selectionHandler;
        }
    }

    private IMultipleComponentHandler InitialiseSelectionHandler(object selectedObject)
    {
        if (selectedObject is IMultipleComponentHandler handler)
            return m\_selectionHandler = handler;

        object\[\] collection;
        if (selectedObject is IEnumerable e
            && !selectedObject.GetAttributes().Any())
        {
            collection = e as object\[\] ?? e.Cast().ToArray();
        }
        else if (selectedObject != null)
        {
            collection = new\[\] { selectedObject };
        }
        else
        {
            collection = Array.Empty();
        }
        return new InspectorMultipleComponentHandler(collection);
    }

    private IMultipleComponentHandler m\_selectionHandler;

This hides the issue of using a temporary variable, snapshots the SelectedObject so the value being used can not change while updating the handler and the getter property for SelectionHandler becomes easier to read.

An added bonus is if SelectedObject changes you can update the handler by calling InitialiseSelectionHandler.

[Daniele Rota Nodari]

This is way cleaner.. It is a sort of lazy initialization.. and usually it is split exactly that way: a backing field, a property that lazily initializes the field, and a method that performs the initialization. This also highlight a possible race condition after the very first null check, that can be avoided by a double-checked locking inside the property get accessor; and the InitialiseSelectionHandler method would remain untouched. I'd just keep the backing field close to the property (I usually keep the field immediately before the property).

[Andre_Prellwitz]

It’s unlikely the compiler knows that no other thread may change the SelectedObject during the getter body. One should acknowledge that fact by capturing its value, not for speed or readability, but for correctness. That so many people find it trivial or overly optimizing is really scary. Pragmatically it may be a non-issue, but it’s a time-bomb and there’s arguably no justification for the “simpler” syntax. Concurrency is hard.

[Andre_Prellwitz]

You’re not wrong, but you’re also not necessarily right. Coding preferences should be agreed upon, codified, and automated to avoid debate. The important thing is that there’s consistency. Calling a coding practice “lazy” (like the use of ‘var’) is condescending, at best, and smells of arrogance. It’s also associated with narrow-mindedness, and quite frankly, can date you in a bad way. I’m sure the intent was a call for action, but your reasoning and diction could be improved. Very simply, the guidance on the use of ‘var’ states that it should be used only if the type is repeated or obvious (without IntelliSense) on the right side of the equals sign; this simultaneously makes it easier to spot variable declarations while respecting the intelligence of the reader, who may not really care what the type is up front, especially if the name is well-chosen or the type name is long. A similar logic can be applied, per your preference, for the implicit new() operator, though I tend to see those used mostly on field initializers. Having said that, the Framework Design Guidelines recommends *against* the use of var, except when using ‘new’, ‘as’, or a hard cast, in which cases it is *permissible*.

[englebart]

Careful with the double checked locking… I suspect C# might fall under the Java or C++ umbrella. [The "Double-Checked Locking is Broken" Declaration](https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)