• Blubb (unregistered)

    var frist = new Frist(); frist = getFrist(); return frist;

  • Prime Mover (unregistered)

    Perhaps the user wanted the option to put a debug statement in between the array access and the return. I've done similar myself when I've been been given to access a messy old class which may or may not be all that well defined. But I wouldn't have done the var, and I would probably have given the method a more streamlined name.

  • (nodebb)

    Oh, yeah, and it's a private method. I wonder how bloated that class is with goofball private accessor methods. If it were a public method it might somehow make some sort of sense by some API standard. But this? No.

  • Remcoder (unregistered)

    My guess would be that someone deemed a simple mere list lookup to be not enterprise-y enough, and they added this function.

  • Anon (unregistered)

    And even if they wanted a default value, one could just write "researchControls.ElementAtOrDefault(theIndex) ?? new ResearchControl()" or even throw an exception without using a method at all...

  • WTFGuy (unregistered)

    Now I'm just fussing about the hairstyle of the pig, but as written it creates and immediately discards a new ResearchControl. The GC will just love this on a hot code path.

    And yes, the extremely descriptive method name that's all about mechanism, not business purpose is a dead giveaway of some ~total noob~ totally untrained developer. Our industry is long past the point where noobs are the problem. It's incompetent untrained journeymen with years of experience that are the problem. Noobs are just the shit-based icing on that ever-taller shit-filled cake.

  • Anon (unregistered)

    interpolation -> inference

  • Scott (unregistered)

    I could see this method being valuable if there was some checking--i.e., if index > length of array or < 0, return a new object (or whatever is appropriate for the system).

    But mostly it seems like solving a problem that doesn't exist.

  • (nodebb)

    There's also the redundancy in the function name. making it much longer than necessary. Perhaps the developer though they might also need to write GetResearchControlFromListOfSomeOtherKindOfObject

  • MiserableOldGit (unregistered)

    Didn't visual studio used to have a class builder that automatically vomitted out stuff like this, complete with redundant naming?

  • (nodebb)

    I'm inclined to be charitable on this one and allow that this could have been the result of a refactor - perhaps the original method was much more complicated for some reason (maybe different parameters or needed to access the list somewhere). Refactoring was done and the result is...this. Which is still a WTF, but I've definitely seen methods left in place instead of deleting it and changing to the new usage everywhere before.

  • (nodebb) in reply to Niroht

    I'm inclined to be charitable on this one

    I'm not. I see variables initialized at declaration and then immediately replaced pretty much every day. A three line method that replaces one line of code is also a regular thing in my life. In my experience, the developer will either come up with some sort of lame post-hoc explanation or a defense that the code "doesn't cause any harm". Only one in a thousand times will someone say something like "oh crap, I'll get rid of that right away".

    Recently, a variant of this submitted to me was:

    if (list.Count != 0) { foreach(var item in list) { // Stuff } }

    The developer had no shame about the inclusion of the if.

  • Your Mammas name (unregistered) in reply to Jaime

    Constructions where you test first used to be commonplace, doing that gets you a little bit more speed in certain circumstances.

  • (nodebb) in reply to Your Mammas name 
    Constructions where you test first used to be commonplace, doing that gets you a little bit more speed in certain circumstances.

    Yes if most calls to that code is with an empty list, then (depending on language and the implementation of the list object and count and foreach operators), then yes, checking for empty may be slightly faster I think, but in most cases it will slow things down....

  • Klimax (unregistered) in reply to Jaime

    That last one is not WTF on DotNet Core as foreach will throw exception if collection is empty.

  • Wizofaus (unregistered)

    I believe the term is type inference, I'm not sure "interpolation" quite fits.

  • Sole Purpose Of Visit (unregistered)

    Or you could just reformulate as:

    private ResearchControl GetResearchControlFromListOfResearchControls(int theIndex, 
    List<ResearchControl> researchControls) => researchControls[theIndex];

    Identical semantics. Saves the jitter doing what you want the jitter to do. Since the second parameter is a list, you might as well redefine it as an interface.

    Nothing to see here, ma'am. Move on.

    Though, as is obvious from my reformulation, you might just as well dereference the list in the first place. It's stupid, sure, but luckily the idiot who wrote it will be saved by compiler optimisations.

    Whether or not this is a benefit can be left to the reader.

  • (nodebb)

    I don't know if anyone has said this yet, but ResearchControl could have been a base class with a default implementation. The contents of the list may have contained sub-classes. In which case, the original premise would be sort of valid, except would still failed to work, as nothing caught the IndexOutOfRangeException.

  • (nodebb) in reply to Klimax

    That last one is not WTF on DotNet Core as foreach will throw exception if collection is empty

    No, foreach will throw an exception if the collection is null, but not if the collection is empty. The stupid code I posted will also throw an exception if the collection is null, so it remains pointless.

  • Ruts (unregistered)

    I like typed languages, and I like to be able to see what an object type may be without depending on an IDE to do it for me. Hence I don't really agree with the sentiment "declaring the type for every variable is a pain"

    How much of a pain, really? Some IDEs will use autocomplete to help fill out the name too, not too many characters to type, is it.

    I find it can be a bigger pain when just viewing source code in a text editor and a 'var' object is declared, getting it's type from a function return, so I may then need to find the function definition to know what type it is. To me, that is more of a pain and usually crops up when investigating something under time pressure, whereas writing code in an IDE doesn't really affect time to develop the solution.

  • MiserableOldGit (unregistered) in reply to Klimax
    That last one is not WTF on DotNet Core as foreach will throw exception if collection is empty.

    No, only if the collection hasn't been initialised. Although I am sure I've worked with something that behaved the way you describe ... early VBA? that language had enough builtin WTFs to make the Dalai Llama kick an orphan.

  • (nodebb)

    But if this started life in C++ and the ResearchContro being copied l to had a strange operator '=' defined that actually said "copy the contents of the thing on the right hand side into this thing", then you might have been forced to construct a ResearchControl first to apply the operator '=' to. And then you return it, so it still exists on the heap avoiding its sudden demise in random-pointer-to-stack land.

    Addendum 2020-07-24 07:55: So in some respects, this looks strange but may have had a valid reason to be implemented that way. And it just failed to be migrated to the latest trendy language in a sensible way.

  • Klimax (unregistered) in reply to MiserableOldGit

    Well, looks like I misremembered it. Well, I am by default C++ dev not C#, the only reason I had to venture there is writing special plugin for an eshop.

  • (nodebb)

    Well, looks like I misremembered it.

    ... and these are the round and round conversations I have with developers who write this kind of code.

    If a foreach behaves any way other than not executing its body for a valid but empty collection, then the language is broken. Putting guard code around it is not the right solution. If C# really had this behavior and I really had to live with it, I would write my own solution, probably in the form of a linq extension method similar to the ForEach that is currently in the framework.

    Remember, "it works" is not the same as "it's correct". Code reviews are for fixing the things that testing can't.

  • (nodebb) in reply to Blubb

    public static Frist GetFirstFrist(this IEnumerable<object> badIdea) => badIdea.OfType<Frist>().FirstOrDefault();

  • Chris (unregistered) in reply to mike_james

    I had a thought that maybe a C++ programmer was working in C#, and confused by all the reference types and "reference-by-values". They may have figured out that accessing the list directly makes you access the original object, and didn't want that. So they did this, in the hopes that you would end up with a copy. In which case, they failed, but I'd also wager that they didn't need a copy anyway. I have a colleague who is always making copies of objects being passed around needlessly "just in case someone changes something". The same type of programmer who just makes all member variables / fields public.

  • MiserableOldGit (unregistered) in reply to Jaime
    Remember, "it works" is not the same as "it's correct". Code reviews are for fixing the things that testing can't.

    I don't see a negative consequence beyond a wasted processor cycle or two, and in reality the optimiser should squash it anyway.

    The real thing is it is an indication that someone doesn't quite understand how things work, or perhaps isn't paying enough attention. I've seen far worse from people who claim to be egg-spurts!

  • yrebrac (github)

    I agree, it's a beginner error. I did this at least once in my first few days of C#, although I hope not since :)

  • Perri Nelson (unregistered) in reply to Remcoder

    While the code in this method is poor (creating and discarding an object to keep the garbage collector busy, failing to guard against index out of range, the poor choice of name, and so on), creating a method for this purpose, or even lots of methods that mask what seem like simple low-level operations is not a WTF and has nothing to do with being "enterprisey."

    Unless you are worried about every possible clock cycle, (and if you are, why not write in assembly instead of C#?), method calls are relatively inexpensive, and having reasonably well named functions allows you to keep all of the code in a higher level function at the same level of abstraction. Not that this function is well named.

  • (nodebb)

    My first thought is wondering if there's a memory leak. If ResearchControl extends System.Windows.Forms.Control, which is disposable, the redundant constructions are creating UI controls that are never disposed.

    Addendum 2023-12-25 21:18: Although I suppose the garbage collector would handle this case periodically as there's zero references to the controls.

Leave a comment on “A Step too Var”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article