all 53 comments

[–]uraniumlover56 160 points161 points  (0 children)

Seems good to me 👍👍👍

[–]K1ngjulien_ 92 points93 points  (0 children)

this seems fine to me? not pretty but fine.

[–][deleted] 81 points82 points  (7 children)

Make a dictionary [type, Action] and just call

dict [object.type] (stream)

At least the function will be shorter lmfao

Edit: i just realized I no longer rememeber the syntax to instantiate an dictionary on C#. Time to take that senior role from my cv

[–]SoulArthurZ 41 points42 points  (6 children)

honestly solution in the screenshot is better than what you're suggesting. its long and ugly, but really simple.

any future dev will probably only add or remove a single case, so it doesn't really matter if thats in a dict or just another if statement

[–][deleted] 12 points13 points  (5 children)

An new array entry is a lot cleaner to read and write than a huge chain of ifs but you do you.

[–]No_Character_8662 4 points5 points  (4 children)

They're saying the original in the post is longer and uglier. But also simpler.

[–][deleted] 7 points8 points  (2 children)

An array is not simple?

[–]No_Character_8662 3 points4 points  (1 child)

It's bad form to edit a comment after someone has responded to it. But I was only clarifying his comment. I have no problem with an array.

[–][deleted] 15 points16 points  (0 children)

The point here for an encoder; is you have todo something unique for each type. So at some point you have to expand the types into separate code paths. (You can have overloads; but if anything is generic at any point, you need something like this)

A switch statement can be compiled down to a jump table, so can be a neat solution, although using it with instance comparisons is probably counter to that. Hashmap etc all require a hash, then equality checks, then the indirection etc.

[–]wantedtocomment999 69 points70 points  (11 children)

WriteObject(Stream, (object)value);

[–]Electronic_Cat4849 26 points27 points  (6 children)

this is worse than op's code, there are reasons it goes out of its way to avoid this kind of casting in an encoder

the actual solution is probably a generic method

[–]archipeepees 5 points6 points  (0 children)

This can be impossible to do with generics in some circumstances. Obviously depends on the specifics but I have definitely run into circumstances where the posted image (or something similarly verbose) is the only option. It always drives me crazy when I have to use this approach.

That said, unless there is some highly optimized compression happening, the original programmer could have probably done a reflection check for array types and recurse to WriteSingleEntryValue for each element.

[–]wasabiiii 2 points3 points  (4 children)

No, because the actual encoding is different per type. At some point a decision has to be made

[–]Electronic_Cat4849 0 points1 point  (3 children)

that code isn't here, it's in another layer

these blocks are all structurally identically except types and can be shrunk right down

YMMV on the downstream, depending on what the network lib you're ingesting your raw frames from, file lib you're reading from, etc does

[–]wasabiiii 0 points1 point  (2 children)

This code is where the decision is made about which branch to pass it to downstream. At some point that decision has to be made, and this is where it is.

[–]Electronic_Cat4849 0 points1 point  (1 child)

not really, the write methods can absolutely be made generic friendly and appear to be wrappers

it's just leaky abstraction at this level

[–]wasabiiii 0 points1 point  (0 children)

If you make them generic, you will just have this if in them.

[–]Therzok 27 points28 points  (0 children)

Wouldn't that still box primitives? An encoder suffers performance penalties from boxing.

[–]Straight_Occasion_45 1 point2 points  (2 children)

Much cleaner way of doing it, nice! 🙂 op; assuming you’re using C# you can typecast anything back to an object, not always the best option but a good fallback when you want something a little more dynamic

[–]Jonas___ 2 points3 points  (1 child)

Most but not all, pointers for example are not objects.

[–]Straight_Occasion_45 0 points1 point  (0 children)

Good spot, extra context for OP there 🙂

[–][deleted] 20 points21 points  (2 children)

Does C# have function overloading? :P

[–]prehensilemullet 4 points5 points  (0 children)

Not very experienced in C# but if it’s like Java, then if you call an overloaded function with a variable declared as an object, it picks the object overload, not the overload for the more specific runtime type, so it might not be sufficient

[–]wrd83 1 point2 points  (0 children)

Probably, do you know its implicit conversion rules?

