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 »

mr_jim: Oct 30, 2006; OUT OF THE DARK

Vanadium posted:

I would consider keeping the maths out of the loop head

Yes, that too. At first glance I missed that there was a loop hiding in there.

# ? Mar 25, 2010 23:38

Adbot: ADBOT LOVES YOU

# ? May 13, 2024 21:43

Supervillin: Feb 6, 2005; Pillbug

Whether you like OTBS or Allman or K&R or some other crappy style, for the love of all that is holy USE BRACES. Fixed late last year:

code:

        JSDefinition *dn = ALE_DEFN(ale);
        if (dn->pn_op == JSOP_GETARG)
            // The definition's source position will be more precise.
            pn = dn;

Edit: vvv Nothing, as it stands. It breaks poo poo the day someone says "Oh, add this one line before (or after) pn = dn" if they don't also add braces. Gotchas and "but you should just watch out for that" should not be standard programming practice.

Supervillin fucked around with this message at 16:15 on Mar 26, 2010

# ? Mar 25, 2010 23:59

An Outland Dish: Jul 26, 2009; by Tiny Fistpump

Supervillin posted:

Whether you like OTBS or Allman or K&R or some other crappy style, for the love of all that is holy USE BRACES. Fixed late last year:
code:
        JSDefinition *dn = ALE_DEFN(ale);
        if (dn->pn_op == JSOP_GETARG)
            // The definition's source position will be more precise.
            pn = dn;

I'm confused by this. What's wrong with the omission of braces here?

# ? Mar 26, 2010 11:58

HFX: Nov 29, 2004

mr_jim posted:

I'd consider creating well-named local variables to store the results of some of those function calls. player.cursor.getYCoord(), player.cursor.getXCoord, and player.getDistanceOfSight() in particular.

Why? Provided you have a half rear end compiler, a query should be in lined running. The only problem with the previous is if the user allowed a player to update while that test was taking place and you didn't want that to change while doing the work. Otherwise, the method calls are fine.

I agree with the math in the loops head complaint.

An Outland Dish posted:

I'm confused by this. What's wrong with the omission of braces here?

Nothing or it wouldn't compile. However, adding braces to every if...then...else, while, for, etc block is a really good programming practice. I've run into a lot of code that is:

code:

if (test1 && test2 && test3...<way off the display now>... && test4) return;
    z = x+5

Of course you can still do that even with braces. This is why I actually started liking white-space significant languages. I used to hate them.

HFX fucked around with this message at 13:53 on Mar 26, 2010

# ? Mar 26, 2010 13:47

ColdPie: Jun 9, 2006

An Outland Dish posted:

I'm confused by this. What's wrong with the omission of braces here?

pre:

if(0)
  //what
  printf("hi");

prints "hi". your if-statement is useless.

# ? Mar 26, 2010 14:07

trex eaterofcadrs: Jun 17, 2005; My lack of understanding is only exceeded by my lack of concern.

ColdPie posted:

pre:
if(0)
  //what
  printf("hi");
prints "hi". your if-statement is useless.

just for fun I ran this through both java and gcc and neither printed anything.

# ? Mar 26, 2010 14:11

Flobbster: Feb 17, 2005; "Cadet Kirk, after the way you cheated on the Kobayashi Maru test I oughta punch you in tha face!"

ColdPie posted:

pre:
if(0)
  //what
  printf("hi");
prints "hi". your if-statement is useless.

http://codepad.org/Xb6mlPGD

Comments are stripped out during the lexical analysis stage, if the compiler does what you're saying it does, it's wrong.

(efb)

# ? Mar 26, 2010 14:11

Mustach: Mar 2, 2003; In this long line, there's been some real strange genes. You've got 'em all, with some extras thrown in.

Unless you mean for that comment to stand in for actual code, you are completely wrong.

Anyhow, the second-worst part about that big java condition is this:

code:

if(something > -1) ...

Assuming something is an integer type, use the drat >= operator. If it's a float, what's so great about -0.4326?

Mustach fucked around with this message at 14:13 on Mar 26, 2010

# ? Mar 26, 2010 14:11

HFX: Nov 29, 2004

Mustach posted:

Unless you mean for that comment to stand in for actual code, you are completely wrong.

Anyhow, the second-worst part about that big java condition is this:
code:
if(something > -1) ...
Assuming something is an integer type, use the drat >= operator. If it's a float, what's so great about -0.4326?

What is that in reference too?

# ? Mar 26, 2010 15:13

