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 »

Axel Rhodes Scholar: May 12, 2001; Courage Reactor

Zombywuf posted:

Have I mentioned Emacs recently?

p.s. Today I discovered that C-M-h C-w will kill the function the point is in :-)

there is/was an emacs thread around, please do post in it it needs to come back

# ? Feb 27, 2009 15:32

Adbot: ADBOT LOVES YOU

# ? Apr 25, 2024 11:26

Lexical Unit: Sep 16, 2003

Found this code today:

code:

const char *host = getenv ("DATA_HOST");
if (!host) host = "";

const char *path = getenv ("DATA_PATH");
if (!path) path = "";

const char *file = /* got from a drop down box, can possibly be "" */;

char cmd[256];
sprintf (cmd, "rsh %s /bin/rm -rf %s/%s &", host, path, file);

FILE *ptr = popen (cmd, "r");
if (ptr) {
  char buf[256];
  while (fgets (buf, sizeof (buf), ptr) != NULL) {
    (void) fprintf (stderr, "%s", buf);
  }
  int rc = pclose (ptr);
} else {
  perror ("popen");
}

if (0 != rc) {
  char message[256];
  sprintf (message, "unable to delete %s", file);
  fprintf (stderr, "%s", message);
}

Edit: Oh yes, I forgot to mention this code is found in a .cpp file.

Lexical Unit fucked around with this message at 22:57 on Feb 27, 2009

# ? Feb 27, 2009 22:54

BigRedDot: Mar 6, 2008

You forgot to mention that it took three people to craft that gem.

# ? Feb 27, 2009 22:59

Lexical Unit: Sep 16, 2003

Actually, more than 5 people have touched the file over the course of 5 years and all 5 of these people still work here. In fact, 3 of the 5 can be considered my supervisors.

# ? Feb 27, 2009 23:02

mr_jim: Oct 30, 2006; OUT OF THE DARK

Lexical Unit posted:

code:

sprintf (cmd, "rsh %s /bin/rm -rf %s/%s &", host, path, file);

Wow. How has this not blown up in your face yet?

At the very least an exception could be raised if not all of the environment variables are set.

# ? Feb 27, 2009 23:12

Lexical Unit: Sep 16, 2003

Good question. I asked my boss the same thing. He shrugged. He is one of the 5.

Edit: The rabbit hole gets deeper. The program comes with a .env that should be sourced before execution. That file looks like this:

code:

export DATA_HOST=${DATA_HOST:-"ERROR: undefined"}
export DATA_PATH=${DATA_PATH:-"ERROR: undefined"}

Problem solved! :downs:

Lexical Unit fucked around with this message at 23:55 on Feb 27, 2009

# ? Feb 27, 2009 23:41

Kelson: Jan 23, 2005

code:

sprintf (cmd, "rsh %s /bin/rm -rf %s/%s &", host, path, file);

mr_jim posted:

Wow. How has this not blown up in your face yet?

At the very least an exception could be raised if not all of the environment variables are set.

I was more worried about someone modifying host, path, or file to include ";echo hackAccount::0:0::/root:/bin/bash >> /etc/passwd"

# ? Feb 28, 2009 03:39

mr_jim: Oct 30, 2006; OUT OF THE DARK

Kelson posted:

code:
sprintf (cmd, "rsh %s /bin/rm -rf %s/%s &", host, path, file);
I was more worried about someone modifying host, path, or file to include ";echo hackAccount::0:0::/root:/bin/bash >> /etc/passwd"

I really hope that the program is running under an unprivileged account, but that might be expecting too much.

# ? Feb 28, 2009 04:40

Kelson: Jan 23, 2005

mr_jim posted:

I really hope that the program is running under an unprivileged account, but that might be expecting too much.

Many of the more important files can still be read from even if they're unprivileged, and many risky apps can be executed as well. An attacker could, for example, use it to download privilege escalating malware (one of the many reasons your server should never execute wget...)

# ? Feb 28, 2009 05:47

BigRedDot: Mar 6, 2008

quote:

An attacker could, for example, use it to download privilege escalating malware (one of the many reasons your server should never execute wget...)

Runs on an airgapped network, so downloading malware or anything else for that matter, is not a concern. That's not an affirmative defense of this code, of course.

# ? Feb 28, 2009 07:56

Lexical Unit: Sep 16, 2003

mr_jim posted:

I really hope that the program is running under an unprivileged account, but that might be expecting too much.

Yeah no. It runs as root.

# ? Feb 28, 2009 08:19

sklnd: Nov 26, 2007; NOT A TRACTOR

