Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›3 »

dwazegek: Feb 11, 2005; WE CAN USE THIS

CPColin posted:

And the classic "set a variable to a value and then immediately set it to something else" move. I'm surprised he doesn't set it back to null in the finally block with a "// GC" comment.

In this case the first null assignment is necessary, otherwise it wouldn't even compile.

The code is also missing a null check in the finally block, which will make debugging all sorts of fun if the factory ever throws. This shouldn't even pass code review, use a goddamn using block, that's what it's for.

# ¿ Apr 20, 2017 18:13

Adbot: ADBOT LOVES YOU

# ¿ May 14, 2024 12:41

dwazegek: Feb 11, 2005; WE CAN USE THIS

Eggnogium posted:

Actually that lurking NullReferenceException is why I think it would just be cleaner to just have DatabaseContextWrapper unitOfWork = databaseContextFactory.GetUnitOfWork() right before the try.

Not entirely. This way somewhat forces all usage of the unitOfWork to be in the try scope, which means that it'll always be disposed. With your method someone could accidentally put a call between the assignment and the try scope, which seemingly would work, until it throws, and unitOfWork doesn't get disposed.

There are also a couple of cases where the runtime can insert an exception where you wouldn't expect it. In your case, if that happens after assignment, but before entering the try scope, then unitOfWork wouldn't be disposed. ThreadAbortExeption is the only one that I can think of at the moment, but I'm pretty sure there are a few others.

All of which comes back around to the original point, just use a using block, that would generate code that handles all issues correctly, without the possibility of loving it up, is also far more idiomatic. It also has the added benefit of not allowing unitOfWork to be reassigned again in the try scope, which is an easy way to otherwise miss disposing a disposable resource.

# ¿ Apr 20, 2017 18:45

dwazegek: Feb 11, 2005; WE CAN USE THIS

Munkeymon posted:

It wouldn't be that big a mystery to debug because that's all the finally block exists to do and it'd be obvious of something weird was happening like an assignment to the UOW inside the try block, so I'm not super offended by the lack of a null check. I am offended that he didn't bother to learn and use new language features. At least he's not afraid of LINQ as far as I can tell.

Until it makes its way into production, and the original exception is swallowed, leaving you with a log entry relating to the null ref exception that doesn't tell you anything of what actually went wrong.

While I can't be certain what happened here, I have seen similar things with colleagues of mine. Compiler complains about unassigned variable usage? Just initialize it as null, problem solved. At no point do they realize that they just turned a compile time error into a runtime error. Also, if the variable was unassigned in certain code paths before the null assignment, then it should be pretty obvious that the variable is now null in certain code paths, so you probably want a null check.

Your example is definitely something that should be caught in review though. Even if he'd recreated a using block's behavior perfectly, it still wouldn't be acceptable to write that piece of code that way.

# ¿ Apr 20, 2017 18:57

dwazegek: Feb 11, 2005; WE CAN USE THIS

LOOK I AM A TURTLE posted:

To be fair, I don't believe even a using block will protect you against this. Using -- like foreach, await, lock, and many other keywords --is entirely a C# feature, and all it really does is internally rewrite your code to a more verbose thing that you could've written out manually. And the assignment will not fall within the try-block generated by the using, as mentioned here: https://msdn.microsoft.com/en-us/library/yh598w02.aspx#Anchor_1. So even with using you're potentially susceptible to untimely ThreadAbortExceptions.

With that said, Always Be Using Using.

Huh, I was certain that it generated the assignment in the try block. But after looking at the generated IL, I must be mistaken, it does indeed do as you said. Well, at least I learned something new

# ¿ Apr 20, 2017 19:10

dwazegek: Feb 11, 2005; WE CAN USE THIS

What's causing that? Is it because the empty string is treated as null, and all comparisons to null are treated as false? That's backwards as gently caress :psyduck:

# ¿ Apr 28, 2017 15:55

dwazegek: Feb 11, 2005; WE CAN USE THIS

VikingofRock posted:

So I might be having a brainfart, but why would (x != '') be 0 here? Shouldn't that be true for every x, including x = NULL?

Every comparison with null results in false (or, more accurately, UNKNOWN, which ultimately results in false).

This include equality, inequality, greater/lesser then, etc.

So if b is null, then a = b results in false, but a != b also results in false, which is kinda hosed up as a is not b, but a is also not not b.

