INotifyPropertyChanged, CallerMemberName and "Code Smell"
As a developer, I hate the term "code smell". It is a loosely defined, elitist term and thus often hurled around in an indefensible manner.
Where this is coming from is, I was looking to add some new classes to my wife's application as she too wants a scheduler of sorts added. But I was loathe to create another Model class with all of its INotifyPropertyChanged implementations and backing properties for the new table(s) I will need.
The first obvious step in refactoring was to move all of my old models to a common base class. Really not sure why I hadn't done this before. But, all that did was stop me from re-implementing INotifyPropertyChanged every single freakin' time. Not a huge gain. So I went looking.
I also hate backing properties, and was trying to see if there was a platform supported way of truly eliminating them. Doesn't exist yet. And, I probably could have come up with something on my own, but if I know what to Google for, I'll always take a tried and tested solution. Which lead me here.
This also reminded me that the CallerMemberName property was included in C# in a version of .Net my wife's work machine runs. Bonus! I can get rid of magic strings in my code as well!
But what kind of irritated me was that the author called backing properties a code smell, and seems to regard this approach as a solution. IF backing properties are indeed a code smell, then all this does is consolidates the smell into a single pile rather than littering it throughout the code. The backing properties aren't gone. It has just been transitioned to a single dictionary.
Don't get me wrong. I'm using a variation of this technique. And it improves the code in a number of ways. Consolidating "code smells" where possible is a great thing. If that code becomes a problem you have fewer places to address it in later. Consolidated code is also often more robust. More things hitting the same code means that single point gets tested far more heavily than individual points would.
But the code smell isn't gone. If in fact it was ever there to begin with. As I said in the beginning, there isn't one solid definition of a code smell. For me, it was always described as a sign of bad code. Backing properties aren't a sign of bad code here. With INotifyPropertyChanged, there is literally no way around it at present. And, if there were, it would simply be syntactic sugar.
The wasteful redundancy of writing individual backing properties for every property... that, to me is a code smell. Not the fact that they exist at all. And this does address that.
Then, if you go back in the series to part 1, he talks about using CallerMemberName to clear up another code smell. Magic strings. Again, this isn't really a code smell in this specific instance and it doesn't actually eliminate it. Why isn't is a code smell? WPF Bindings use magic strings. And so too does the underlying library. So if it is truly a code smell, it is the .Net code that smells and not your own. At the end of the day, magic strings is driving data binding.
But CallerMemberName eliminates the magic strings and gives you compile time validation! How does it not eliminate the code smell? CallerMemberName is part of CompilerServices (a fancy name for "stuff that doesn't actually happen at runtime"). And the actually IL that gets emitted? You guessed it! Uses the exact same literal "magic strings". It doesn't use reflection. It doesn't use anything inherently safe. It just uses unsafe techniques in a generally safer place. If dealing with backing properties was like moving smelly code into a giant heap, then this is like Febreezing the offending code. You can't smell the bad code anymore. But doesn't mean it is truly gone.
Once again though, I'd advocate for using this technique. The odds that you'll ship IL with the wrong magic strings or that someone will mess with your IL are much more unlikely than you screwing it up in your own code. But don't call it a solution to a code smell. This is just syntactic sugar that hides a code smell.
Part 3 I didn't even bother pursuing. Inverting where we indicate when dependent notifications should be fired is meaningless. Saying it is in the wrong order is purely a matter of perception and I don't think either is more correct than the other. In my opinion, the unmodified code makes it more obvious exactly when and where the dependent notification is fired. The "solution" proposed abstracts that away and requires you naming your attribute in a way that is universally understood.
The second problem for me is that the solution itself still leverages the same "code smell" CallerMemberName was JUST brought out to address earlier; magic strings. I'd almost argue that if your justification for eliminating the strings was more about eliminating the magic strings than it was about simplifying, part 3 simply proved that the effort was futile because you still have magic strings and they are still being used to fire notifications.
To end this though, I want to recap, the article itself is a great read. And the suggestions are generally quite great (including part 3 depending on personal preference). I am glad I found the articles.
As a developer who has been around a while, I thoroughly loathe the term "code smell". As with above, solutions often don't even truly resolve the "smells" they blame and whether or not something is actually a code smell at all is purely subjective to start with.
Most people don't learn just one programming language if they learn any at all and each language has its own philosophy and its own inherent "framework smells" that "rub off" on certain aspects of the code and the developer. We tend to bring some code smell baggage with us as we cross into new languages or become over zealous with certain philosophies. Bad code is a thing. But, what constitutes bad code is variable.
Where this is coming from is, I was looking to add some new classes to my wife's application as she too wants a scheduler of sorts added. But I was loathe to create another Model class with all of its INotifyPropertyChanged implementations and backing properties for the new table(s) I will need.
The first obvious step in refactoring was to move all of my old models to a common base class. Really not sure why I hadn't done this before. But, all that did was stop me from re-implementing INotifyPropertyChanged every single freakin' time. Not a huge gain. So I went looking.
I also hate backing properties, and was trying to see if there was a platform supported way of truly eliminating them. Doesn't exist yet. And, I probably could have come up with something on my own, but if I know what to Google for, I'll always take a tried and tested solution. Which lead me here.
This also reminded me that the CallerMemberName property was included in C# in a version of .Net my wife's work machine runs. Bonus! I can get rid of magic strings in my code as well!
But what kind of irritated me was that the author called backing properties a code smell, and seems to regard this approach as a solution. IF backing properties are indeed a code smell, then all this does is consolidates the smell into a single pile rather than littering it throughout the code. The backing properties aren't gone. It has just been transitioned to a single dictionary.
Don't get me wrong. I'm using a variation of this technique. And it improves the code in a number of ways. Consolidating "code smells" where possible is a great thing. If that code becomes a problem you have fewer places to address it in later. Consolidated code is also often more robust. More things hitting the same code means that single point gets tested far more heavily than individual points would.
But the code smell isn't gone. If in fact it was ever there to begin with. As I said in the beginning, there isn't one solid definition of a code smell. For me, it was always described as a sign of bad code. Backing properties aren't a sign of bad code here. With INotifyPropertyChanged, there is literally no way around it at present. And, if there were, it would simply be syntactic sugar.
The wasteful redundancy of writing individual backing properties for every property... that, to me is a code smell. Not the fact that they exist at all. And this does address that.
Then, if you go back in the series to part 1, he talks about using CallerMemberName to clear up another code smell. Magic strings. Again, this isn't really a code smell in this specific instance and it doesn't actually eliminate it. Why isn't is a code smell? WPF Bindings use magic strings. And so too does the underlying library. So if it is truly a code smell, it is the .Net code that smells and not your own. At the end of the day, magic strings is driving data binding.
But CallerMemberName eliminates the magic strings and gives you compile time validation! How does it not eliminate the code smell? CallerMemberName is part of CompilerServices (a fancy name for "stuff that doesn't actually happen at runtime"). And the actually IL that gets emitted? You guessed it! Uses the exact same literal "magic strings". It doesn't use reflection. It doesn't use anything inherently safe. It just uses unsafe techniques in a generally safer place. If dealing with backing properties was like moving smelly code into a giant heap, then this is like Febreezing the offending code. You can't smell the bad code anymore. But doesn't mean it is truly gone.
Once again though, I'd advocate for using this technique. The odds that you'll ship IL with the wrong magic strings or that someone will mess with your IL are much more unlikely than you screwing it up in your own code. But don't call it a solution to a code smell. This is just syntactic sugar that hides a code smell.
Part 3 I didn't even bother pursuing. Inverting where we indicate when dependent notifications should be fired is meaningless. Saying it is in the wrong order is purely a matter of perception and I don't think either is more correct than the other. In my opinion, the unmodified code makes it more obvious exactly when and where the dependent notification is fired. The "solution" proposed abstracts that away and requires you naming your attribute in a way that is universally understood.
The second problem for me is that the solution itself still leverages the same "code smell" CallerMemberName was JUST brought out to address earlier; magic strings. I'd almost argue that if your justification for eliminating the strings was more about eliminating the magic strings than it was about simplifying, part 3 simply proved that the effort was futile because you still have magic strings and they are still being used to fire notifications.
To end this though, I want to recap, the article itself is a great read. And the suggestions are generally quite great (including part 3 depending on personal preference). I am glad I found the articles.
As a developer who has been around a while, I thoroughly loathe the term "code smell". As with above, solutions often don't even truly resolve the "smells" they blame and whether or not something is actually a code smell at all is purely subjective to start with.
Most people don't learn just one programming language if they learn any at all and each language has its own philosophy and its own inherent "framework smells" that "rub off" on certain aspects of the code and the developer. We tend to bring some code smell baggage with us as we cross into new languages or become over zealous with certain philosophies. Bad code is a thing. But, what constitutes bad code is variable.
Comments
Post a Comment