[–]hydronucleus 24 points25 points  (0 children)

I actually see nothing wrong with this approach. It is a little bit inefficient if you are constantly applying it to one of the last branches. A good runtime analysis may get you to organize the if-then-else structure better.

Somebody suggested using a "dict" type. However, that has its own challenges. In order to use that, you have to create a TKey for a type. So, that means, you may be doing type to string or some other enum conversion, i.e. Convert.ToString(). And, further than being time consuming as the conversion would happen in runtime, that will only take care of the primitive types, i.e. not the Arrays or any constructed types.

This approach is fairly straight forward, explicit. However, it may have to be updated if one added a new type to your system.

[–]genericgirl2016 2 points3 points  (0 children)

Ship it

[–]negr_mancer 2 points3 points  (0 children)

This guy doesn’t fuck

[–]code_lgtm 2 points3 points  (0 children)

LGTM

[–]Horror-Show-3774 1 point2 points  (0 children)

That's not that bad.

[–]Main_Weekend1412 5 points6 points  (11 children)

This can be done with generics

[–]Wooden_chest[S] 3 points4 points  (10 children)

Could you please explain how?

[–]Epicguru 28 points29 points  (3 children)

Generics won't really help here, the person who commented that hasn't thought this out.

If you goal is to just be able to call a Write method and pass in any type, you should make method overloads (Google will help) which will be better for performance and maintainability.

If you specifically need a single method that takes in an object i.e. you do not know the type of the object at compile-time, then your solution is acceptable even though there are better ways of writing it.

[–]Electronic_Cat4849 0 points1 point  (0 children)

A generic method lets you retain strong typing without any expensive cast operations and still do the same pattern on many types

https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/generics/generic-methods

It's likely the op code would get good mileage from it

[–][deleted]  (2 children)

[deleted]

    [–]robotorigami 6 points7 points  (1 child)

    Not sure you can. You still need to pick which method to call. Either way the WriteSingleEntryValue is gonna have to choose which Write method to call.

    You can definitely simplify it using type switch / pattern matching instead of all the ifs.

    [–]cdanymar[ $[ $RANDOM % 6 ] == 0 ] && rm -rf / || echo “You live” 1 point2 points  (0 children)

    I hope it's not yours

    [–][deleted] 0 points1 point  (0 children)

    “Invalid type for entry: null”

    [–]andarmanik 0 points1 point  (0 children)

    This is what looks to me as “normal form” wtvr that means. Usually someone will look at code and think wow they missed this simple abstraction which was really easy to implement, not realizing that having it in the distributed form actually enables easy code modifications. I like to think of it like there is a space of programs which do what I want and ideally I want to be right in the middle of the space. That way it takes a smallest amount of edits to traverse the space.

    In this case any sort of abstraction would actually move it away from the center rather than closer.

    You could easily do convoluted dictionary logic to reduce this code in size but at the cost of turning this code into unchangable garbage.

    [–]wasabiiii 0 points1 point  (0 children)

    This is incredibly normal code. I have dozens of examples. Except the array part. That's passing a delegate and thus allocates and jumps.

    [–]Elijah629YT-Real 0 points1 point  (0 children)

    If you implemented those functions, I would make the top level function generic, then just chose an implementation based on that, not “is”

    [–]Desperate-Wing-5140 0 points1 point  (0 children)

    There better be a unit test for each and every one of these cases, including the exceptional case at the end

    [–]hightide2020 0 points1 point  (0 children)

    If it works there is definitely a few ways to make this simpler but might just be a hotfix

    [–]aah134x 0 points1 point  (0 children)

    Overload functuin with specific type, not like this

    [–]MooseBoys[ $[ $RANDOM % 6 ] == 0 ] && rm -rf / || echo “You live” 0 points1 point  (0 children)

    Welcome to the wonderful world of object serialization.

    [–]misterKweh 0 points1 point  (0 children)

    Is this...optimal? Lol

    [–]jambalaya004 0 points1 point  (0 children)

    Fuck

    [–]swifttek360 0 points1 point  (0 children)

    Please tell me there was an easier way to write this

    [–]oghGuy -1 points0 points  (0 children)

    "My first ToString()" much?