I'm working on porting a ~30,000+ line embedded c app to linux. Now, the entire thing is a case study regarding coding horrors and I could go on for quite some time with fun examples from it, but here's my favorite so far:

code:

#define PRINT_TO_STRING printEnd += sprintf(printEnd,

my editor is really loving the unmatched parens that are strewn about the code as a result.

# ? Mar 7, 2009 04:07

ColdPie: Jun 9, 2006

sklnd posted:

I'm working on porting a ~30,000+ line embedded c app to linux. Now, the entire thing is a case study regarding coding horrors and I could go on for quite some time with fun examples from it, but here's my favorite so far:
code:
#define PRINT_TO_STRING printEnd += sprintf(printEnd,
my editor is really loving the unmatched parens that are strewn about the code as a result.

So... does it start writing to unallocated memory if the string gets longer than the size of printEnd? Man, that's so much easier and safer than just writing a simple logging function! Good work team, commit that poo poo.

# ? Mar 7, 2009 07:01

Zombywuf: Mar 29, 2008

code:

try
{
  return result;
}
finally
{
  log.Info("Some log message");
}

Log message changed to protect the guilty.

# ? Mar 11, 2009 13:08

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"

Not directly code-related, but my new job uses this horrible VCS from IBM, named "Synergy". Check out the rad 90's design (it looks even worse in UNIX):

Use of the UNIX version involves SSH, port forwarding, and NFS. Its sole purpose appears to be to inhibit any useful development.

# ? Mar 12, 2009 02:17

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"

code:

class LogSingleton {
public:
	static void PrintMessage (std::string text) {
		if (!instance)
			instance = new LogSingleton ();
		instance->RealPrintMessage (text);
	}
	
	static void PrintError (std::string text) {
		if (!instance)
			instance = new LogSingleton ();
		instance->RealPrintError (text);
	}
	
private:
	void RealPrintMessage (std::string text) {
		std::cout << text << "\n";
	}
	void RealPrintError (std::string text) {
		std::cout << "AN ERROR" << text << "\n";
	}
	static LogSingleton *instance;
};

# ? Mar 13, 2009 03:26

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

Haha, someone read about singletons, and the example was probably a logger. That's awesome.

Sometimes I write code like this - the part of my brain that says "You're doing too much" just disengages.

# ? Mar 13, 2009 03:32

Strong Sauce: Jul 2, 2003; You know I am not really your father.

Why... why am I calling this private method...

# ? Mar 13, 2009 05:39

Zombywuf: Mar 29, 2008

Janin posted:

code:

class LogSingleton {
public:
	static void PrintMessage (std::string text) {
		if (!instance)
			instance = new LogSingleton ();
		instance->RealPrintMessage (text);
	}
	
	static void PrintError (std::string text) {
		if (!instance)
			instance = new LogSingleton ();
		instance->RealPrintError (text);
	}
	
private:
	void RealPrintMessage (std::string text) {
		std::cout << text << "\n";
	}
	void RealPrintError (std::string text) {
		std::cout << "AN ERROR" << text << "\n";
	}
	static LogSingleton *instance;
};

What's you're problem, would you rather gently caress with the std::cout singleton when you need to change logging behaviour?

I'll admit it could use some thread safety and should really be two classes, but it's not horrific.

# ? Mar 13, 2009 11:37

TSDK: Nov 24, 2003; I got a wooden uploading this one

Zombywuf posted:

What's you're problem,

Ahem...

code:

class LogSingleton {
public:
	static void PrintMessage (std::string text) {
		std::cout << text << "\n";
	}
	
	static void PrintError (std::string text) {
		std::cout << "AN ERROR" << text << "\n";
	}
};

# ? Mar 13, 2009 12:18

Zombywuf: Mar 29, 2008

I'm assuming the original developer was planning to add more sophisticated code later, like locking, but didn't get the time.

Basically it's overcomplicated for what it does right now, but it's pretty clear as to how it's doing what it's doing.

# ? Mar 13, 2009 13:13

TSDK: Nov 24, 2003; I got a wooden uploading this one

Zombywuf posted:

I'm assuming the original developer was planning to add more sophisticated code later, like locking, but didn't get the time.

Basically it's overcomplicated for what it does right now, but it's pretty clear as to how it's doing what it's doing.

Perhaps he/she didn't get the time because he/she was always adding in code that wasn't actually needed.

Coding for the future can be appropriate, however, there's also a lot to be said for waiting until you have a better idea of what's needed before writing code. That way if you never need it then you've not wasted any time, and if what you actually turn out to need is different to what you thought you'd need, then there's less code that needs changing.

