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!

«‹›1503 »

Cuntpunch: Oct 3, 2003; A monkey in a long line of kings

Subjunctive posted:

code:

function f(thing) {
    if (!thing.prop) {
        throw "need a prop";
    } else {
        var stuff = getStuff(thing);
        if (!stuff.id.isUnique()) {
            throw "need unique";
        } else {
            doItToStuff(stuff);
        }
    }
}

function f(thing) {
  if (!thing.prop) {
    throw "need a prop";
  }
  
  var stuff = getStuff(thing);
  if (!stuff.id.isUnique()) {
    throw "need unique";
  }

  doItToStuff(stuff);
}

The assumption you're making is that all coders indent the else:

code:

if (foo)
{

}
else
{

}
is also a pretty common pattern

Not to say it doesn't lead to confusing the complexity of lots of if-else nesting in C-likes

# ? May 11, 2016 16:36

Adbot: ADBOT LOVES YOU

# ? Jun 5, 2024 07:27

Dessert Rose: May 17, 2004; awoken in control of a lucid deep dream...

Cuntpunch posted:

The assumption you're making is that all coders indent the else:
code:
if (foo)
{

}
else
{

}
is also a pretty common pattern
Not to say it doesn't lead to confusing the complexity of lots of if-else nesting in C-likes

Your code style still runs into the same problem, there's no functional difference between using Allman style or whatever you call } else {.

Unless for some ungodly reason you don't indent your else block in which case you definitely belong in this thread.

# ? May 11, 2016 16:40

Subjunctive: Sep 12, 2006; ✨sparkle and shine✨

Cuntpunch posted:

The assumption you're making is that all coders indent the else:

No, I'm assuming that they indent the contents of the else, because they aren't utter savages.

# ? May 11, 2016 16:42

Hammerite: Mar 9, 2007; And you don't remember what I said here, either, but it was pompous and stupid.; Jade Ear Joe

I could snark and point out that you're throwing rather than returning, but I'll resist the temptation and accept that return and throw can be treated as indistinct for the purposes of this argument.

The problem with the code you exhibited isn't that you're using else after return (throw), rather it's that you're passing up the opportunity to use the "early return" idiom.

The check for thing.prop is an entry condition. You should early-return if it's violated, and no else is called for. In the case of the second if-else, the "if" and "else" blocks are comparable in length, so it doesn't resemble an entry condition. The if-else is called-for here because it draws attention to the dichotomy between the two cases.

So you should present it like this:

code:

function f(thing) {
  if (!thing.prop) {
    throw "need a prop";
  }
  
  var stuff = getStuff(thing);
  if (!stuff.id.isUnique()) {
    throw "need unique";
  } else {
    doItToStuff(stuff);
  }
}

# ? May 11, 2016 16:46

feedmegin: Jul 30, 2008

But what colour should we paint the bikeshed?

# ? May 11, 2016 16:59

Subjunctive: Sep 12, 2006; ✨sparkle and shine✨

Hammerite posted:

I could snark and point out that you're throwing rather than returning, but I'll resist the temptation and accept that return and throw can be treated as indistinct for the purposes of this argument.

The problem with the code you exhibited isn't that you're using else after return (throw), rather it's that you're passing up the opportunity to use the "early return" idiom.

The check for thing.prop is an entry condition. You should early-return if it's violated, and no else is called for. In the case of the second if-else, the "if" and "else" blocks are comparable in length, so it doesn't resemble an entry condition. The if-else is called-for here because it draws attention to the dichotomy between the two cases.

So you should present it like this:

How do you decide if you should put an else after a given conditional exit? Both of them are "signal error if a necessary condition isn't held". Having to compute some initial state and check it for preconditions is hardly uncommon. Having it be based on the number of lines in the then/else blocks sounds super fragile in the context of maintenance that could change the size of one of them. Now you need to reindent all the else content because you added or removed some statements in the then block.

E:

feedmegin posted:

But what colour should we paint the bikeshed?

Sorry, yes, this is a lovely derail. I'll bow out.

Subjunctive fucked around with this message at 17:11 on May 11, 2016

# ? May 11, 2016 17:03

Cuntpunch: Oct 3, 2003; A monkey in a long line of kings

I hate big else-if chaining, in general, and yeah you're correct up above: sorry, my brain is *burnt out* from having to do about 60% of the sprint work for a 5-developer team over the past few weeks.

With regards to fast-returns, I'm a much bigger fan and I think it tends to create easier-to-debug code - so long as the architecture itself is structured to support it:

