• (nodebb)

    expiresIn = $"{accessToken.ExpiresIn}"

    I'd guess that if this syntax didn't exist in this form, and the cow-orker had to write this instead:

    expiresIn = accessToken.ExpiresIn.clone(); // assuming such a deep-copy method exists...

    the said cow-orker wouldn't be so keen on doing it. (And it would be seen more easily in code reviews...)

  • Sole Purpose Of Visit (unregistered)

    Hideous.

    But tbh I don't think there's much of an issue with GC, which in .Net is generational. None of these cloned strings are going to get moved to the next generation, so as far as I can see they're just (literally) a waste of space.

  • Chatogaster (unregistered)

    In $"{accessToken.ExpiresIn}", I'd expect ExpiresIn to be a member of the structure of accessToken (and not a local variable). Furthermore, I doubt C# guarantees (per spec) that the returned string occupies a new memory location. It shouldn't, since plain strings are immutable and optimizations (in this case that amounts to undoing stupidity) should not be made impossible.

    The circumstantial arguments provided may be wrong - or perhaps my 0.001% level of acquaintance with C# is insufficient - but what was done looks very silly regardless. It's very much like writing something like n = eval("n") + 1 to increment n (in evil languages that support eval's).

  • (nodebb)

    There is a subtle side-effect. If accessToken.ExpiresIn is null, then expiresIn will be a blank string

  • (nodebb) in reply to Steve_The_Cynic

    I'm not sure. That interpolation mess is also extremely ugly and highly visible in code reviews.

  • (nodebb)

    Really to a different memory location? There are some odd string optimizations in C#, which could cause two different strings with same (byte) content to point to the same memory location.

  • Hans (unregistered)

    Not really understanding what the types are and how they work. Just like what I often see: int myInt = int.Parse(myDataRow["intColumn"].ToString()) - a completely unnecessary conversion to and from string.

  • (nodebb) in reply to Mr. TA

    You're not wrong, except that I think it's a bit less visible than an explicit call to a clone() member. But either way, the interpolation mess is easier to just bang out unthinkingly (and, I'd guess, explodes in a different way if the variable changes its type or name or whatever) than the explicit call to a member function.

    Maybe.

  • Chatogaster (unregistered) in reply to HenrikHoyer

    I don't know C#, but that seems plausible and it's a good point.

    In other languages, similar constructs might return the string "null", and some might even decide to throw some kind of exception. If ExpiresIn were an integer (perhaps denoting a number of seconds), it might yield "123" or something unanticipated like "1.7E16" or "NaN".

    When using built-in string interpolation in the context of non-trivial interfaces, it's bound to open the door towards injections (the default example is SQL-injection, or for a more modern example: Log4J-injection).

    Moral: stay far away from using string interpolations, unless you can defend it's really the right thing to do.

  • AnAnonDevLead (unregistered) in reply to BernieTheBernie

    Agreed. In .NET, strings are interned. You are correct that this will not result in more (overall) memory allocation as suggested in the original article, unless the original string were null, in which case it could feasibly allocate a new empty string. It's a waste of resource though, performing the string interpolation which ends up yielding a reference to the original string.

  • Cole (unregistered)

    Based on my experience perusing IL dumps, the compiler is pretty smart when it comes to interpolated strings. Sometimes it'll use string.Concat, other times it'll use DefaultInterpolatedStringHandler. I threw this into SharpLab:

    string a = "abc";
string b = $"{a}";

    And it generated IL that decompiles to:

    string text = "abc";
string text2 = text ?? "";
  • Sole Purpose Of Visit (unregistered) in reply to Chatogaster

    I'd agree -- this is the WTF as far as I am concerned.

    Leaving aside the question of why one would use a string rather than a timestamp of some sort, it's hard to see why ExpiresIn should ever not be a member of the token. The current "code" (and I'm deliberately using scare quotes here) leaves open the possibility that there are different expiry strings stored in different places, which is ... suboptimal.

    Mind you, if this nonsense is plastered all over the code base, I'm tolerably certain that there are multiple other WTFs in low level design.

  • MaxiTB (unregistered)

    I am confused ... first of .net 7 eliminates all overheads by string interpolation on compiler level (so even before ngen), so there is no disadvantage over concrete implementations, even when the member is a string itself. And in the future those optimization will for sure improve.

    Secondly nothing is copied when string are in play in .net. Strings are immutable unique objects, they are (lazily) interned ALWAYS. This means any string "copy" is actually a hash lookup for a unique address.

    So I honestly don't see what is wrong with this code without any more context. It's actually perfectly fine.

  • (nodebb)

    Thanks to Cole for posting the details... (I have used this as a part of training/exams/interviews) for high end scenarios. The fun part is "what the compiler does" is usually quite different than "what the spec mandates" and the fun that can happen when different compilers make radically different (equally compliant) choices - even different versions by the same vendor, or based upon seemingly unrelated other considerations.

  • MaxiTB (unregistered) in reply to Cole

    I just wrote a small console app with Console.WriteLine for both values and the IL code is a LDSTR, DUP and double CALL Console.WriteLine (.net7 prev5). So no null check, no duplication of strings, just calling WriteLine twice with a constant loaded once.

  • oni (unregistered)

    TRWTF is that the RSS feed displayed this as: expiresIn = %CONTENT%#34;{accessToken.ExpiresIn}"

  • (nodebb) in reply to AnAnonDevLead

    I'm not sure but I would think that by now, they figured out a way to never allocate repetitive empty strings.

  • (nodebb) in reply to Hans

    @Hans int myInt = int.Parse(myDataRow["intColumn"].ToString()) - a completely unnecessary conversion to and from string.

    That is unfortunately not unnecessary, at least in the case I often find myself.

    If myDataRow is a DataRow object from System.Data.SqlClient, myDataRow["field"] returns an object. But all the .Parse and .TryParse want strings for their arguments, so you have to ToString() them. It's a bummer, wish *Parse accepted an object instead.

  • (nodebb) in reply to Bim Zively

    If myDataRow is a DataRow object from System.Data.SqlClient, myDataRow["field"] returns an object. But all the .Parse and .TryParse want strings for their arguments, so you have to ToString() them. It's a bummer, wish *Parse accepted an object instead.

    Way to miss the point. Sure, myDataRow["field"] returns an object, but you should know, based on the fact that you put the data in there, that it will come out as the same type as how you put it in. It should be perfectly safe to simply cast it to the type that you know it already is, unless all of your datasets are created from ad-hoc code.

    Microsoft provided strongly-typed rows many years ago that did exactly that. This particular solution wasn't all that great, so it has been superseded by many other strategies, but it still shows that developers have been trying to avoid round-tripping all of their data elements through string for a very long time. It's not just a performance concern - it's really hard to write reliable software if every time data is accessed a developer has to make a decision about what data they are expecting. Thousands of decisions makes for tens of bugs in the best of cases. These bugs can be categorically eliminated by imposing a sane data representation strategy.

  • Chris (unregistered)

    This is really clever code, actually. It makes sure that the local variable expiresIn can be created without worrying about how accessToken.ExpiresIn has been implemented. Is it a string? An int? Something other object with a suitable ToString() method? Who knows, and who cares?

    My tongue is firmly in cheek, but I guess there could be circumstances where you have no control over how accessToken has been implemented (e.g. third-party library), and it is prone to changes without notice.

  • Duston (unregistered)

    Could have been worse... expiresIn = $"{accessToken.ExpiresIn}".text().toString()

  • (nodebb) in reply to Chris

    My tongue is firmly in cheek, but I guess there could be circumstances where you have no control over how accessToken has been implemented (e.g. third-party library), and it is prone to changes without notice.

    It still isn't the right solution. The code was stated to have been C# and var was invented specifically to address this condition. As long as the change was compatible with how the data was being used, var would properly compile to the correct code. If the change somehow made the usage invalid, the use of var would result in a compile error. Win-win.

  • Freddy the Ready (unregistered) in reply to Hans

    I don't necessarily agree that it is unnecessary. I've seen plenty of cases where the type coming back from a database query is not quite the type you expect. e.g. the type is an Int16 not an Int32, and so doing a direct read to an int results in an error. But... what I will agree on is that this is a pretty rubbish way of getting around the problem!

  • WTFGuy (unregistered)

    @MaxiTB: So what you mean is: the compiler is smart enough to simply ignore stupid code by stupid coders and your response is "that's fine"? I respectfully disagree. There is nothing fine about stupid code stupidly written. The primary audience for source code is not compilers. It's maintenance programmers. And the author of this code is saying loud and clear: "I am dangerously ignorant of thinking in general, programming in specific, and largely clueless about how my language works and how to use it." That is not the sign of somebody likely to produce quality code low in unintended side effects, odd corner cases and just general incorrectness even in the mainstream case.

    Instead it's a gigantic "I should have been fired after no more than a week on the job" sign.

  • farhankanju (unregistered)
    Comment held for moderation.

Leave a comment on “Tying Two Strings”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article