31
The Yandere Dev/Pirate Software dilemma (media.piefed.social)
submitted 5 months ago by to c/godot@programming.dev
12 comments fedilink hide all child comments
 

UPDATE: The consensus seems to be overwhelmingly in favour of the match variant. And not to worry, I have replaced the magic numbers with an enum. Will try to remember to merge the branch tomorrow

Does an if-statement block or a switch statement fit better here? For context (and advertisement), this is part of my all-purpose utility plugin ( Codeberg link)

The code:

		# Method 1 (Yandere Dev Technique)  
		if self.throw_errors and status==MpupTest.TESTSTATUS.ERROR:  
			push_error(result)  
		if self.throw_warnings and status==MpupTest.TESTSTATUS.WARNING:  
			push_warning(result)  
		
		# Method 2 (Pirate Software Technique)  
		match status:  
			MpupTest.TESTSTATUS.ERROR:  
				if self.throw_errors:  
					push_error(result)  
			MpupTest.TESTSTATUS.WARNING:  
				if self.throw_warnings:  
					push_warning(result)
a code screenshot (code in description)

all 12 comments

sorted by: hot top controversial new old
[–] 14 points 5 months ago

The match statement is cleaner.

With the sequential if statements, it might seem on a casual read that both if statements could pass - you'd catch both warnings and errors. Only by looking at the conditions do you realize that only up-to-1 of them will run.

With the match statement, it's very immediately obvious that only one of the commands will run.

  • permalink
  • fedilink
  • source
    •
    [–] 9 points 5 months ago* (1 child)

    If status is modified in another thread you could have a race condition with method 1 because you're checking it twice. The two methods are not exactly the same code due to that.

  • permalink
  • fedilink
  • source
  • hideshow 1 child comment
    •
    • [–] 7 points 5 months ago (1 child)

    Not sure about how to do this with Godot, but a common pattern for this in other languages is creating something like a 32 bit value where the lower bits are the enum and the upper bits are the status code (assuming they are <=16bit each) and then doing a switch on the resulting packed value.

  • permalink
  • fedilink
  • source
  • hideshow 1 child comment
    •
  • [–] 5 points 5 months ago*

    Both these devs are far too full of themselves to think of using an enum table or anything clever with datatypes themselves.

    Because that would mean the way they've been doing things was potentially grossly inefficient at runtime... and they've been doing that for years...

    But a narcissist never makes a mistake, so instead, here's a bunch of reasons why actually writing optimized code is stupid, first among them being 'well it didn't occur to me, therefore doing it this way would reduce code readability for this code base that only I and no else needs to read'.

    I am guessing the concept of a sentinel value or an overloadable function would make their minds implode.

    But anyway, 'switch' is 'match' in gdscript:

    https://docs.godotengine.org/en/latest/tutorials/scripting/gdscript/gdscript_basics.html#match

    match <test value>:
	<pattern(s)>:
		<block>
	<pattern(s)> when <pattern guard>:
		<block>
	<...>
  • permalink
  • fedilink
  • source
  • parent
    • [–] 3 points 5 months ago

    I'd personally lean towards the switch pattern

  • permalink
  • fedilink
  • source
    •
    [–] 4 points 5 months ago* (1 child)

    Are 1 and 2 magic values in this case?
    You could use consts or even use an enum to further improve readability:

    const STATUS_ERROR   := 1
const STATUS_WARNING := 2
# or alternatively:
enum Status { FINE, ERROR, WARNING }

    In the latter case, you'd change status to be the new enum type Status.

    edit: Wait, nevermind. I think you're already using enums in the actual code (not screenshot).

  • permalink
  • fedilink
  • source
  • hideshow 1 child comment
    •
    • [–] 4 points 5 months ago

    i am rust dev so I pretty much just use match all the time

  • permalink
  • fedilink
  • source
    •
    [–] 2 points 5 months ago (1 child)

    Naively: Isn’t the second one slightly better because the first one will always check both variables twice but the second one only checks one of them once then the other variable once when inside?

  • permalink
  • fedilink
  • source
  • hideshow 1 child comment
    •
  • [–] [S] 3 points 5 months ago

    You're right, it is slightly better. Not really a concern here since this code runs only once per run for each unit test (and only for debugging, it's not meant to be in the game release) so it shouldn't be a performance issue, but still.

  • permalink
  • fedilink
  • source
  • parent
    • [–] 1 point 5 months ago

    I would be tempted to rename "result" to something more descriptive.

  • permalink
  • fedilink
  • source
    •