It makes more sense if you consider it as unknown. B is null (so its value is unknown), so it's also unknown if a = b, or if a != b.

# ¿ Apr 28, 2017 21:20

dwazegek: Feb 11, 2005; WE CAN USE THIS

CPColin posted:

code:

    DateWork = string.Format("{0:yyyy-MM-dd}", FromDate);
    FromDate = DateTime.Parse(DateWork);
    DateWork = string.Format("{0:yyyy-MM-dd}", ToDate);
    ToDate = DateTime.Parse(DateWork);

What's the context for this?

The datetime-to-string-to-datetime is real dumb, but I can forgive someone for not knowing about the Date property and coming up with an alternative (although this might break based on culture).

Going by .NET naming conventions, FromDate, ToDate and DateWork are (public) properties, which seems to point to far more horrors.

# ¿ Sep 29, 2017 17:16

dwazegek: Feb 11, 2005; WE CAN USE THIS

iospace posted:

Here's the actual code, for what it's worth (didn't have it on hand when I posted about it last night):

It's polling a 4x4 keypad.

What is this even doing? Maybe I'm missing something, but how does the column value ever get set to anything sensible?

# ¿ Oct 4, 2017 23:43

dwazegek: Feb 11, 2005; WE CAN USE THIS

iospace posted:

What it's doing is polling a 4x4 push button array. If I remember right (no data sheet/schematic in front of me atm, so it may be reversed), a 5v signal is sent down the columns of the buttons, and push buttons will complete the circuit on the corresponding row. So the upper four bits of PORTA is held high, then when one of the lower four bits goes high, check to see what column it really is on based on the row that went high. That program is polling constantly to check for button presses.

Sorry, I'm still unsure as to how it works.

From what I can tell the high nibble of PORTA represents the row, and the low nibble the column of the pressed button. So, presumably, if no button is pressed PORTA == 0x00, and if a button is pressed then exactly 1 bit of each nibble is 1.

I tried to reason about the code if PORTA was a variable (that might be accessed by different threads), and, as far as I can tell, then column would either always be 0, or essentially random.

But seeing as PORTA isn't a regular variable, I don't really know what to make of it. Does it work something like this?

code:

         PORTA &= 0xF0;                   //CLEAR COLUMN
         PORTA |= 0x01;                   //COLUMN 0 SET HIGH
         row = PORTA & 0xF0;              //READ ROWS
         if(row != 0x00){                 //KEY IS IN COLUMN 0
            column = 0;
            break;                        //BREAK OUT OF while(1)
         }

At first the low nibble is cleared and then set to the column to test.
If it's the wrong column, then (somehow) PORTA is set to 0x00, or only the high nibble is set to 0.
If it's the right column, then it retains its value.
By reading and testing the high nibble it's determined whether or not the column is the right one.

Either way, all of this seems really weird to me.
Would it be possible (and a lot easier) to test for a keypress, and if one is detected to just read the values of the high and low nibble from PORTA, map the values 0x01, 0x02, 0x04 and 0x08 to 0, 1, 2 and 3 respectively, and then you'd know the row and column?

# ¿ Oct 5, 2017 00:57

dwazegek: Feb 11, 2005; WE CAN USE THIS

CPColin posted:

This is weird, right? It's not just because I don't know C# very well?

It has a bunch of unnecessary checks, as well as a possible localization bug. So, yeah, it's definitely not great.

Why 1 jan 1900 wouldn't be valid, but a few nanoseconds later would be is also strange.

A more succinct way would be

code:


public static void IsValidDate(DateTime? date) 
{
    return date > new DateTime(1900, 1, 1) && date != DateTime.MaxValue;
}

# ¿ Oct 16, 2017 21:45

dwazegek: Feb 11, 2005; WE CAN USE THIS

Linear Zoetrope posted:

If I had to guess, the
code:
catch {
    throw;
}
Is meant to obliterate largely meaningless stack trace information from sections of code out of your own control? I have no idea if it's a common pattern though, I don't use C#.

No, then you'd have to do

code:

catch(Exception ex) {
    throw ex;
}

A bare throw is only valid in a catch scope, and rethrows while maintaining the stacktrace.

The catch/rethrow here is most likely to have a place to attch a debugger.

# ¿ Oct 16, 2017 21:49

dwazegek: Feb 11, 2005; WE CAN USE THIS

biznatchio posted:

Plus, the stack trace changing behavior masking the true exception source is annoying as poo poo in cases where you actually want to do real error handling before rethrowing an exception.

You could do this

code:


catch (Exception ex) when (Handle(ex))
{
    throw; // this line never gets executed
}

bool Handle(Exception ex) 
{
    //handling goes here
    return false;
}

Yes, it's really ugly, but afaik it shouldn't mess up the line numbers.

# ¿ Oct 17, 2017 17:28

dwazegek: Feb 11, 2005; WE CAN USE THIS

Hammerite posted:

c# will complain if a path through a non-void method exists that reaches the end without returning/throwing/etc. Sometimes it gets things wrong - for example, this doesn't compile:
code:
// fails compilation with CS0161 "not all code paths return a value"
static bool X (bool y)
{
    switch (y)
    {
        case true: return true;
        case false: return false;
    }
}
but I think that's still better than the opposite failure mode of allowing something through that shouldn't be allowed.

code:

public static void DoStuff()
{
    switch (Blah())
    {
        case false:
            Console.WriteLine("false");
            break;
        case true:
            Console.WriteLine("true");
            break;
        default:
            Console.WriteLine("huh?");
            break;

    }
}

private static unsafe bool Blah()
{
    byte b = 2;
    void* bPtr = &b;
    return *((bool*)bPtr);
}

A coding horror? Perhaps. As far as I know this issue only crops up when switching on bools though. So just never do that.

# ¿ Mar 5, 2018 21:23

dwazegek: Feb 11, 2005; WE CAN USE THIS

NihilCredo posted:

No need to do that, if the list is empty .All() will immediately return true anyway

If you're attempting to mimic the behavior of Distinct().Count() == 1, then you'd want it to return false on an empty list.

That aside, the First call should probably be brought out of the predicate, and if you want the code to be suited for any IEnumerable<int>, then it'd have to be written completely differently.

# ¿ Mar 8, 2018 18:42

dwazegek: Feb 11, 2005; WE CAN USE THIS

hackbunny posted:

Tons, honestly. C# forces you to use try/finally or IDisposable, both of which you have to use explicitly, while in C++ you just write a destructor and let stuff go out of scope. The only convenient C# constructs for RAII are using and synchronized, and they are both function scope only and hardly universally applicable

But that's a few lines extra here and there, a far cry from the original poster's claims, and also doesn't really involve copy/pasted code. For stuff like using it's the exact same number of lines of actual code.

There are also cases where C# ends up having less lines of code, and, depending on context those are arguably more common.

Mooey Cow posted:

And all this "syntactic sugar" that expands to a multi-kb class that'll be instantiated every time a function is called. Ugh.

This is something I'd like to see an example of. Code that instantiates a class every time it's called is common, but a "multi-kb" class seems to be an enormous exaggeration.

# ¿ Jun 15, 2018 01:39

dwazegek: Feb 11, 2005; WE CAN USE THIS

Mooey Cow posted:

For instance, every local variable you use in an iterator function will/can be made into a member variable in a hidden class so it can preserve state between yields. I believe async/await does something similar. If you're careless or don't know about this, causing enormous allocations from what looks like fairly innocuous stuff is very easy.

While it's true that variables can be captured in a generated type, you'd also have to be capturing hundreds of variables before the class ends up in multi-kb territory. Which means that you have hundreds of variables in method scope to capture in the first place, which seems to be the real problem.

Also some of the genetrated types are structs, so instead of a variable on the stack, you end up with a generated struct on the stack with the variable in it, so there's not that much difference.

Your point about templates is true though. There is some tooling out there to generate code for that sort of stuff, but that's kind of a poo poo solution compared to templates.

However, I do wonder if C# is the right tool for your job if you frequently run into that sort of issue. Math heavy stuff has never really been one of C#'s strengths.

# ¿ Jun 15, 2018 17:47

dwazegek: Feb 11, 2005; WE CAN USE THIS

One of our applications puts items in a database table for later processing, so basically a queue. A separate scheduled task then calls a wcf service to retrieve the items, batched per 7 items (why 7? who the gently caress knows). After processing an item the task makes another call to the same wcf service to remove the item from the queue. Relatively straightforward stuff.

The stupid thing is that the id for the item is a guid. Which is stored as a char(38) in the db. And is exposed as a string by the wcf service. But the call to dequeue the item takes a guid, so the client has to parse the string it got from the server to a guid in order to pass it back to the same server. Which in turn obviously turns it back to a string when it goes to the db :downs:

# ¿ Jun 15, 2018 19:46

dwazegek: Feb 11, 2005; WE CAN USE THIS

Hammerite posted:

Who cares? unless it's actually the bottleneck in your application, which doesn't sound likely

You don't think that it's an issue that a web service exposes an id as a string, but requires the client to post that same id back as a guid? Or that the id could have just been a guid throughout the application, which would have resulted in better performance, and, more importantly, less potential for bugs? Sure, it's not the worst coding horror in the world, but still...

# ¿ Jun 15, 2018 21:27

dwazegek: Feb 11, 2005; WE CAN USE THIS

Minimum password lengths reduce the total number of possible passwords, but people who are knowledgeable about passwords won't use short passwords anyway, so you're not reducing security by enforcing minimum lengths.

Mixing different character types is also generally a good thing, and people who are knowledgeable are going to do that anyway, so again, no reduction in security.

While it would be better if everyone in the world had a solid understanding of how to use passwords, that's also not reasonable to expect, so we use password rules to help those people.

It only becomes an issue when the people who come up with the rules make bad decisions that prevent good passwords from being chosen, but the solution to that problem isn't to abolish the rules entirely.

# ¿ Jun 20, 2018 06:42

dwazegek: Feb 11, 2005; WE CAN USE THIS

You don't even need users to get bad user inut, sometimes your browser does it for you: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/17358578/
:stare:

# ¿ Jun 21, 2018 17:14

dwazegek: Feb 11, 2005; WE CAN USE THIS

Should have used a bool array of 50k items so you can do an O(1) index operation instead of an O(n) indexOf.

# ¿ Jun 28, 2018 18:48

dwazegek: Feb 11, 2005; WE CAN USE THIS

isShouldOuputExtraFieldsFlagSetToTrue

# ¿ Jul 16, 2018 18:21

dwazegek: Feb 11, 2005; WE CAN USE THIS

rjmccall posted:

Oh, does C# not directly support rectangular allocation? I guess I'd forgotten that it has multi-dimensional arrays for that case.

Putting the bounds in the right position is still a good idea to reserve room to extend the language to do rectangular allocation.

It does, new int[3, 5] is perfectly valid.

Edit: that said, rectangular allocation of jagged arrays is not supported.

# ¿ Sep 15, 2018 08:39

dwazegek: Feb 11, 2005; WE CAN USE THIS

In the Netherlands, the top result is the Dutch suicide prevention site (which is a completely different url). So this seems to be a serious problem.

# ¿ Jan 28, 2019 09:25

dwazegek: Feb 11, 2005; WE CAN USE THIS

New Yorp New Yorp posted:

Almost agree; they should accept IList if they need list behavior (indexers, adding/removing elements) or IEnumerable if they just need to be able to enumerate the members of the collection.

ICollection, IReadOnlyCollection and IReadOnlyList are also really useful. I generally even prefer IReadOnlyCollection over IEnumerable as it sidesteps a number of gotchas that IEnumerable has.

# ¿ Mar 5, 2019 19:15

dwazegek: Feb 11, 2005; WE CAN USE THIS

Linear Zoetrope posted:

This also is almost always foreseeable, or at least with experience you usually know when it might be the case in the future and can be defensive about it. Other than that? Just use a public field.

In C# there's really no advantage to using public fields over properties, while there are a number of disadvantages*. You should always use public properties over fields, unless there is some explicit reason to use a field.

*Fields cannot be part of an interface, properties allow for different access modifiers on get/set, properties can be computed, changing a field is always a breaking API change, just to name a few.

# ¿ May 31, 2019 11:10

dwazegek: Feb 11, 2005; WE CAN USE THIS

CapnAndy posted:

Oh, c'mon. It is an entire file, with its own custom namespace, with a custom class you have to instantiate, with carefully laid out regions, all to store a single string that you could just declare literally anywhere if you needed it. I've never seen something so idiotically beautiful.

There's plenty of stupid stuff in that class: the regions, not using auto-properties, the pointless constructor declaration, but there's absolutely no inherent horror in wrapping a string in a custom type.

Maybe the way it's used makes it stupid, but that's not demonstrated from that example.

# ¿ May 31, 2019 20:38

dwazegek: Feb 11, 2005; WE CAN USE THIS

The boilerplate stuff is excessive and pointless, but, if anything, the class doesn't take the idea far enough. If it were immutable with some basic input validation in the constructor, then it'd be a definite improvement over just passing strings around.

So maybe the real horror is that they half-assed it :shrug:

# ¿ May 31, 2019 21:00

dwazegek: Feb 11, 2005; WE CAN USE THIS

I kinda feel that DI singletons should have been called something else, as they (at least in my experience) have very little to do with the actual singleton design pattern.

In all the codebases I've worked on, almost all the stuff that gets registered as a singleton for DI is just a stateless class, that could be instantiated an arbitrary number of times, but there really is no need to ever have more than one. Stuff like validators, mappers, factories and the like. They could be given a scoped or transient lifestyle, and the application would work just fine. The only reason they are registered as singleton is to reduce some unnecessary overhead and so that they can be injected into other singletons.

Personally I've never encountered a use case for a "real" singleton. I've seen scenarios where you only need or want one instance, but never a scenario where literally only one instance should exist.

NihilCredo posted:

Yeah, a DI framework is just an indirection layer that sacrifices compile-time safety in exchange for reducing manual DI boilerplate. That's a tradeoff to be weighed very, very carefully.

Certain DI frameworks (like SimpleInjector) have a strict verification system, which at least makes it possible to perform unit tests that ensure that the application's configuration is correct, which makes weighing the tradeoff easier. Granted, it's not compile-time safety, but it can still be done as part of the build process.

# ¿ Apr 26, 2020 12:11

dwazegek: Feb 11, 2005; WE CAN USE THIS

Xarn posted:

https://docs.microsoft.com/en-us/dotnet/api/system.dayofweek?view=net-5.0

(Also lol at listing the fields in alphabetical format)

That does not mean that the first day of the week is hardcoded to be Sunday. It just means that Sunday is hardcoded to be the first field in the enum. As Hammerite mentioned, the culture defines what the first day of the week is, not the enum. Given that they chose to use an enum, this is literally the only way they could have done it (other than starting the enum at a different day, of course).

All .NET enums are always convertible to some integer type, so there's no way to prevent people from converting it to a number. You could opt to not explicitly set the backing value in the enum, but then .NET will just implicitly give the enum fields a value.

Jabor's suggestion is really the only thing they could have done to make it harder for people to do the wrong thing, but even that wouldn't have prevented it. I guess they could've also opted to give the fields random values that change between every .NET release, but that would probably have caused MS more issues that it would ever solve.

Since the addition of extension methods, they could've also added a culture sensitive extension method that returns the "number" of the day of the week, which would've been nice. Granted if someone needs that, it's quite easy to just implement.

# ¿ Feb 10, 2021 16:51

dwazegek: Feb 11, 2005; WE CAN USE THIS

Absurd Alhazred posted:

C++ lets you declare enum class which forces you to actively cast to an integer to do any arithmetic on it, which helps.

It's the same in C#, you can't preform arithmetic on an enum directly, you'd have to cast it first. And, iirc, in order to do that, you need to know which integer type is the backing type, e.g. if you cast an enum to a short while it's backed by an int, you get a runtime exception.

# ¿ Feb 10, 2021 16:54

dwazegek: Feb 11, 2005; WE CAN USE THIS

ultrafilter posted:

Does C# let you do order comparisons on enums without casting them? If so, that's a big part of the problem.

Yes, it does. As you'd expect it just compares the backing values.

# ¿ Feb 10, 2021 17:03

dwazegek: Feb 11, 2005; WE CAN USE THIS

Munkeymon posted:

Huh, my bad - could have sworn that worked but you can do
code:
void Main() {
	E e = (E)2;
	f(e).Dump();
}

enum E {
	A,
}

string f(E e){
	switch(e){
		case E.A: return "A";
	}
	return string.Empty;
}
and the compiler is fine with it, so that's probably why. Presumably so that a code could gracefully handle deserializing an enum from a newer version with more enum values.

Except that that's really not any different from these earlier examples that Hammerite posted (aside from using a return rather than a throw):

code:

public static string SomeString(this MyEnum e)
{
    switch (e)
    {
        case MyEnum.X: return "A";
        default: throw new Exception();
    }
}

public static string SomeString(this MyEnum e)
{
    switch (e)
    {
        case MyEnum.X: return "A";
        default: break;
    }
    throw new Exception();
}

public static string SomeString(this MyEnum e)
{
    switch (e)
    {
        case MyEnum.X: return "A";
    }
    throw new Exception();
}

In C# all enums are always backed by an integer type, the default being Int32, so any values that the integer type can represent, are also possible for the enum type. So, unless you're covering all 4 billion possibilities, the switch statement isn't exhaustive, and thus needs a default case, or the code will just fall through the switch, and you need a return/throw after it.

Even if you use an exhaustive pattern matching switch, the compiler rarely seems to handle that specific case, because it hardly ever comes up. E.g. this also doesn't compile:

code:

	public static string Blah(int i)
	{
		switch (i)
		{
			case int n when n < 0: return "";
			case int n when n >= 0: return "";
		}
	}

Even the newer switch expressions, which do let you omit the default case, simply auto-generates one for you if you do so.

Stupid side note, switching on a boolean has changed sometime between the last .NET Framework release and .NET5. In .NET5 (and possible older core versions as well), this compiles:

code:

	public static string Blah(bool b)
	{
		switch (b)
		{
			case true: return "T";
			case false: return "F";
		}
	}

In .NET Framework it doesn't. This used to be a weird edge case, because the switch statement would only match the "true" case if the boolean actually had binary value 0x01, so if you marshalled a non 0x00/0x01 value to a boolean, it wouldn't match any case statements. Which also means that the switch statement is not exhaustive, as it only covers 2 out of the possible 256 values.

I guess they finally decided to fix this weird issue in core at some point. Granted, it was never much of an issue, as there's little to no reason to switch on a boolean.

dwazegek fucked around with this message at 22:05 on Feb 12, 2021

# ¿ Feb 12, 2021 22:03

dwazegek: Feb 11, 2005; WE CAN USE THIS

Hammerite posted:

ArgumentException, NotSupportedException, NotImplementedException, InvalidOperationException all imply to client code that it's their fault. It's not. It's my fault - my code was meant to handle all enum members, and I hosed up.

If someone casts an integer value that doesn't exist in the enum, then it definitely is their fault, so an ArgumentOutOfRangeException is applicable.

I guess you could first check for Enum.IsDefined, and throw ArgumentOutOfRangeException in that case and a different exception in the default case of your switch. But even then you should just define your own exception type, and not throw Exception.

# ¿ Feb 12, 2021 22:12

dwazegek: Feb 11, 2005; WE CAN USE THIS

Xarn posted:

And if the safe parts of your language then have to safeguard against bool having 256 potential states/representations, you are doing it wrong.

It's only if you switch on a boolean, which no one does because why would you. I guess it could also still be an issue if you use unsafe code to read the bits of a boolean and only expect 0x00 or 0x01, but then you're really doing some stupid stuff.

Using it in as part of a boolean expression, or in an if/while/for/ternary/whatever statement, will work correctly. Even a direct comparison to another boolean works as it should (i.e. true == true, regardless of whatever bits the true value has). Also, as I mentioned, it seems to have been fixed in later compiler or language versions.

It really seems to have been a bug and not a flaw in the actual language.

# ¿ Feb 14, 2021 17:15

dwazegek: Feb 11, 2005; WE CAN USE THIS

You could define the derived types as nested types of the base class, that would make the derived types a bit more discoverable, although then you'd get name clashes with the properties.

Making the base class' ctor protected internal or even private protected would prevent types from being added outside the assembly. Still no guarantee that the switch statements are exhaustive, but with regular enums they aren't either...

Another downside is that, since it's a class, there is no compile time way to prevent null values. The newer non-nullable reference type stuff helps, but it offers no guarantees.

# ¿ Feb 20, 2021 17:34

dwazegek: Feb 11, 2005; WE CAN USE THIS

Ola posted:

You can do that with F# while you wait for C# to support it properly. It's so easy to refactor and maintain as well. And the match expressions are dreamy.
code:
match thirdPartyApiResult with
| Ok data -> Success (mySpecialParser data)
| Error e [ when e.StatusCode = 401 ] -> ErrorMessage "wrong password"
| Error e [ when e.StatusCode > 401 and e.StatusCode < 500 ] -> ErrorMessage "you hosed up"
| Error e [ when e.StatusCode >= 500] -> ErrorMessage "they hosed up"
| Error e  -> ErrorMessage "we hosed up"

AFAIK you can do all of this in C# 9, with a very similar syntax.

Edit: C# 9 isn't officially supported on anything other than Net5, but, unofficially, if you enable langversion latest you can use some features, including those patten matching features on earlier versions as well, including even older framework versions as far back as at least 4.7.2.

dwazegek fucked around with this message at 11:54 on Feb 21, 2021

# ¿ Feb 21, 2021 11:48

dwazegek: Feb 11, 2005; WE CAN USE THIS

Why not just link the relevant work items to the pr, instead of writing a novel in the description. :confused:

# ¿ Apr 28, 2021 12:29

dwazegek: Feb 11, 2005; WE CAN USE THIS

ExcessBLarg! posted:

I'm not following this Unity stuff. From what I gathered:

Unity overloads == so "== null" checks go down a slow path.

Devs shouldn't be doing null checks anyways because standard reference types can't be null (or at least, null is an error condition).

They might be doing null checks because it's a Nullable type, but they should be using Option types instead.
OK, but, if Unity is overloading ==, I'd expect the first two statements of the method to be (i) "is it null? return false" and (ii) "is it this? return true". Does Unity not do that because they think you shouldn't be doing null or identity checks in the first place, or that you should only use Object.ReferenceEquals? Or is the slow part even getting to the overloaded method in the first place?

Unity declares a type called `Object` and most classes in Unity derive from it. This is not the same as `System.Object`, and yes, reusing the name Object is unnecessarily confusing.

The problem with comparing Unity's object to null (aside from being slow), comes from the decision to return true when comparing a disposed object to null. In other words, if a comparison to null returns true, that means that either the compared object was actually null, or it wasn't null, but was disposed. This has always been an indefensible idea, but with more recent language additions it's become even dumber.

The null coalescing operator ( ?? ), null conditional operator ( ?. ), null pattern matching ( is null / is not null ), null cases in switch statements and nullable reference types all work based on strict null equality. So you can have an object that seemingly equates to null in Unity, but then get completely inconsistent results from any of those operators. This causes even more issues, because tools like Resharper will assume that no one would be so dumb as to return true when comparing a non null to null, so it will suggest converting this:

code:

if (x != null)
{
  x.DoSomething();
}

to this:

code:

x?.DoSomething();

Which should be the same thing, but isn't in Unity's case.

From what I remember, Unity petitioned Microsoft for changes to c# so that their hosed up version of null-ness would work with these newer features. Microsoft's response was basically "lol, no".

To be clear, Unity's "equal to null when disposed" idea was a terrible idea, even before these newer language features existed. E.g. the result of a null check can change without reassigning the variable that's being checked, something that would otherwise be completely impossible:

code:

bool b1 = x == null; // false
x.Dispose();
bool b2 = x == null; // true;

There's a bunch of other issues, but you get the idea.

# ¿ May 6, 2022 22:08

Adbot: ADBOT LOVES YOU

# ¿ May 14, 2024 12:41

dwazegek: Feb 11, 2005; WE CAN USE THIS

pokeyman posted:

Wait, so Unity uses C# but is not able (allowed? willing?) to make any changes to the compiler or language or runtime? That's so weird. Fork it you cowards.

Going from memory here, but it wasn't so much that they wanted to fork the compiler or anything like that. They wanted the actual c# spec to be changed to allow for their weirdness.

That said, even if they created their own compiler, they'd then potentially have to recompile the entire .NET runtime and all 3rd party packages in order for it to actually work. Take something like this:

code:

public static void DoSomethingIfNotNull(IDoSomething x)
{
  x?.DoSomething();
}

If that method was compiled using any compiler that followed the c# spec, `DoSomething` would only be called on `x` if `x` is an actual, for real, null reference (i.e. object.ReferenceEquals(x, null) returns true). They could write their own compiler that would have that code work using their incorrect interpretation of null-ness. However if that method lives in an assembly they didn't compile, it would still do the correct thing, rather than what they want.

As a side note, if their goal was to simplify checks for valid objects, they could've just written an extension method on `UnityEngine.Object` that does both, e.g.

code:

public static bool IsValid(this UnityEngine.Object obj)
{
  return obj != null && !obj.IsDestroyed();
}

As far as I can tell, this would've side-stepped basically every major problem their equality implementation has.

# ¿ May 7, 2022 08:50

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›3 »