Mustach: Mar 2, 2003; In this long line, there's been some real strange genes. You've got 'em all, with some extras thrown in.

This post

# ? Mar 26, 2010 15:16

HFX: Nov 29, 2004

Mustach posted:

This post

He said it was in Java. In order to be assignable to a byte, it has to be integer coercible which only the integer types are capable without a using a cast or a library functions (welcome to Java's one silent type coercion)

# ? Mar 26, 2010 15:22

ColdPie: Jun 9, 2006

TRex EaterofCars posted:

just for fun I ran this through both java and gcc and neither printed anything.

Hahaha durp I typed my test case wrong :downs:

# ? Mar 26, 2010 15:27

Mustach: Mar 2, 2003; In this long line, there's been some real strange genes. You've got 'em all, with some extras thrown in.

HFX posted:

He said it was in Java. In order to be assignable to a byte, it has to be integer coercible which only the integer types are capable without a using a cast or a library functions (welcome to Java's one silent type coercion)

I was bitching about the code's use of > -1 instead of >= 0.

# ? Mar 26, 2010 16:19

mr_jim: Oct 30, 2006; OUT OF THE DARK

HFX posted:

Why? Provided you have a half rear end compiler, a query should be in lined running. The only problem with the previous is if the user allowed a player to update while that test was taking place and you didn't want that to change while doing the work. Otherwise, the method calls are fine.

I agree with the math in the loops head complaint.

I wouldn't do it as an optimization, just as a matter of style. I think that something like this (using local variables, and leaving everything else the same):

code:

else {
    byte yCoord = player.cursor.getYCoord();
    byte xCoord = player.cursor.getXCoord();
    byte sightDistance = player.getDistanceOfSight();

    for ( byte y = ( yCoord - sightDistance > -1 ) ?
            (byte) ( yCoord - sightDistance ) : 0;
            ( yCoord + sightDistance <= newMap.returnMapHeight() &&
              y < (byte) ( yCoord + sightDistance ) ) ||
            ( yCoord >= newMap.returnMapHeight() -
              sightDistance && y < newMap.returnMapHeight() ); y++ ) {
        for ( byte x = ( xCoord - sightDistance > -1 ) ?
                (byte) ( xCoord - sightDistance ) : 0;
                ( xCoord + sightDistance <= newMap.returnMapWidth() &&
                  x < (byte) ( xCoord + sightDistance ) ) ||
                ( xCoord >= newMap.returnMapWidth() - sightDistance &&
                  x < newMap.returnMapWidth() ); x++ ) {

is more readable than this:

code:

else
	{
		for ( byte y = ( player.cursor.getYCoord() - player.getDistanceOfSight() > -1 ) ? 
			(byte) ( player.cursor.getYCoord() - player.getDistanceOfSight() ) : 0; 
			( player.cursor.getYCoord() + player.getDistanceOfSight() <= newMap.returnMapHeight() && 
			y < (byte) ( player.cursor.getYCoord() + player.getDistanceOfSight() ) ) || 
			( player.cursor.getYCoord() >= newMap.returnMapHeight() - 
			player.getDistanceOfSight() && y < newMap.returnMapHeight() ); y++ )
		{
			for ( byte x = ( player.cursor.getXCoord() - player.getDistanceOfSight() > -1 ) ? 
				(byte) ( player.cursor.getXCoord() - player.getDistanceOfSight() ) : 0; 
				( player.cursor.getXCoord() + player.getDistanceOfSight() <= newMap.returnMapWidth() 
				&& x < (byte) ( player.cursor.getXCoord() + player.getDistanceOfSight() ) ) || 
				( player.cursor.getXCoord() >= newMap.returnMapWidth() - player.getDistanceOfSight() 
				&& x < newMap.returnMapWidth() ); x++ )
			{

But they are both still pretty bad.

# ? Mar 26, 2010 16:54

HFX: Nov 29, 2004

mr_jim posted:

I wouldn't do it as an optimization, just as a matter of style. I think that something like this (using local variables, and leaving everything else the same):

code:

else {
    byte yCoord = player.cursor.getYCoord();
    byte xCoord = player.cursor.getXCoord();
    byte sightDistance = player.getDistanceOfSight();

    for ( byte y = ( yCoord - sightDistance > -1 ) ?
            (byte) ( yCoord - sightDistance ) : 0;
            ( yCoord + sightDistance <= newMap.returnMapHeight() &&
              y < (byte) ( yCoord + sightDistance ) ) ||
            ( yCoord >= newMap.returnMapHeight() -
              sightDistance && y < newMap.returnMapHeight() ); y++ ) {
        for ( byte x = ( xCoord - sightDistance > -1 ) ?
                (byte) ( xCoord - sightDistance ) : 0;
                ( xCoord + sightDistance <= newMap.returnMapWidth() &&
                  x < (byte) ( xCoord + sightDistance ) ) ||
                ( xCoord >= newMap.returnMapWidth() - sightDistance &&
                  x < newMap.returnMapWidth() ); x++ ) {

is more readable than this:

code:

else
	{
		for ( byte y = ( player.cursor.getYCoord() - player.getDistanceOfSight() > -1 ) ? 
			(byte) ( player.cursor.getYCoord() - player.getDistanceOfSight() ) : 0; 
			( player.cursor.getYCoord() + player.getDistanceOfSight() <= newMap.returnMapHeight() && 
			y < (byte) ( player.cursor.getYCoord() + player.getDistanceOfSight() ) ) || 
			( player.cursor.getYCoord() >= newMap.returnMapHeight() - 
			player.getDistanceOfSight() && y < newMap.returnMapHeight() ); y++ )
		{
			for ( byte x = ( player.cursor.getXCoord() - player.getDistanceOfSight() > -1 ) ? 
				(byte) ( player.cursor.getXCoord() - player.getDistanceOfSight() ) : 0; 
				( player.cursor.getXCoord() + player.getDistanceOfSight() <= newMap.returnMapWidth() 
				&& x < (byte) ( player.cursor.getXCoord() + player.getDistanceOfSight() ) ) || 
				( player.cursor.getXCoord() >= newMap.returnMapWidth() - player.getDistanceOfSight() 
				&& x < newMap.returnMapWidth() ); x++ )
			{

But they are both still pretty bad.

They are both pretty readable to me, one is just longer lines and I slightly lose reference that the xcoordinate refers to the cursor and not the player. I'd really recommend a lot of that work being pushed into functions which did returned a result of the test.

I've been dealing with people this week who worry about how many functions they call and thus writing terribly large methods. They figure the function calls are expensive (they aren't) and don't realize the compiler is inlining a lot of them anyway.

Mustach posted:

I was bitching about the code's use of > -1 instead of >= 0.

Good point. I really have no idea, but it wouldn't be the first time I've seen people scared of >= or <=.

# ? Mar 26, 2010 17:20

Crazy RRRussian: Mar 5, 2010; by Fistgrrl

What is wrong with complicated conditions and/or math in for loop? It can make sense and be elegant in certain ways. If you can't really handle a "weird" looking for loop then what are you doing programming in first place? I am pretty sure that coding horror is not just slightly "weird" looking code but something that is wrong/inefficient and/or just plain stupid.

Now the real coding horror is the post increment instead of pre-increment as last statement tin for loop's head!

# ? Mar 26, 2010 17:47

Mustach: Mar 2, 2003; In this long line, there's been some real strange genes. You've got 'em all, with some extras thrown in.

RussianManiac/hexadecimal again?

# ? Mar 26, 2010 17:55

Vanadium: Jan 8, 2005

Why the hell would java care whether you pre- or postincrement anything

# ? Mar 26, 2010 17:58

Dijkstracula: Mar 18, 2003; You can't spell 'vector field' without me, Professor!

LockeNess Monster posted:

What is wrong with complicated conditions and/or math in for loop? It can make sense and be elegant in certain ways.

What's wrong with it is that it prevents reading the loop header as a "starting from ____, and until _______, do what's in this code block" statement. Like, really, is that ternary operator really any easier to read than using min()?

Mustach posted:

RussianManiac/hexadecimal again?

Yep, I think so. Tsk tsk, you played your hand too early with the pre/postincrement comment. A shameful troll :qq:

# ? Mar 26, 2010 18:00

Seth Turtle: May 6, 2007; by Tiny Fistpump

Why can't you just do this and make it readable to everyone?

EDIT: North and South might be backwards but whatever.

EDIT 2: ClampLo is 10x better than Min. :colbert:

code:

    private static byte clampLo(byte lo, byte arg) {
       return (lo > arg) ? lo : arg;
    }
    private static byte clampHi(byte arg, byte hi) {
       return (hi < arg) ? hi : arg;
    }

code:

else {
    byte yCoord = player.cursor.getYCoord();
    byte xCoord = player.cursor.getXCoord();
    byte sightDistance = player.getDistanceOfSight();

    byte xEast  = xCoord + sightDistance;
    byte xWest  = xCoord - sightDistance;
    byte yNorth = yCoord + sightDistance;
    byte ySouth = yCoord - sightDistance;

    for ( byte y = Math.max(0,ySouth); y < Math.min(yNorth,newMap.returnMapHeight()); ++y) {
        for ( byte x = Math.max(0,xWest); x < Math.min(xEast,newMap.returnMapWidth()); ++x ) {
           ...
        }
    }

Seth Turtle fucked around with this message at 19:48 on Mar 26, 2010

# ? Mar 26, 2010 18:07

Zhentar: Sep 28, 2003; Brilliant Master Genius

^^^^ Woo re-implementing standard library functions with unusual names! Brilliant!

LockeNess Monster posted:

What is wrong with complicated conditions and/or math in for loop? It can make sense and be elegant in certain ways.

"Elegant" is not the word I'd choose for needing to search through 6 lines of code to find the semicolons needed to even begin understanding what the loop is doing.

# ? Mar 26, 2010 18:59

Avenging Dentist: Oct 1, 2005; oh my god is that a circular saw that does not go in my mouth aaaaagh

Mustach posted:

RussianManiac/hexadecimal again?

Hey mods can we have a price check on how much hexadecimal has spent in order to be repeatedly told to get out of CoC?

# ? Mar 26, 2010 20:25

Blotto Skorzany: Nov 7, 2008; He's a PSoC, loose and runnin'
came the whisper from each lip
And he's here to do some business with
the bad ADC on his chip
bad ADC on his chiiiiip

$entheogen = ?
$hexadecimal = ?
$russianmaniac = ?
$whateverhisnewnameis = ?

Let's Find Out!

# ? Mar 26, 2010 20:29

Crazy RRRussian: Mar 5, 2010; by Fistgrrl

Ok, I was kinda joking about pre/post increment stuff, I just don't think that some slightly complicated for loop header statements are coding horror. The code in question in this thread could be cleaned up and made more concise but it is not really a coding horror.

Also who is this hexadecimal you guys think I am?

# ? Mar 26, 2010 21:44

Zombywuf: Mar 29, 2008

mr_jim posted:

I wouldn't do it as an optimization, just as a matter of style. I think that something like this (using local variables, and leaving everything else the same):

code:

else {
    byte yCoord = player.cursor.getYCoord();
    byte xCoord = player.cursor.getXCoord();
    byte sightDistance = player.getDistanceOfSight();

    for ( byte y = ( yCoord - sightDistance > -1 ) ?
            (byte) ( yCoord - sightDistance ) : 0;
            ( yCoord + sightDistance <= newMap.returnMapHeight() &&
              y < (byte) ( yCoord + sightDistance ) ) ||
            ( yCoord >= newMap.returnMapHeight() -
              sightDistance && y < newMap.returnMapHeight() ); y++ ) {
        for ( byte x = ( xCoord - sightDistance > -1 ) ?
                (byte) ( xCoord - sightDistance ) : 0;
                ( xCoord + sightDistance <= newMap.returnMapWidth() &&
                  x < (byte) ( xCoord + sightDistance ) ) ||
                ( xCoord >= newMap.returnMapWidth() - sightDistance &&
                  x < newMap.returnMapWidth() ); x++ ) {

is more readable than this:

code:

else
	{
		for ( byte y = ( player.cursor.getYCoord() - player.getDistanceOfSight() > -1 ) ? 
			(byte) ( player.cursor.getYCoord() - player.getDistanceOfSight() ) : 0; 
			( player.cursor.getYCoord() + player.getDistanceOfSight() <= newMap.returnMapHeight() && 
			y < (byte) ( player.cursor.getYCoord() + player.getDistanceOfSight() ) ) || 
			( player.cursor.getYCoord() >= newMap.returnMapHeight() - 
			player.getDistanceOfSight() && y < newMap.returnMapHeight() ); y++ )
		{
			for ( byte x = ( player.cursor.getXCoord() - player.getDistanceOfSight() > -1 ) ? 
				(byte) ( player.cursor.getXCoord() - player.getDistanceOfSight() ) : 0; 
				( player.cursor.getXCoord() + player.getDistanceOfSight() <= newMap.returnMapWidth() 
				&& x < (byte) ( player.cursor.getXCoord() + player.getDistanceOfSight() ) ) || 
				( player.cursor.getXCoord() >= newMap.returnMapWidth() - player.getDistanceOfSight() 
				&& x < newMap.returnMapWidth() ); x++ )
			{

But they are both still pretty bad.

The real horror is spaces around braces.

# ? Mar 26, 2010 22:22

GROVER CURES HOUSE: Aug 26, 2007; Go on...

Zombywuf posted:

The real horror is spaces around braces.

At least it's (byte) and not ( byte ).

# ? Mar 26, 2010 22:38

mr_jim: Oct 30, 2006; OUT OF THE DARK

Also, breaking lines after a ||, but before a &&.

# ? Mar 26, 2010 22:49

GROVER CURES HOUSE: Aug 26, 2007; Go on...

mr_jim posted:

Also, breaking lines after a ||, but before a &&.

And then forgetting to break a line before one of the &&s. That block is physically painful to read.

# ? Mar 26, 2010 22:52

blorpy: Jan 5, 2005

ColdPie posted:

pre:
if(0)
  //what
  printf("hi");
prints "hi". your if-statement is useless.

holy moley coldpie

it's you. you're the coding horror itt.

# ? Mar 26, 2010 23:24

trex eaterofcadrs: Jun 17, 2005; My lack of understanding is only exceeded by my lack of concern.

Zombywuf posted:

The real horror is spaces around braces.

I just got access to the source of an extremely expensive product and their coding style is Allman with bracespace. :smith:

# ? Mar 26, 2010 23:49

ColdPie: Jun 9, 2006

Markov Chain Chomp posted:

holy moley coldpie

it's you. you're the coding horror itt.

I was sure the comment didn't count as a statement, so I tried it out, and found out that it does! Except I'm apparently retarded and managed to do it wrong. But that sure didn't stop me from posting, no way.

# ? Mar 27, 2010 00:10

Mustach: Mar 2, 2003; In this long line, there's been some real strange genes. You've got 'em all, with some extras thrown in.

At least you're not hexadecimal.

�or are you

# ? Mar 27, 2010 00:53

1337JiveTurkey: Feb 17, 2005

I was teaching some middle schoolers how to use Game Maker's visual programming interface and it turns out that a comment block immediately after an if or an else statement acts in exactly the manner that ColdPie's describing. It sucks when something like that comes up during a live demonstration of how and why you should always comment your code.

# ? Mar 27, 2010 01:44

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

TRex EaterofCars posted:

I just got access to the source of an extremely expensive product and their coding style is Allman with bracespace.

if there's one thing visual studio does well (and it actually does a lot of things well), it's that it defaults to enforcing a sensible brace standard, so most c# projects don't have crazy brace styles.

unless you have a problem with the default VS brace style, which i guess is a problem for you

# ? Mar 27, 2010 01:46

Dijkstracula: Mar 18, 2003; You can't spell 'vector field' without me, Professor!

Ryouga Inverse posted:

unless you have a problem with the default VS brace style, which i guess is a problem for you

Am I gonna have to go on another tirade about how Allman-style bracing is the most beautiful of all bracing < :mad:

# ? Mar 27, 2010 02:14

Avenging Dentist: Oct 1, 2005; oh my god is that a circular saw that does not go in my mouth aaaaagh

Dijkstracula posted:

Am I gonna have to go on another tirade about how Allman-style bracing is the most beautiful of all bracing <>

It's also the ANSI standard.

# ? Mar 27, 2010 02:18

mr_jim: Oct 30, 2006; OUT OF THE DARK

K&R supremacy.

# ? Mar 27, 2010 02:59

Scaevolus: Apr 16, 2007

Seth Turtle posted:

code:

    private static byte clampLo(byte lo, byte arg) {
       return (lo > arg) ? lo : arg;
    }
    private static byte clampHi(byte arg, byte hi) {
       return (hi < arg) ? hi : arg;
    }

Why isn't he just using the normal int type? This is the kind of optimization you would do on an 8-bit processor.

# ? Mar 27, 2010 04:24

Avenging Dentist: Oct 1, 2005; oh my god is that a circular saw that does not go in my mouth aaaaagh

Scaevolus posted:

Why isn't he just using the normal int type? This is the kind of optimization you would do on an 8-bit processor.

Why isn't he making a generic--- oh wait, Java. :laugh:

# ? Mar 27, 2010 04:28

Adbot: ADBOT LOVES YOU

# ? May 13, 2024 21:43

Zhentar: Sep 28, 2003; Brilliant Master Genius

mr_jim posted:

K&R supremacy.

1TBS is accurately named.

# ? Mar 27, 2010 04: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 »