From a WebAPI endpoint, for example:

code:

public IHttpActionResult Post(Foo param)
{
	if(param.value == "invalid")
	{
		return BadRequest("Derp");
	}
	if(param.value2 == "invalid")
	{
		return BadRequest("Hurf");
	}

	//Now do all your actual processing
}

# ? May 11, 2016 17:14

Hammerite: Mar 9, 2007; And you don't remember what I said here, either, but it was pompous and stupid.; Jade Ear Joe

Subjunctive posted:

How do you decide if you should put an else after a given conditional exit? Both of them are "signal error if a necessary condition isn't held". Having to compute some initial state and check it for preconditions is hardly uncommon. Having it be based on the number of lines in the then/else blocks sounds super fragile in the context of maintenance that could change the size of one of them. Now you need to reindent all the else content because you added or removed some statements in the then block.

It is subjective. There are some places where it is definitely wrong to use else after return because it is clearly a precondition check. There are other places where it is more readable to use if-else, and still other cases where it's debatable which should be used. It's not purely to do with number of lines, although you could use that as a factor in deciding.

"If I change the logic then I might need to change the appearance of the code to make sure it remains readable" isn't an argument against making code readable.

# ? May 11, 2016 17:18

qntm: Jun 17, 2009

feedmegin posted:

But what colour should we paint the bikeshed?

If I'm honest, I have absolutely no idea how to build a bikeshed.

# ? May 11, 2016 17:39

Cuntpunch: Oct 3, 2003; A monkey in a long line of kings

qntm posted:

If I'm honest, I have absolutely no idea how to build a bikeshed.

Who cares, take the initiative and just start building. If you're doing it wrong, I'm sure someone will correct you down the line.

# ? May 11, 2016 17:47

TooMuchAbstraction: Oct 14, 2012; I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.; Fun Shoe

Cuntpunch posted:

Who cares, take the initiative and just start building. If you're doing it wrong, I'm sure someone will correct you down the line.

This is the coding horrors thread, not the crappy construction tales thread.

# ? May 11, 2016 17:56

Cuntpunch: Oct 3, 2003; A monkey in a long line of kings

TooMuchAbstraction posted:

This is the coding horrors thread, not the crappy construction tales thread.

Same underlying problems. Over here you've got SOLID, over there you've got SLP(carpentry, anyway, gently caress knows what acronym plumbers use), and in both industries - having been in both - I can tell you that finding good projects reliably conforming to their respective architectural concerns is a rare thing.

# ? May 11, 2016 18:55

Factor Mystic: Mar 20, 2006; Baby's First Post-Apocalyptic Fiction

Plorkyeran posted:

Every person I've worked with that liked ? true : false was the most senior person on the team.

It's probably possible to write a Rosyln analyzer that finds scenarios where boolean literals are returned from a method or expression. That'd probably make it possible to turn things like this into at least a warning.

# ? May 11, 2016 19:58

Cuntpunch: Oct 3, 2003; A monkey in a long line of kings

Factor Mystic posted:

It's probably possible to write a Rosyln analyzer that finds scenarios where boolean literals are returned from a method or expression. That'd probably make it possible to turn things like this into at least a warning.

In the case of C# why would you even need this? If the ternary evaluation expression *doesn't* implicitly convert to bool, it won't even compile.

# ? May 11, 2016 20:03

nielsm: Jun 1, 2009

Cuntpunch posted:

In the case of C# why would you even need this? If the ternary evaluation expression *doesn't* implicitly convert to bool, it won't even compile.

Uh what? Ternaries can return any type, as long as the type of both the "if-true" and the "if-false" expression have the same type.
Factor Mystic is probably talking about detecting cases where a conditional is used to select between two boolean literals, and the result regardless of whether the condition was true or false, is used for a single assignment/immediate return/call.

# ? May 11, 2016 20:39

Cuntpunch: Oct 3, 2003; A monkey in a long line of kings

That makes more sense. Burnout excuse, etc. I read it as thinking of an analyzer that would go "hey the left hand of the ternary resolves to a bool!" and I apologize for being dumb, that's *the level of people I cope with daily*

That being said, I think more complicated versions of this come across more as a *smell* than a strictly redundant stupidity?

code:

var shouldWeDoThing = foo.name == "bar" ? (foo.phone == criteria) : (foo.email == criteria);

Doesn't strike me as being so bad, except that you're largely just using ternary as a complicated if-else.

# ? May 11, 2016 21:09

