code sexiness question
-
Ok, you mentioned speed but it did not occur to me that this was the very reason for the change. When not performance critical, readability takes over anything else. Maintaining code is a nightmare, maintaining spaghetti code is a PITA inside the nightmare.
yea, yea, I totally agree.. this comes from C++ developer doing C#.... they pretend be very concerned by speed but their own code is an indecipherable big bowl of spaghetti... :doh:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
The first method below is probably 3e-9 seconds faster per call than the second method... And a code reviewer asked that I used that syntax
private IMultipleComponentHandler SelectionHandler
{
get
{
if (m_selectionHandler == null)
{
var objects = SelectedObject; // <== MAIN DIFFERENCEif (objects is IMultipleComponentHandler handler) return m\_selectionHandler = handler; object\[\] collection; if (objects is IEnumerable e && !objects.GetAttributes().Any()) { collection = e as object\[\] ?? e.Cast().ToArray(); } else if (objects != null) { collection = new\[\] { objects }; } else { collection = Array.Empty(); } return m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;but... that extra variable annoys me (
var objects = SelectedObject;), I see it as increasing code complexity for little benefit. I prefer that simpler versionprivate IMultipleComponentHandler SelectionHandler { get { if (m\_selectionHandler == null) { 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 m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;What says you?
For the record this is in a view model, this code is absolutely NOT performance critical.
A new .NET Serializer
All in one Menu-Ribbon Bar
Taking over the world since 1371!If we are talking about sexiness it annoys me way more that you have if (m_selectionHandler == null) instead of if (m_selectionHandler != null) return m_selectionHandler;
MessageBox.Show(!string.IsNullOrWhiteSpace(_signature)
? "This is my signature: " + Environment.NewLine + _signature
: "404-Signature not found"); -
If we are talking about sexiness it annoys me way more that you have if (m_selectionHandler == null) instead of if (m_selectionHandler != null) return m_selectionHandler;
MessageBox.Show(!string.IsNullOrWhiteSpace(_signature)
? "This is my signature: " + Environment.NewLine + _signature
: "404-Signature not found");ha, err... yea, I don't care either way.. but now that you mention it, I usually tend to it more that way too.. mm... I think what happens is some previously annoying code comment and refactoring, so it ended up that way...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
If we are talking about sexiness it annoys me way more that you have if (m_selectionHandler == null) instead of if (m_selectionHandler != null) return m_selectionHandler;
MessageBox.Show(!string.IsNullOrWhiteSpace(_signature)
? "This is my signature: " + Environment.NewLine + _signature
: "404-Signature not found"); -
ha, err... yea, I don't care either way.. but now that you mention it, I usually tend to it more that way too.. mm... I think what happens is some previously annoying code comment and refactoring, so it ended up that way...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Well, you know i didn't use it much at first but then i found it very useful, since i prefer reading a method not like a chapter of a book but a "flowchart". So if i get into that method i don't want to scroll down to see what happens if something is something but i want to know what happens if this is true, okay return. Next "branch" if this happens, do that and return. And so on. Makes readability in my opinion way better and if you are looking for a specific branch of actions you may find it faster. But that's just my approach :)
MessageBox.Show(!string.IsNullOrWhiteSpace(_signature)
? "This is my signature: " + Environment.NewLine + _signature
: "404-Signature not found"); -
Well, you know i didn't use it much at first but then i found it very useful, since i prefer reading a method not like a chapter of a book but a "flowchart". So if i get into that method i don't want to scroll down to see what happens if something is something but i want to know what happens if this is true, okay return. Next "branch" if this happens, do that and return. And so on. Makes readability in my opinion way better and if you are looking for a specific branch of actions you may find it faster. But that's just my approach :)
MessageBox.Show(!string.IsNullOrWhiteSpace(_signature)
? "This is my signature: " + Environment.NewLine + _signature
: "404-Signature not found");exactly my thoughts process too! but, those code reviews are getting to me, too many stupid comments, and style guideline I don't like, so those days I just give up and do whatever the hell they say, but that also puts me in a bad mood and makes me think.. less clearly shall we say...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
The first method below is probably 3e-9 seconds faster per call than the second method... And a code reviewer asked that I used that syntax
private IMultipleComponentHandler SelectionHandler
{
get
{
if (m_selectionHandler == null)
{
var objects = SelectedObject; // <== MAIN DIFFERENCEif (objects is IMultipleComponentHandler handler) return m\_selectionHandler = handler; object\[\] collection; if (objects is IEnumerable e && !objects.GetAttributes().Any()) { collection = e as object\[\] ?? e.Cast().ToArray(); } else if (objects != null) { collection = new\[\] { objects }; } else { collection = Array.Empty(); } return m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;but... that extra variable annoys me (
var objects = SelectedObject;), I see it as increasing code complexity for little benefit. I prefer that simpler versionprivate IMultipleComponentHandler SelectionHandler { get { if (m\_selectionHandler == null) { 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 m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;What says you?
For the record this is in a view model, this code is absolutely NOT performance critical.
A new .NET Serializer
All in one Menu-Ribbon Bar
Taking over the world since 1371!The "extra" variable doesn't bother me. I don't think there's much difference in the cognitive load required to understand each function. Given that the first one passed code review, it's "sexier", IMO. Edit: I have to add, is the extra time you're spending trying to decide which version is more readable adding the requisite amount of value? *hides*
Real programmers use butterflies
-
The first method below is probably 3e-9 seconds faster per call than the second method... And a code reviewer asked that I used that syntax
private IMultipleComponentHandler SelectionHandler
{
get
{
if (m_selectionHandler == null)
{
var objects = SelectedObject; // <== MAIN DIFFERENCEif (objects is IMultipleComponentHandler handler) return m\_selectionHandler = handler; object\[\] collection; if (objects is IEnumerable e && !objects.GetAttributes().Any()) { collection = e as object\[\] ?? e.Cast().ToArray(); } else if (objects != null) { collection = new\[\] { objects }; } else { collection = Array.Empty(); } return m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;but... that extra variable annoys me (
var objects = SelectedObject;), I see it as increasing code complexity for little benefit. I prefer that simpler versionprivate IMultipleComponentHandler SelectionHandler { get { if (m\_selectionHandler == null) { 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 m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;What says you?
For the record this is in a view model, this code is absolutely NOT performance critical.
A new .NET Serializer
All in one Menu-Ribbon Bar
Taking over the world since 1371!Storing the value of the property (
SelectedObject) into a local variable (objects) is like a snapshot. It ensures that any subsequent instruction refers to the same object. This is important when you need consistency throughout the method, the method can be relatively time consuming and some other thread can concurrently change the property value. You can always rely on compiler optimizations and hope that the compiled machine code will take care of such thing, or you can do it by yourself with the local variable. Another reason could be that you know in advance that someone will add code inside the method to purposely change the object value; such change will require to put the object into a variable, like the collegue told you to do. Doing it later will require to replace any reference to the property with references to the variable: those changes would be spread along the method polluting versioning differences. It can even be of help if copy-paste is performed to some other method where the logic must be kept but the property to process has a different name. These are indeed pre-optimizations, and we can argue about their usefullness and their development cost. Additionally, the name of the property is not totally meaningful, due to the fact it is singular but it can store multiple objects (like the name of the variable unfolds): this could be an important hint for the future-you maintaining the code. -
exactly my thoughts process too! but, those code reviews are getting to me, too many stupid comments, and style guideline I don't like, so those days I just give up and do whatever the hell they say, but that also puts me in a bad mood and makes me think.. less clearly shall we say...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
If you can't change the rules, break the rules XD Nah srsly, i tend to go on change it, love it or leave it mentality. This solves many frustration related issues for me so far.
MessageBox.Show(!string.IsNullOrWhiteSpace(_signature)
? "This is my signature: " + Environment.NewLine + _signature
: "404-Signature not found"); -
The first method below is probably 3e-9 seconds faster per call than the second method... And a code reviewer asked that I used that syntax
private IMultipleComponentHandler SelectionHandler
{
get
{
if (m_selectionHandler == null)
{
var objects = SelectedObject; // <== MAIN DIFFERENCEif (objects is IMultipleComponentHandler handler) return m\_selectionHandler = handler; object\[\] collection; if (objects is IEnumerable e && !objects.GetAttributes().Any()) { collection = e as object\[\] ?? e.Cast().ToArray(); } else if (objects != null) { collection = new\[\] { objects }; } else { collection = Array.Empty(); } return m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;but... that extra variable annoys me (
var objects = SelectedObject;), I see it as increasing code complexity for little benefit. I prefer that simpler versionprivate IMultipleComponentHandler SelectionHandler { get { if (m\_selectionHandler == null) { 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 m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;What says you?
For the record this is in a view model, this code is absolutely NOT performance critical.
A new .NET Serializer
All in one Menu-Ribbon Bar
Taking over the world since 1371!Just for giggles, how about:
private IMultipleComponentHandler SelectionHandler => m_selectionHandler ??= SelectedObject switch
{
null => new InspectorMultipleComponentHandler(Array.Empty<object>()),
IMultipleComponentHandler handler => handler,
object[] e when !e.GetAttributes<IgnoreIEnumerableAttribute>.Any() => e,
IEnumerable e when !e.GetAttributes<IgnoreIEnumerableAttribute>.Any() => e.Cast<object>().ToArray(),
var e => new InspectorMultipleComponentHandler(new[] { e }),
};As with your first example, this only accesses
SelectedObjectonce.
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
Just for giggles, how about:
private IMultipleComponentHandler SelectionHandler => m_selectionHandler ??= SelectedObject switch
{
null => new InspectorMultipleComponentHandler(Array.Empty<object>()),
IMultipleComponentHandler handler => handler,
object[] e when !e.GetAttributes<IgnoreIEnumerableAttribute>.Any() => e,
IEnumerable e when !e.GetAttributes<IgnoreIEnumerableAttribute>.Any() => e.Cast<object>().ToArray(),
var e => new InspectorMultipleComponentHandler(new[] { e }),
};As with your first example, this only accesses
SelectedObjectonce.
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
we're still stuck on .NET 4.7.2 at the moment.... :(( not for long I heard .NET 6 is coming, like the winter!
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
Just for giggles, how about:
private IMultipleComponentHandler SelectionHandler => m_selectionHandler ??= SelectedObject switch
{
null => new InspectorMultipleComponentHandler(Array.Empty<object>()),
IMultipleComponentHandler handler => handler,
object[] e when !e.GetAttributes<IgnoreIEnumerableAttribute>.Any() => e,
IEnumerable e when !e.GetAttributes<IgnoreIEnumerableAttribute>.Any() => e.Cast<object>().ToArray(),
var e => new InspectorMultipleComponentHandler(new[] { e }),
};As with your first example, this only accesses
SelectedObjectonce.
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
The "extra" variable doesn't bother me. I don't think there's much difference in the cognitive load required to understand each function. Given that the first one passed code review, it's "sexier", IMO. Edit: I have to add, is the extra time you're spending trying to decide which version is more readable adding the requisite amount of value? *hides*
Real programmers use butterflies
I am just fed up with all those rubbing me wrong micro management useless comments... I try to just shrug it off... But it annoys me every time some (of those particular) guys reviews... but on the other hand getting any review at all is also hard work, so bloody annoying...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
we're still stuck on .NET 4.7.2 at the moment.... :(( not for long I heard .NET 6 is coming, like the winter!
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
If you're using VS2019 or 2022, you can still use that construct in .NET 4.7.2; you just need to manually edit your project file to enable C# 9. :) If you already have a
<LangVersion>element in the file, change it to<LangVersion>9.0</LangVersion>. Otherwise, add that element next to the<TargetFramework>element. Quite a few C# 8/9/10 features will work in .NET Framework projects: Using C# 9 outside .NET 5 · Discussion #47701 · dotnet/roslyn · GitHub[^]
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
Whiskey. Tango. Foxtrot. :omg:
Freedom is the freedom to say that two plus two make four. If that is granted, all else follows. -- 6079 Smith W.
switch expression - C# reference | Microsoft Docs[^] Pattern matching overview - C# guide | Microsoft Docs[^] :)
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
The first method below is probably 3e-9 seconds faster per call than the second method... And a code reviewer asked that I used that syntax
private IMultipleComponentHandler SelectionHandler
{
get
{
if (m_selectionHandler == null)
{
var objects = SelectedObject; // <== MAIN DIFFERENCEif (objects is IMultipleComponentHandler handler) return m\_selectionHandler = handler; object\[\] collection; if (objects is IEnumerable e && !objects.GetAttributes().Any()) { collection = e as object\[\] ?? e.Cast().ToArray(); } else if (objects != null) { collection = new\[\] { objects }; } else { collection = Array.Empty(); } return m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;but... that extra variable annoys me (
var objects = SelectedObject;), I see it as increasing code complexity for little benefit. I prefer that simpler versionprivate IMultipleComponentHandler SelectionHandler { get { if (m\_selectionHandler == null) { 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 m\_selectionHandler = new InspectorMultipleComponentHandler(collection); } return m\_selectionHandler; } } private IMultipleComponentHandler m\_selectionHandler;What says you?
For the record this is in a view model, this code is absolutely NOT performance critical.
A new .NET Serializer
All in one Menu-Ribbon Bar
Taking over the world since 1371!I'd use the first version - that way if SelectedObject is changed (by another thread for example) the non-null value is preserved and the app doesn't crash. It's the way I handle event raising - my standard template code is:
/// /// Event to indicate Description /// public event EventHandler Name; /// /// Called to signal to subscribers that Description /// /// protected virtual void OnName(EventArgs e) { EventHandler eh = Name; if (eh != null) { eh(this, e); } }That way, in the (unlikely) event that the last handler is removed from the c=hain, the app doesn't crash and does something sensible.
"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!
-
If you're using VS2019 or 2022, you can still use that construct in .NET 4.7.2; you just need to manually edit your project file to enable C# 9. :) If you already have a
<LangVersion>element in the file, change it to<LangVersion>9.0</LangVersion>. Otherwise, add that element next to the<TargetFramework>element. Quite a few C# 8/9/10 features will work in .NET Framework projects: Using C# 9 outside .NET 5 · Discussion #47701 · dotnet/roslyn · GitHub[^]
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
Interesting... Though in our case there is some sort of build system, which is still mysterious to me, that generate the .csproj files.. so I would need to get familiar with that first! :laugh: In fact... I really ought to become more familiar with this particular system.... :sigh:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
I'd use the first version - that way if SelectedObject is changed (by another thread for example) the non-null value is preserved and the app doesn't crash. It's the way I handle event raising - my standard template code is:
/// /// Event to indicate Description /// public event EventHandler Name; /// /// Called to signal to subscribers that Description /// /// protected virtual void OnName(EventArgs e) { EventHandler eh = Name; if (eh != null) { eh(this, e); } }That way, in the (unlikely) event that the last handler is removed from the c=hain, the app doesn't crash and does something sensible.
"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!
Two obvious alternatives to that:
public event EventHandler Name = delegate { };
protected virtual void OnName(EventArgs e)
{
Name(this, e); // Name can never be null
}public event EventHandler Name;
protected virtual void OnName(EventArgs e)
{
Name?.Invoke(this, e);
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
I am just fed up with all those rubbing me wrong micro management useless comments... I try to just shrug it off... But it annoys me every time some (of those particular) guys reviews... but on the other hand getting any review at all is also hard work, so bloody annoying...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Understandable. I can't stand code reviews myself, because I have some different philosophies about how code needs to be written than a lot of people I have worked with. However, when I am in a position where I am in charge of code reviews, I tend to go easy and stick to enforcing in-shop style guidelines more than anything. I don't care about fast for bizdev unless something is slow enough you want to get out and push. I would have accepted either version of your code. I think both are readable *enough* - and this is one of the areas where I differ with a lot of people. I don't spend as much time chasing readability as other coders. I like to look at cognitive load more than readability, because I feel like readability can be had by reading the complicated parts of a function more than once. The trick is in *understanding* what you've read. That's the part where I care, but also the part I'm not great at. One of the reasons I write here is to try to improve my skillset in terms of making my code understandable. My functions are too long, but that's due to some cognitive issues I have myself, and it's part of how I've adapted to them.
Real programmers use butterflies
-
Two obvious alternatives to that:
public event EventHandler Name = delegate { };
protected virtual void OnName(EventArgs e)
{
Name(this, e); // Name can never be null
}public event EventHandler Name;
protected virtual void OnName(EventArgs e)
{
Name?.Invoke(this, e);
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
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!