# ? Mar 13, 2009 14:25

Zombywuf: Mar 29, 2008

TSDK posted:

Perhaps he/she didn't get the time because he/she was always adding in code that wasn't actually needed.

Coding for the future can be appropriate, however, there's also a lot to be said for waiting until you have a better idea of what's needed before writing code. That way if you never need it then you've not wasted any time, and if what you actually turn out to need is different to what you thought you'd need, then there's less code that needs changing.

True, but as horrors go, this is unhorrorfic.

# ? Mar 13, 2009 14:38

twodot: Aug 7, 2005; You are objectively correct that this person is dumb and has said dumb things

Zombywuf posted:

True, but as horrors go, this is unhorrorfic.

Reading code like this is, for me, exactly like reading sentences where people are unnecessarily using unusual words, or even using words that aren't words like irregardless. They very clearly don't have a good grasp of the best ways of conveying ideas, they likely don't understand at all what they are saying, and are just copying what other people have said, and they probably just aren't smart or curious at all. After all, if they were they would have bothered to look up irregardless.

edit:
If you write code like this where you do something unnecessary because you believe it will be necessary in the future, you better at least comment why you are doing something unnecessary. If I saw this in a source tree I was working on, I would do something similar to what TSDK did without thinking twice.

twodot fucked around with this message at 16:08 on Mar 13, 2009

# ? Mar 13, 2009 16:02

julyJones: Feb 12, 2007; Stopped making sense.

twodot posted:

. . .they probably just aren't smart or curious at all. After all, if they were they would have bothered to look up irregardless.

look up irregardless

To actually add to the discussion: I do this kind of thing myself, for better or worse. For me, it's a defensive strategy; you can't be guaranteed the person who's going to be using or modifying that code later will know how it was meant to be used, especially if people don't read or write documentation, like where I work.

# ? Mar 13, 2009 16:36

twodot: Aug 7, 2005; You are objectively correct that this person is dumb and has said dumb things

julyJones posted:

look up irregardless

Is there some pet dictionary of yours I should use, because every dictionary I've looked it up in says it's a nonstandard usage of regardless.

quote:

To actually add to the discussion: I do this kind of thing myself, for better or worse. For me, it's a defensive strategy; you can't be guaranteed the person who's going to be using or modifying that code later will know how it was meant to be used, especially if people don't read or write documentation, like where I work.

Adding unnecessary code lets people more easily understand how it was meant to be used? I would think they would just be confused by the unnecessary code, and assume whoever wrote it wasn't thinking at the time, or that it was necessary at one point in them, but since then whatever reason it had to exist was removed, and it's unnecessary now.

edit:
To clarify my point, code which doesn't do anything but waste cycles at best offers no additional clarity, because there are lots of ways useless code can be introduced into a project. If you want to prevent people from using a function/class/whatever in certain ways you have two tools 1) Add asserts that stop people from doing whatever you are trying to stop them from doing 2) Add comments (or documentation I guess) telling them not to do it. If they don't read your comments, they likely won't read your code either.

twodot fucked around with this message at 16:54 on Mar 13, 2009

# ? Mar 13, 2009 16:42

MarsMattel: May 25, 2001; God, I've heard about those cults Ted. People dressing up in black and saying Our Lord's going to come back and save us all.

I think the real horror is the two copies of the string being created each time a message is printed. :smith:

# ? Mar 13, 2009 16:44

biznatchio: Mar 31, 2001; Buglord

twodot posted:

Is there some pet dictionary of yours I should use, because every dictionary I've looked it up in says it's a nonstandard usage of regardless.

We use ANSI around these parts. None of that nonstandard city talk. :clint:

# ? Mar 13, 2009 16:47

julyJones: Feb 12, 2007; Stopped making sense.

twodot posted:

Adding unnecessary code lets people more easily understand how it was meant to be used? I would think they would just be confused by the unnecessary code, and assume whoever wrote it wasn't thinking at the time, or that it was necessary at one point in them, but since then whatever reason it had to exist was removed, and it's unnecessary now.

That is a possibility too, but my hope is always that when people see something they're not sure about, they'll ask around and get some opinions from people before removing code. My bet in this case would be that taking the time to write a dozen lines or so, if there's even a small chance there'll actually be a real logger implemented later, is better than leaving people to their own devices, in which case you might get a bunch of variations on screen and file output that'll be a pain to refactor later.

# ? Mar 13, 2009 17:01

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

Zombywuf posted:

std::cout singleton

Hahahahahahahaha

# ? Mar 13, 2009 20:46