TheresaJayne: Jul 1, 2011

I don;t know how bad this actually is, I personally don;t like this but its a rule we have at work - no Magic numbers so this

return data[3] & 0xFF | (data[2] & 0xFF << 8) | (data[1] &0xFF << 16) | (data[0] & 0xFF << 24);

which takes the 4 bytes passed in and turns them into an int;

is now

return data[FOURTH_BYTE] & BYTE_MASK | (data[THIRD_BYTE] & BYTE_MASK << BYTE_SIZE_SHIFT) | (data[SECOND_BYTE] & BYTE_MASK << DOUBLE_BYTE_SIZE_SHIFT) | (data[FIRST_BYTE] & BYTE_MASK << TRIPLE_BYTE_SIZE_SHIFT);

with all the NAMES being private static final int at the top of the class.

and if the same number is used elsewhere, its called something different.

# ? May 12, 2016 06:39

rjmccall: Sep 7, 2007; no worries friend; Fun Shoe

I personally would not consider any of those to be "magic numbers"; a comment on the line above like "Interpret the first four bytes as a big-endian int." should make all the numbers self-describing enough. Maybe you could add another comment about &ing with 0xFF to suppress sign-extension if you were really worried that the reader might not be familiar with the pain of Java byte manipulation.

The places where you generally want to use a named constant (like a static final int) instead of a literal are when the exact numeric value is meaningful for some non-numeric reason. Sometimes people phrase this as "if you can imagine the number changing", but that's not quite right; for example, the GIF specification says that the value 2 in a particular place means to dispose of a graphic by restoring those pixels to the background color, and that's never going to change, but you still should write GRAPHIC_DISPOSAL_REPLACE_WITH_BACKGROUND or something instead of 2. The point is that the named constant tells you what the value means, and it (implicitly, indirectly) suggests what the alternative values might be.

It's usually even better to take advantage of whatever enum feature your language gives you, but sometimes that's not workable.

rjmccall fucked around with this message at 08:03 on May 12, 2016

# ? May 12, 2016 08:00

Eela6: May 25, 2007; Shredded Hen

TheresaJayne posted:

I don;t know how bad this actually is, I personally don;t like this but its a rule we have at work - no Magic numbers so this

return data[3] & 0xFF | (data[2] & 0xFF << 8) | (data[1] &0xFF << 16) | (data[0] & 0xFF << 24);

which takes the 4 bytes passed in and turns them into an int;

is now

return data[FOURTH_BYTE] & BYTE_MASK | (data[THIRD_BYTE] & BYTE_MASK << BYTE_SIZE_SHIFT) | (data[SECOND_BYTE] & BYTE_MASK << DOUBLE_BYTE_SIZE_SHIFT) | (data[FIRST_BYTE] & BYTE_MASK << TRIPLE_BYTE_SIZE_SHIFT);

with all the NAMES being private static final int at the top of the class.

and if the same number is used elsewhere, its called something different.

That sounds awful. The rule of 'no magic numbers' is generally good. Replacing magic numbers with defines that talk about what they do is great. SIGNAL_THRESHOLD = 2**-5, or whatever.

But in this case, your bit flags are exactly what they do: they're bit flags! Do you have to define ZERO = 0, ONE = 1?

I do support representing bit flags as binary when you can.

code:

# good:
LEVEL_ONE_COMPETE = 0b0001
LEVEL_TWO_COMPLETE = 0b0010
LEVEL_THREE_COMPLETE = 0b0100
LEVEL_FOUR_COMPLETE = 0b1000

#bad
FIRST_BIT = 1
SECOND_BIT = 2
THIRD_BIT = 4
FOURTH_BIT = 8

Note: I am still a relatively novice / academic programmer. Take my thoughts with a grain of salt.

rjmccall posted:

The point is that the named constant tells you what the value means, and it (implicitly, indirectly) suggests what the alternative values might be.

This gets to the core of the matter, I think.

Eela6 fucked around with this message at 08:15 on May 12, 2016

# ? May 12, 2016 08:12

Spatial: Nov 15, 2007

rjmccall posted:

Java byte manipulation.

Triggered.

# ? May 12, 2016 13:11

KernelSlanders: May 27, 2013; Rogue operating systems on occasion spread lies and rumors about me.

Hammerite posted:

code:

function f(thing) {
  if (!thing.prop) {
    throw "need a prop";
  }
  
  var stuff = getStuff(thing);
  if (!stuff.id.isUnique()) {
    throw "need unique";
  } else {
    doItToStuff(stuff);
  }
}

