- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
I'm accepting bets that IsNotValid is a simple null check.
Admin
From this snippet, we can't actually tell the type of
viewNodeFilter
(the probably private field that is used by theViewNodeFilter
property - note the difference in casing). All we know is thatviewNodeFilter = value
is valid whenvalue
is a string. The field could be some custom type that has an implicit conversion operator defined.So this may not be an abuse of extension methods. It could just as easily be an abuse of implicit conversions, instead.
Admin
I'm guessing that the backing field viewNodeFilter is an object, not a string. That's why it has a method and the public property returns that private field as a serialized string.
Weird choice of each having different types nonetheless
Admin
Reminds me of something I saw in our flagship app's code (written by a different team): They had a get property that did a database lookup.
Yikes.
Admin
From one of my mentors:
"No force in the universe, NOT EVEN GOD HIMSELF, will ever convince me that a baby doesn't die every time someone either directly or indirectly makes a database call in a constructor."
Admin
The type of the property is a string, and the backing field is assignable from a string. AFAIK, there isn't a way to magically upconvert strings to objects in .NET.
Admin
This code is so weird on many levels that it seems to be made up honestly.
System.String is sealed, so it can't be inherited.
You can write an implicit operator to cast an object to a string and the other way around, but this sounds super far fetched honestly.
The code would look like this:
And yes, those operators are only be allowed to be located in one of the affected objects and since Syste,String is closed, the code has to be in ViewNodeFilter.
But again, this sounds so far fetched, I honestly doubt the code is real - it would actually take a lot of experience to make some as stupid like that work which makes it doubtful that someone would abuse a property like that. But eh, I have seen a lot of stupid stuff from seemingly "experienced" developer, so there's always a tiny possibility that this is actually something real haha
Addendum 2024-07-22 16:29: Nvm, I read the code completely wrong on my mobile lol
Admin
"...take a lot of experience to make some as stupid like that work " Objection! Assumes facts not in evidence
Admin
I once debugged a couple of Python lines that looked absolutely innocent but for some reason took forever to complete:
temp_histogram = sensor.histogram curr_histogram = sensor.histogram
in the end I found out that sensor.histogram was a Python decorator hiding a "getter" function which was actually fetching data from the hardware via a 9600 baud serial protocol... an operation which took from 3 to 5 seconds to complete... and these two lines were inside a loop touching all sensors... 50% speed boost by just replacing the second line with "curr_histogram = temp_histogram"!!!
Admin
Magic properties belong in Vegas, not in your code.
Admin
One of the bits of code from a former employer that sticks in my mind was C#:
All hell breaks loose if you delete the apparent redundancy - it's doing initialisation of Bar behind the scenes as a side-effect of the getter.
Admin
mmmm ... cereal
Admin
mmmm ... cereal
Admin
whoops, double post.
Post ... Raisin Bran. yum.
Admin
I'm thinking that this is part of an attempt to clean things up. Originally the JSON was stored in a string and this is an attempt to separate it out but for some reason it has to retain it's string nature. Or this might be a step towards replacing the String with a JSON type that someone was working on when they didn't have something more important to do and it simply hasn't been finished.
I know I've done cleanups that way--build it right but keep patching the new into the old so everything else behaves. Eventually the old simply calls the new and can be stripped.
Admin
public class Foo{ private string FooValue;
public Foo(string val) { FooValue = val; }
public static implicit operator Foo(string val) { return new Foo(val); } }
Foo bar = "Magic is real";
Admin
The getter is protected, meaning only an instance of the containing object's class, or a derived class, can access it. But the setter is public, accessible by anyone anywhere. Usually it's the other way around, so everyone else can look but not touch, so I'm trying to see how this is used.
It seems like you get an instance of this class, and feed it a string value. Then call a method on it, or pass the instance to something else that calls a method on it, part of which it gets the serialised version of the string value and does something with it. Even if it had multiple methods that all need to use the serialised version of the string, doing it with one property is wrong.
Applying the serialisation when it is set would probably make more sense, but it would be better to then store the serialised string as a separate field. Or you could have a protected method that produces the serialised string, but like the current implementation, that would be run every time you needed it.
Admin
@Jules ref
Lots of properties are lazy-initialized on first access. This may be a more a matter of trying to get the lazy initialization done once ahead of time before entering the event-driven multi-threaded aynsc part of the rest of the code which uses
Bar
object(s). Which themselves don't have any kind of race prevention.There are lots of ways to end up with e.g. library code that's inherently single-threaded being used in larger multi-threaded apps where there's no way to fix the library, so you have to protect it from races explicitly at a much higher level in your app than you wish you did.
Admin
With some of the code bases I've worked with, doing something like that would cause some other code to break because it relied on sensor.histogram being called twice.