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!

«‹›1501 »

BattleMaster: Aug 14, 2000

Avenging Dentist posted:

I've got bad news for you.

Well the wacky-rear end nonstandard one for the systems I work on doesn't check :colbert:

code:

void SRAMfree(unsigned char * NEAR pSRAM)
{	
	// Release the segment
	(*(SALLOC *)(pSRAM - 1)).bits.alloc = 0;
}

That's probably worth a coding horror by itself. Edit: It has a seperate function that needs to be called to actually relink the list to reflect the new free space

BattleMaster fucked around with this message at 04:31 on Nov 11, 2009

# ? Nov 11, 2009 04:29

Adbot: ADBOT LOVES YOU

# ? Apr 27, 2024 17:17

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

BattleMaster posted:

Well the wacky-rear end nonstandard one for the systems I work on doesn't check

Excuses, excuses. Next you'll tell me you don't want to implement your own (working) version of libc.

# ? Nov 11, 2009 04:32

TOO SCSI FOR MY CAT: Oct 12, 2008; this is what happens when you take UI design away from engineers and give it to a bunch of hipster art student "designers"

Avenging Dentist posted:

You do realize this is not only legit, but required to avoid a leak? (Unless there's some other behavior in UnregisterChild that breaks things).

Sorry, I've been staring at this all day and maybe I forgot it's not obvious. Look at the call stack:

code:

[parent] SomeClass::~SomeClass()
[parent]  delete first child
[child ]    SomeClass::~SomeClass()
[parent]      UnregisterChild()
[parent]        itr->UnregisterSibling
[parent]  delete second child
[child ]    SomeClass::~SomeClass()
[parent]      UnregisterChild()
[parent]        itr->UnregisterSibling // itr = first child, segfault

e: the issue is that after deleting each child, the vector retains a pointer to the child's old memory. This could be accessed later by procedures calling up into the parent, causing a crash.

Solved it by copying the vector to a backup, clearing, then deleting, but it was a pain to search through all the destructors looking for this.

TOO SCSI FOR MY CAT fucked around with this message at 05:13 on Nov 11, 2009

# ? Nov 11, 2009 05:11

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

Janin posted:

Sorry, I've been staring at this all day and maybe I forgot it's not obvious. Look at the call stack:

Well, not having the implementation of that, I figured it wouldn't care about that. Thought I can't fathom why something that appears to be a tree would need to care about its siblings.

# ? Nov 11, 2009 05:12

TOO SCSI FOR MY CAT: Oct 12, 2008; this is what happens when you take UI design away from engineers and give it to a bunch of hipster art student "designers"

Avenging Dentist posted:

Well, not having the implementation of that, I figured it wouldn't care about that. Thought I can't fathom why something that appears to be a tree would need to care about its siblings.

I posted the implementation of UnregisterChild() in the original code block :confused:

each leaf contains pointers to its parents, and children, and siblings, and random other nodes elsewhere in the tree because the guy who designed this library doesn't have to implement it, so he doesn't care how difficult working on it is

# ? Nov 11, 2009 05:15

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

Janin posted:

I posted the implementation of UnregisterChild() in the original code block

And nothing in the code you posted breaks anything. I mean, I know why it would fail, but that part isn't actually shown.

Avenging Dentist fucked around with this message at 08:49 on Nov 11, 2009

# ? Nov 11, 2009 08:43

Tim Berners-Lee: Apr 12, 2007; Inventor of the Internet

Janin posted:

Sorry, I've been staring at this all day and maybe I forgot it's not obvious. Look at the call stack:
code:
[parent] SomeClass::~SomeClass()
[parent]  delete first child
[child ]    SomeClass::~SomeClass()
[parent]      UnregisterChild()
[parent]        itr->UnregisterSibling
[parent]  delete second child
[child ]    SomeClass::~SomeClass()
[parent]      UnregisterChild()
[parent]        itr->UnregisterSibling // itr = first child, segfault
e: the issue is that after deleting each child, the vector retains a pointer to the child's old memory. This could be accessed later by procedures calling up into the parent, causing a crash.

Solved it by copying the vector to a backup, clearing, then deleting, but it was a pain to search through all the destructors looking for this.

As long as UnregisterChild() (or something else in the destructor) removes the child from the parents list of children, it looks like it should work. That seems to be the intent of the code even though that step was omitted. (Unless each child needs to UnregisterSibling on each other child. Then that won't work.) It's a bug, but I don't see the horror.

# ? Nov 11, 2009 10:47

newsomnuke: Feb 25, 2007

Tim Berners-Lee posted:

It's a bug, but I don't see the horror.

Well the horror comes more from the fact that the whole thing could just be written as

code:

SomeClass::~SomeClass() {

  vector<SomeClass*>::iterator itr;
  for (itr = children.begin(); itr != children.end(); ++itr) {
      delete *itr;
  }
}

# ? Nov 11, 2009 14:32

TOO SCSI FOR MY CAT: Oct 12, 2008; this is what happens when you take UI design away from engineers and give it to a bunch of hipster art student "designers"

Tim Berners-Lee posted:

As long as UnregisterChild() (or something else in the destructor) removes the child from the parents list of children, it looks like it should work. That seems to be the intent of the code even though that step was omitted. (Unless each child needs to UnregisterSibling on each other child. Then that won't work.) It's a bug, but I don't see the horror.

The point is that 1) the destructor leaves dangling pointers in attributes and 2) deleting children can call methods which access those pointers. You're over-thinking it, just look at the code and you'll see that as soon as the first child is deleted the object enters an invalid state.

# ? Nov 11, 2009 19:16

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

no wonder I can't find any uses of method getTaxRateUS().

code:

private static double getTaxRate(String strTaxMode, 
    ServletContext application, 
    HttpServletRequest request, 
    String strPostalCode) {
	if (strPostalCode == null || strPostalCode.length() == 0) {
		return 0;	
	}
	try {
		Object[] args = new Object[3];
		args[0] = (Object) application;
		args[1] = (Object) request;
		args[2] = (Object) strPostalCode;
		Class[] methodParameters = new Class[3];
		methodParameters[0] = Class.forName("javax.servlet.ServletContext");
		methodParameters[1] = Class.forName("javax.servlet.http.HttpServletRequest");;
		methodParameters[2] = Class.forName("java.lang.String");
		Method[] arrMethods = Tax.class.getDeclaredMethods();
		Method methodToExecute = null;
		if (arrMethods.length > 0) {
			for (int count = 0;
                            (count < arrMethods.length && methodToExecute == null); 
                            count++) {
				Method aMethod = arrMethods[count];
				if (aMethod.getName().equals("getTaxRate" + strTaxMode)) {
					methodToExecute = Tax.class.getDeclaredMethod(
                                            "getTaxRate" + strTaxMode,
                                            methodParameters);
				}
			}	
		}
		if (methodToExecute != null) {
			Double rate = (Double) methodToExecute.invoke(
                            Class.forName("syrinx.cart.Tax"), 
                            args);
			if (rate != null) {
				return rate.doubleValue();
			}	

		}
	}
	catch (Exception exc) {
		Utils.reportException(exc);
		return 0;
	}
	return 0;
}

# ? Nov 11, 2009 20:29

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

Impressive, particularly since that entire thing is just:

code:

private static double getTaxRate(String strTaxMode, ServletContext application, HttpServletRequest request, String strPostalCode) {
  if (strPostalCode == null || strPostalCode.length() == 0)
    return 0;	
  try {
    Object[] args = { application, request, strPostalCode };
    Class[] argClasses = { ServletContext.class, HttpServletRequest.class, String.class };
    Method m = Tax.class.getDeclaredMethod("getTaxRate" + strTaxMode, argClasses)
    if (m != null) return ((Double) m.invoke(null, args)).doubleValue();
  } catch (Exception exc) {
    Utils.reportException(exc);
  }
  return 0;
}

(and even smaller if you're in Java 1.5)

# ? Nov 11, 2009 21:38

HFX: Nov 29, 2004

BattleMaster posted:

Well the wacky-rear end nonstandard one for the systems I work on doesn't check
code:
void SRAMfree(unsigned char * NEAR pSRAM)
{	
	// Release the segment
	(*(SALLOC *)(pSRAM - 1)).bits.alloc = 0;
}
That's probably worth a coding horror by itself. Edit: It has a seperate function that needs to be called to actually relink the list to reflect the new free space

PowerTV? That is a joke of an OS and nothing is compliant. I would perfectly expect to see code exactly like that to get around some undocumented "feature".

HFX fucked around with this message at 21:46 on Nov 11, 2009

# ? Nov 11, 2009 21:42

Zaxxon: Feb 14, 2004; Wir Tanzen Mekanik

why do I keep seeing crap like this.

#define BSP_DISABLE_RXMP __BSP_DISABLE_RXMP__

and such. Really who does this poo poo help?

# ? Nov 12, 2009 01:55

BattleMaster: Aug 14, 2000

HFX posted:

PowerTV? That is a joke of an OS and nothing is compliant. I would perfectly expect to see code exactly like that to get around some undocumented "feature".

Nah, it's for a PIC18 microcontroller. Some of the weird poo poo that you have to come up with to get things to run within the memory and clock speed constraints are pretty awesome though.

I did some thinking last night and thought a of reason why it works that way. If you want your program to execute faster and you are confident that the pointer isn't invalid you can forgo checking it. Also, you can call the stack relinking function once after freeing a whole bunch of things so it only happens once instead of every time. Now normally that wouldn't matter a whole lot but sometimes you just want something to be as zippy as possible because it's running on a battery and clocked at 100kHz to save power.

Sometimes embedded systems are coding horrors in and of themselves

Edit: though to be honest I kind of want to write my own malloc/free implementation now because it would be way more efficient to just unlink the segment you just freed rather than periodically iterating through the entire heap to see if stuff was flagged as free. Oh god it's me, I'm the coding horror

BattleMaster fucked around with this message at 02:17 on Nov 12, 2009

# ? Nov 12, 2009 02:05

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

rjmccall posted:

Impressive, particularly since that entire thing is just:

code:

private static double getTaxRate(String strTaxMode, ServletContext application, HttpServletRequest request, String strPostalCode) {
  if (strPostalCode == null || strPostalCode.length() == 0)
    return 0;	
  try {
    Object[] args = { application, request, strPostalCode };
    Class[] argClasses = { ServletContext.class, HttpServletRequest.class, String.class };
    Method m = Tax.class.getDeclaredMethod("getTaxRate" + strTaxMode, argClasses)
    if (m != null) return ((Double) m.invoke(null, args)).doubleValue();
  } catch (Exception exc) {
    Utils.reportException(exc);
  }
  return 0;
}

(and even smaller if you're in Java 1.5)

It gets even worse when you know the context. There's only one method it will ever match.

# ? Nov 12, 2009 02:08

xarph: Jun 18, 2001

Janin posted:

Please post more details about insane Japanese software, because this post right here is incredible.

This one is tame, except it does it for EVERY. SINGLE. PROMPT. in a 100+ step installation checklist.

# ? Nov 12, 2009 03:24

Ensign Expendable: Nov 11, 2008; Lager beer is proof that god loves us; Pillbug

Wow, that's....is it all command line?

# ? Nov 12, 2009 05:37

xarph: Jun 18, 2001

Ensign Expendable posted:

Wow, that's....is it all command line?

Yes, though to their credit, the customer never sees this part (it's the install process for the OS that lives in one of our appliances).

# ? Nov 12, 2009 06:10

TheSleeper: Feb 20, 2003

Does it go on to ask if you're sure that you're sure that "yes" is correct?

# ? Nov 12, 2009 06:19

ErIog: Jul 11, 2001

The brilliant part is that it defaults to 'yes' which means that it's not actually confirming anything. You could make a slip on the enter key and probably race through half the prompts without realizing it.

It's stupid, but at least inoffensive in its implementation.

# ? Nov 12, 2009 06:53

MagneticWombats: Aug 19, 2004; JUMP!

Couldn't you do something like

code:

 yes | stupidInstall

# ? Nov 12, 2009 21:19

Inverse Icarus: Dec 4, 2003; I run SyncRPG, and produce original, digital content for the Pathfinder RPG, designed from the ground up to be played online.

MagneticWombats posted:

Couldn't you do something like
code:
 yes | stupidInstall 

I never never heard of this. It's so hilariously simple, and useful.

# ? Nov 12, 2009 22:16

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

MagneticWombats posted:

Couldn't you do something like
code:
 yes | stupidInstall 

this works until:

code:

Do you want to erase the hard drive? [no]
Are you sure you want to erase the hard drive? [no]

# ? Nov 12, 2009 22:18

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

Ryouga Inverse posted:

this works until:

One would think that if you were installing an operating system onto a device, that this wouldn't matter. Also one would think that you'd know what the prompts were before you decided you could pipe yes into it.

# ? Nov 12, 2009 22:21

Smugdog Millionaire: Sep 14, 2002; 8) Blame Icefrog

Ryouga Inverse posted:

this works until:

code:

Do you want to erase the hard drive? [no]
Are you sure you want to erase the hard drive? [no]

If typing yes to those questions doesn't erase the hard drive, why did they prompt you about it?

# ? Nov 13, 2009 00:06

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.

Here's some more barrels from our UI code:

code:

/**
 *
 * Not implemented
 *
 */
public void showTitle(String title, int thing)
{
    showTitle(title, thing);
}

Elsewhere...

code:

try
{
    somebody.showTitle(title, thing);
    // etc...
}
catch(Throwable t)
{
    logger.log("Error while blah-di-dah: ", t);
}

# ? Nov 13, 2009 00:11

xarph: Jun 18, 2001

MagneticWombats posted:

Couldn't you do something like
code:
 yes | stupidInstall 

Can't drop to shell.

Usually we hook it up to a serial console and cat in a long text file that looks like this:

quote:

n

n

<ip>

4

One time it broke and we couldn't figure out why; turns out the intern went and opened it in notepad and wondered why there were squares all over the place. He replaced all the squares with newlines and saved it.

Why did this break everything?

a) what's unix's default newline?
b) what's the newline for the DOS .txt format that notepad.exe still uses?

edit: "but it worked in putty! :downs:

# ? Nov 13, 2009 00:12

pokeyman: Nov 26, 2006; That elephant ate my entire platoon.

Ryouga Inverse posted:

this works until:

I can't figure out a way to make yes take a line feed. Just strips slashes here on OS X.

# ? Nov 13, 2009 00:13

chips: Dec 25, 2004; Mein Führer! I can walk!

Mustach posted:

Here's some more barrels from our UI code:

code:

/**
 *
 * Not implemented
 *
 */
public void showTitle(String title, int thing)
{
    showTitle(title, thing);
}

Elsewhere...

code:

try
{
    somebody.showTitle(title, thing);
    // etc...
}
catch(Throwable t)
{
    logger.log("Error while blah-di-dah: ", t);
}

Does this deliberately cause a stack depth exception of some kind?

# ? Nov 13, 2009 00:53

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.

Based on all evidence that I could find, it was not intentional.

# ? Nov 13, 2009 02:37

Sweeper: Nov 29, 2007; The Joe Buck of Posting; Dinosaur Gum

Avenging Dentist posted:

No because delete checks for null before it does anything.

You explain that to my programming professor who marked down my code because I didn't check for NULL. I showed him the iso standard and he didn't give me my points back.

# ? Nov 13, 2009 04:42

Ensign Expendable: Nov 11, 2008; Lager beer is proof that god loves us; Pillbug

Sweeper posted:

You explain that to my programming professor who marked down my code because I didn't check for NULL. I showed him the iso standard and he didn't give me my points back.

My programming professor took off marks for returning from a void.

# ? Nov 13, 2009 05:26

nbv4: Aug 21, 2002; by Duchess Gummybuns

Maybe this isn't quite a coding horror, but I'll tell it anyway. About five months ago, I was looking around the simple machines site (a forum software package), and noticed that RC1 of the new 2.0 version was released a few days earlier. I thought "oh cool i'll update my forum to the new 2.0 tonight", since my forum is fairly low traffic and not super vital. Plus I like using cutting edge software. I got busy and eventally just decided to wait a few more days until 2.0 proper gets released. Fast forward to today. I went to the site to see what the heck is going on and I see that there was a RC1.1, RC1.2, and now RC2. I take a look at the changelog and I see this:

quote:

SMF 2.0 RC2 8 November 2009
=======================================================
November 2009
-------------------------------------------------------
* The preview post could cause a JavaScript error in some browsers. (Post template) [Bug 3717]
! Inserting BBC in non-WYSIWYG mode was broken for some browsers. (script.js)
! Language selection did not show correctly in profile (Profile-Modify.php)
! In some cases the guest_title may not exist (Load.php) [Bug 3916]
* IE6 and IE7 were having difficulties with the Core theme's implementation of min-height. (index.css, rtl.css)
* Auto suggest will throw an error if the field doesn't exist (ManageBans template)
! Guests can't have warnings, so don't try to find them. (PersonalMessage.php) [Bug 3941]
+ Allow multiple selections in select boxes. (Admin template)
! Prevent the detectBrowser function from being mislead by Internet Explorer identifying itself as two different versions. (Load.php)
! Improved debugging of load times. (Subs-Db-mysql.php, Subs-Db-postgresql.php, Subs-Db-sqlite.php, Subs.php, ViewQuery.php, index language file)
! Stat collection didn't send out proper database information (Stats.php)
! Update the default theme's name when installing the Core theme on upgrade. (upgrade_2-0_mysql.sql, upgrade_2-0_postgresql.sql, upgrade_2-0_sqlite.sql) [Bug 3779]
* Rewrote the 'Getting Started' part of the help section to match Curve's look and feel. (Help template, index.css)
* Changed the way linktrees are classes in order for there to be more than three linktree instances on a page. (index template, index.css)
* Rewrote the 'Registering' and 'Forum Tools' parts of the help section to match Curve's look and feel. (Help template, Manual language file, index.css)
* The board selection part of the advanced search screen could break on some resolutions. (Search template, index.css, compat.css, rtl.css)
! Don't typecast when checking for empty variables in the View Member section of the admin centre. (ManageMembers.php) [Bug 3917]
! In some rare cases, creating a cache file for language files failed, leading to undefined indices. (ManageMaintenance.php) [Bug 3951]
* Curve does not support showing buttons as images, so don't offer it as an option. (Settings template)
! Skip separators when saving theme settings, as they aren't actual settings. (Themes.php)
! Make sure there's always a birthday email template set, and be consistent with it. (install script, upgrade script, ManageMail.php, ScheduledTasks.php)
! Fixed footer breaking due to possible invalid XHTML. (Recent template)
! Output additional HTML for quotes to make it possible to style them with rounded corners. (Subs.php)
! Time bbcode was handled improperly when editing a post. (Subs-Post.php)
! Buddy list used wrong permission check. (Profile.php)
! Store the selected filter for the Who's Online page in the session, so it may be applied again if the user revisits the page later. (Who.php)
! Remove the index on the hits column of the log_activity table, as it's a waste of cycles. (install SQL files, upgrade SQL files) [Bug 3960]
! Added three new smileys sets to the package, replacing the old ones. (Smileys/default/*, Smileys/aaron/*, Smileys/akyhne/*)
* Updated Curve's post icons to reflect the new default smiley set. (Themes/default/images/post/*)
! Hide the View Results button for polls when it is no longer possible to vote. (Display.php)
! Include the compatibility stylesheets for themes designed for 2.0 Beta, too. (Load.php)

This is just for the first 8 days of November. I think someone doesn't know what "release candidate" means...

# ? Nov 13, 2009 08:46

Datyedyeguy: Aug 20, 2007; COLOR

Found this looking over a new programmer's code.

code:

bool RitualCrystal::CheckCollision( const Point& point )
{
	bool result = m_crystalBmp->HitTest( point );

	if( !result )
		return false;

	return true;
}

# ? Nov 14, 2009 15:51

tef: May 30, 2004; -> some l-system crap ->

pokeyman posted:

I can't figure out a way to make yes take a line feed. Just strips slashes here on OS X.

code:

$ yes '
'

# ? Nov 14, 2009 16:33

ctz: Feb 6, 2003

Datyedyeguy posted:

Found this looking over a new programmer's code.

If this is the worst thing you can find then s/he doesn't sound bad to me.

Not to mention that the obvious use of logical not may be incorrect, depending on the type returned by 'HitTest'.

# ? Nov 14, 2009 17:29

Adhemar: Jan 21, 2004; Kellner, da ist ein scheussliches Biest in meiner Suppe.

Datyedyeguy posted:

Found this looking over a new programmer's code.

code:

bool RitualCrystal::CheckCollision( const Point& point )
{
	bool result = m_crystalBmp->HitTest( point );

	if( !result )
		return false;

	return true;
}

My god. This is only excusable if you guys are being paid per line.

# ? Nov 14, 2009 18:03

dimebag dinkman: Feb 20, 2003

That's kind of scraping the bottom of the Coding Horror barrel. Unless I am missing something, it doesn't seriously impact performance, code readability, logical correctness, or anything like that - it's just a bit redundant. Also, are you sure this is the final form this function will take? I can imagine sometimes I might write a function like that if I know that pretty soon there's going to be more going on in it.

# ? Nov 14, 2009 18:16

king_kilr: May 25, 2007

dimebag dinkman posted:

That's kind of scraping the bottom of the Coding Horror barrel. Unless I am missing something, it doesn't seriously impact performance, code readability, logical correctness, or anything like that - it's just a bit redundant. Also, are you sure this is the final form this function will take? I can imagine sometimes I might write a function like that if I know that pretty soon there's going to be more going on in it.

Yeah, no it's terrible. It decreases readability.

# ? Nov 14, 2009 18:30

Adbot: ADBOT LOVES YOU

# ? Apr 27, 2024 17:17

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

It's indicative of 1:1 mental modelling to code, and shows that to him the code only makes sense if it explicit returns "true" somewhere and "false" somewhere else. There's no jump to the higher order of thinking where the return value of the call can be returned, or once you've achieved that, possibly scrapping the helper function altogether because you realize the inner call does everything you need.

# ? Nov 14, 2009 19:31

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!

«‹›1501 »