This would be a great situation for lazy values, although I think the real problem is that getStuff isn't handling its own error checking. "need a prop" should be handed in getStuff and having to check is leaking implementation details.

# ? May 12, 2016 13:36

Steve French: Sep 8, 2003

Subjunctive posted:

code:


function f(thing) {
    if (!thing.prop) {
        throw "need a prop";
    } else {
        var stuff = getStuff(thing);
        if (!stuff.id.isUnique()) {
            throw "need unique";
        } else {
            doItToStuff(stuff);
        }
    }
}

function f(thing) {
  if (!thing.prop) {
    throw "need a prop";
  }
  
  var stuff = getStuff(thing);
  if (!stuff.id.isUnique()) {
    throw "need unique";
  }

  doItToStuff(stuff);
}

Since somehow nobody else has called you out on this yet, I guess I will. Maybe a bit more fair comparison if you use the same size indentation in both cases? Come on.

# ? May 12, 2016 15:16

TooMuchAbstraction: Oct 14, 2012; I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.; Fun Shoe

TheresaJayne posted:

I don;t know how bad this actually is, I personally don;t like this but its a rule we have at work - no Magic numbers so this

return data[3] & 0xFF | (data[2] & 0xFF << 8) | (data[1] &0xFF << 16) | (data[0] & 0xFF << 24);

which takes the 4 bytes passed in and turns them into an int;

is now

return data[FOURTH_BYTE] & BYTE_MASK | (data[THIRD_BYTE] & BYTE_MASK << BYTE_SIZE_SHIFT) | (data[SECOND_BYTE] & BYTE_MASK << DOUBLE_BYTE_SIZE_SHIFT) | (data[FIRST_BYTE] & BYTE_MASK << TRIPLE_BYTE_SIZE_SHIFT);

Man, I remember talking about this in the intermediate-level coding class I took in college. Prof pointed out that a hard-and-fast "no bare numbers in code" rule means that if you want to determine if a number is even, you have to do "if (foo % TWO == 0)". Which is dumb! The meaning of the word "even" as it applies to your program logic is not going to change!

A magic number is a number whose value is not implicitly tied to its function in the code. You #define them (or make a static const, whatever) so that if their value should need to change for some reason, you can change the #define instead of needing to find every instance of the value so you can update it -- attaching a meaningful name to the value is a secondary bonus. In your example, however, the values of the numbers are tied to their function in the code: you're manipulating specific bytes of a known-size number. Creating #defines for each offset is worsening legibility for no gain in later ease of modification.

Indeed, if you did need to change the offsets later, and accomplished this by changing the #defines, you'd almost certainly make the code even harder to read! A classic way to make obfuscated code is to have #defines that don't do exactly what they appear to do.

# ? May 12, 2016 15:27

tyrelhill: Jul 30, 2006

Steve French posted:

Since somehow nobody else has called you out on this yet, I guess I will. Maybe a bit more fair comparison if you use the same size indentation in both cases? Come on.

Who cares? Complaining about code indendtation and styling in general is the most pedantic poo poo ever.

# ? May 12, 2016 15:28

Subjunctive: Sep 12, 2006; ✨sparkle and shine✨

Steve French posted:

Since somehow nobody else has called you out on this yet, I guess I will. Maybe a bit more fair comparison if you use the same size indentation in both cases? Come on.

Sorry, I was editing on my phone and didn't notice; my habit is 2, but I do 4 on forums for clarity. I don't think it would make a big difference, though, since for the un-nested case it's just one level of indentation.

(Why is my habit not the clearer choice? 25 years of bad habits.)

# ? May 12, 2016 15:32

CPColin: Sep 9, 2003; Big ol' smile.

rjmccall posted:

I personally would not consider any of those to be "magic numbers"; a comment on the line above like "Interpret the first four bytes as a big-endian int."

Yeah, I agree with that. For a computation where none of the pieces really has any meaning on its own, I think it's fine to skip defining constants, as long as some documentation says where the values come from.

We have code all over the place at work that looks like this:

Java code:

private static final String DESCRIPTION = "blah blah blah";

public String getDescription()
{
   return DESCRIPTION;
}

and it always makes me want to refactor it.

# ? May 12, 2016 15:34

Steve French: Sep 8, 2003

Subjunctive posted:

Sorry, I was editing on my phone and didn't notice; my habit is 2, but I do 4 on forums for clarity. I don't think it would make a big difference, though, since for the un-nested case it's just one level of indentation.