POKEMAN SAM: Jul 8, 2004

My girlfriend was sent an auto-generated password to log into some medical site. We couldn't believe the password:

) |. /

It worked fine to login, but I mean, who the hell thought of writing a generator to create those kinds of passwords?

# ? Mar 13, 2009 22:32

Lonely Wolf: Jan 20, 2003; Will hawk false idols for heaps and heaps of dough.

Never heard of the ole generate a password by taking a random 1x14 rectangle out of a piece of ASCII art? Oldest trick in the book, man.

# ? Mar 13, 2009 23:16

POKEMAN SAM: Jul 8, 2004

Lord Uffenham posted:

Never heard of the ole generate a password by taking a random 1x14 rectangle out of a piece of ASCII art? Oldest trick in the book, man.

Hahaha, that's great.

# ? Mar 13, 2009 23:27

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

I want my password to be Bender! o-(8 E|

# ? Mar 13, 2009 23:32

Mikey-San: Nov 3, 2005; I'm Edith Head!

Avenging Dentist posted:

I want my password to be Bender! o-(8 E|

5318008 is the best password

# ? Mar 13, 2009 23:38

JingleBells: Jan 7, 2007; Oh what fun it is to see the Harriers win away!

Janin posted:

Not directly code-related, but my new job uses this horrible VCS from IBM, named "Synergy". Check out the rad 90's design (it looks even worse in UNIX):

Use of the UNIX version involves SSH, port forwarding, and NFS. Its sole purpose appears to be to inhibit any useful development.

Oh god, don't remind me.

Our client used this for their source control, when we gave them a new release they always wanted us to (somehow) get code into it. We just gave up and put zips on our FTP. Only one machine had the software on it, and no-one on the project at the time knew how to use the drat thing, it was just one big clusterfuck.

You don't happen to work for a large british utility company?

# ? Mar 14, 2009 00:24

fletcher: Jun 27, 2003; ken park is my favorite movie; Cybernetic Crumb

I'm trying to fix a bug in some poorly written Actionscript 2.0 app. I wish I could post the entire source, this thing is a loving disaster. I already went through the supporting javascript, it's down to 30 very clear lines instead of 250 lines of garbled bullshit. The actionscript is thousands of lines though, gently caress.

edit: here we go. Why pass it as an argument to the function when you can build it into the function name??

code:

private function saveInput(objField):Void{
	var userinput = objField.userinput;
	_root.debug("INPUT","=========== saveInput("+userinput+") ==========");
	Constants.data_main[0] = userinput;
	Constants.obj_timelines[0].redraw();
}

private function saveInput5(objField):Void{
	var userinput = objField.userinput;
	_root.debug("INPUT","=========== saveInput5("+userinput+") ==========");
	Constants.data_main[0] = userinput;
	Constants.data_main[1] = userinput;
	Constants.data_main[2] = userinput;
	Constants.data_main[3] = userinput;
	Constants.data_main[4] = userinput;
	Constants.obj_timelines[0].redraw();
	Constants.obj_timelines[1].redraw();
	Constants.obj_timelines[2].redraw();
	Constants.obj_timelines[3].redraw();
	Constants.obj_timelines[4].redraw();
}

fletcher fucked around with this message at 02:25 on Mar 14, 2009

# ? Mar 14, 2009 01:37

TheSleeper: Feb 20, 2003

fletcher posted:

I'm trying to fix a bug in some poorly written Actionscript 2.0 app. I wish I could post the entire source, this thing is a loving disaster.

Oh cool, thanks for the heads up.

# ? Mar 14, 2009 02:15

fletcher: Jun 27, 2003; ken park is my favorite movie; Cybernetic Crumb

TheSleeper posted:

Oh cool, thanks for the heads up.

I just wanted to vent

# ? Mar 14, 2009 02:16

Adbot: ADBOT LOVES YOU

# ? Apr 25, 2024 11:26

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"

JingleBells posted:

You don't happen to work for a large british utility company?

No, it's an American company.

Zombywuf posted:

What's you're problem, would you rather gently caress with the std::cout singleton when you need to change logging behaviour?

I'll admit it could use some thread safety and should really be two classes, but it's not horrific.

cout isn't a singleton, LogSingleton isn't a (proper) singleton, almost any use of "singletons" is dumb, and the particular implementation of static methods proxying to private method is absurd.

MarsMattel posted:

I think the real horror is the two copies of the string being created each time a message is printed.

References are regularly used elsewhere in the code in lieu of return values, but I've never seen a single const reference parameter.

# ? Mar 14, 2009 05:43

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 »