(Why is my habit not the clearer choice? 25 years of bad habits.)

Haha, I figured it was accidental somehow; I just found it amusing imagining that you were trying to put one over on us

# ? May 12, 2016 15:46

Hammerite: Mar 9, 2007; And you don't remember what I said here, either, but it was pompous and stupid.; Jade Ear Joe

var x = y == z ? y : z;

# ? May 12, 2016 16:04

Soricidus: Oct 21, 2010; freedom-hating statist shill

TooMuchAbstraction posted:

Man, I remember talking about this in the intermediate-level coding class I took in college. Prof pointed out that a hard-and-fast "no bare numbers in code" rule means that if you want to determine if a number is even, you have to do "if (foo % TWO == 0)". Which is dumb! The meaning of the word "even" as it applies to your program logic is not going to change!

Uh I can still see a magic number there pal

The correct way to write that is of course

code:

if ((foo & PARITY_MASK) == PARITY_EVEN) {...}

# ? May 12, 2016 19:20

QuarkJets: Sep 8, 2008

I missed the discussion a couple pages back but wanted to point out that while Python's GIL is a PITA if you're just trying to parallelize some random thing, you can release it completely in functions that are compiled by Numba. I wouldn't be surprised if this is spawning other coding horrors though

# ? May 12, 2016 19:54

raminasi: Jan 25, 2005; a last drink with no ice

Hammerite posted:

var x = y == z ? y : z;

The nightmare scenario I immediately imagined was an overloaded ==.

# ? May 12, 2016 19:59

quiggy: Aug 7, 2010; [in Russian] Oof.

Hammerite posted:

var x = y == z ? y : z;

This is beautiful.

# ? May 12, 2016 20:04

JawnV6: Jul 4, 2004; So hot ...

TooMuchAbstraction posted:

Man, I remember talking about this in the intermediate-level coding class I took in college. Prof pointed out that a hard-and-fast "no bare numbers in code" rule means that if you want to determine if a number is even, you have to do "if (foo % TWO == 0)". Which is dumb! The meaning of the word "even" as it applies to your program logic is not going to change!

Whoa there, what's that magic numeral on the other side of the comparison? Shouldn't it be "if (foo % TWO == ZERO)"

Even better "if (foo % TWO == NO_REMAINDER_VALUE)"

# ? May 12, 2016 20:54

TooMuchAbstraction: Oct 14, 2012; I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.; Fun Shoe

JawnV6 posted:

Whoa there, what's that magic numeral on the other side of the comparison? Shouldn't it be "if (foo % TWO == ZERO)"

Even better "if (foo % TWO == NO_REMAINDER_VALUE)"

See, this kind of poo poo is why I was tempted to just write "if (foo % TWO)" and assume I was operating in a language with auto conversion between ints and bools (and that treated int 0 as false). Pedants. Pedants everywhere!

# ? May 12, 2016 21:24

Linear Zoetrope: Nov 28, 2011; A hero must cook

Obviously you should use the Keycap Digit Two emoji for your variable name.

# ? May 12, 2016 21:45

Klades: Sep 8, 2011

JawnV6 posted:

Whoa there, what's that magic numeral on the other side of the comparison? Shouldn't it be "if (foo % TWO == ZERO)"

Even better "if (foo % TWO == NO_REMAINDER_VALUE)"

Careful, you might get some definition collisions. Gotta use namespaces!
if (foo % NUMBERS::TWO == DIVISION_RESULTS::NO_REMAINDER_VALUE)

# ? May 12, 2016 22:18

Van Kraken: Feb 13, 2012

code:

#define IS_ODD % 2
#define IS_EVEN % 2 == 0

# ? May 12, 2016 22:19

Eela6: May 25, 2007; Shredded Hen

Van Kraken posted:

code:

#define IS_ODD % 2
#define IS_EVEN % 2 == 0

Wow.

# ? May 12, 2016 22:21

xzzy: Mar 5, 2009

I still see some magic numbers in there.

What if you need two to be four in the future? Look at all those updates you have to make.

# ? May 12, 2016 22:27

Adbot: ADBOT LOVES YOU

# ? Jun 5, 2024 07:27

quiggy: Aug 7, 2010; [in Russian] Oof.

xzzy posted:

I still see some magic numbers in there.

What if you need two to be four in the future? Look at all those updates you have to make.

#define IF_IS_DIVISIBLE_BY(x) % x == 0

:v:

# ? May 12, 2016 22:29

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!

«‹›1503 »