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 »

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

FamDav posted:

Are those spaces between function name and args really a thing? .

It can get worse. I've seen code similar to this:

code:

while ( x > 0 ) { 
    call_thing ( x , y ) ;
}

Like nails on a chalkboard to me.

# ? Mar 28, 2012 17:59

Adbot: ADBOT LOVES YOU

# ? May 15, 2024 17:58

Vanadium: Jan 8, 2005

FamDav posted:

Are those spaces between function name and args really a thing? .

That's also part of the gnome C style guidelines. Look for + Whitespace, halfways through http://git.gnome.org/browse/gtk+/plain/docs/CODING-STYLE.

Vanadium fucked around with this message at 20:57 on Mar 28, 2012

# ? Mar 28, 2012 19:59

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

I do not get that style at all. More so when they say that function definitions should not indent curly braces. And then for switch/case statements a case doesn't get indented after the indentation form the curly brace.

I'm all for following coding styles but it should just boil down to a few things. Don't make me remember complex indenting rules.

# ? Mar 28, 2012 20:32

double sulk: Jul 2, 2010

I started a new job today. For the time being I'm working with original ASP, even though I thought I'd be working with Rails. I don't know much about it/Ruby, but it was a position where I could go slowly -- then they had to go and find someone who knows it. It's not much money, barely anything, but I wanted the experience.

Keep in mind during all of this that I'm a bad programmer. This place has a system running (in the ASP) where your browser session has a key which would "activate" an offer if you entered the site through the appropriate landing page. You can also enter the key manually. So say the key is ABCD, and the session has to check for it. Well, instead of affixing properties to each current key which can simply be checked to see if it passes in the database or not as far as applying that offer, there are good old magic numbers. So absentmindedly, because I don't know ASP's syntax in the slightest, I asked this guy about it and he had me type the following to check for this specific key code:

code:

If Session("offer") <> "ebates-50" And pKey <> "1299628" Then 'not valid
		removeOffer = True
elseif Session("offer") <> "1299624" And pKey <> "1299624" Then
		removeOffer = True
End If

I entered it and it didn't work, and then I actually looked at the code and realized how bad this guy's programming skills are. So now, we have this hacked up solution.

code:

Select Case Session("offer")
	Case "1299624", "1299628", "ebates-50" 'valid
		removeOffer = False
	Case Else
		removeOffer = True
End Select
	
Select Case Session("key")
	Case "1299624", "1299628" 'valid
		removeOffer = False
	Case Else 'not valid
		removeOffer = True	
End Select

double sulk fucked around with this message at 23:41 on Mar 28, 2012

# ? Mar 28, 2012 21:39

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

Vanadium posted:

That's also part of the gnome C style guidelines. Look for + Whitespace, halfways through http://git.gnome.org/browse/gtk+/plain/docs/CODING-STYLE.

Not all GNOME code is like that... We mostly follow the GNU style guidelines, but module authors can do their own thing if they want... Like everything else opinionated, it's not that bad once you get used to it. I'm over bikeshedding, and just shut up and try my best to follow the code style of the module.

And hey, sometimes I write my code in GNU style, just because.

# ? Mar 28, 2012 22:00

ozymandOS: Jun 9, 2004

One of the features my mediocre company added to its crappy software recently was cross-time zone support. We don't store anything in UTC--not that we could, since we're maintaining backwards compatibility with lots of data that hasn't historically been in UTC. But see our justification:

quote:

Any timestamp that gets saved in UTC is dangerous because we need to convert the times to display in our application. We often do this wrong.

# ? Mar 28, 2012 23:17

Plorkyeran: Mar 22, 2007; To Escape The Shackles Of The Old Forums, We Must Reject The Tribal Negativity He Endorsed

I guess it's good to understand your limitations.

# ? Mar 28, 2012 23:45

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

If you display converted UTC wrong, won't you display converted local time wrong too? What's UTC's fault in that rationale?

# ? Mar 29, 2012 00:18

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

Well at least in that case it's only hosed up in the cross-time-zone part, instead of being hosed up everywhere.

That way it only affects your biggest customers instead of all them :rolleye:

# ? Mar 29, 2012 00:38

New Yorp New Yorp: Jul 18, 2003; Only in Kenya.; Pillbug

Sulk posted:

I started a new job today. For the time being I'm working with original ASP, even though I thought I'd be working with Rails.

I'd quit on the spot if a new employer pulled that poo poo. It wouldn't even be so bad if they bait-and-switched you with Ruby and ASP .NET webforms; at least ASP .NET has a career path (even if it's a diminished one with the rising popularity of ASP .NET MVC). At least you'd be learning a .NET language. Classic ASP is a career dead end.

# ? Mar 29, 2012 01:24

double sulk: Jul 2, 2010

Ithaqua posted:

I'd quit on the spot if a new employer pulled that poo poo. It wouldn't even be so bad if they bait-and-switched you with Ruby and ASP .NET webforms; at least ASP .NET has a career path (even if it's a diminished one with the rising popularity of ASP .NET MVC). At least you'd be learning a .NET language. Classic ASP is a career dead end.

I'm really strongly considering it. The issue, like I said, is that the day after I was brought on, they found someone else who supposedly knows Rails really well. Because the particular site we're working with (it's not a big company so it isn't urgent to update) is in Rails, it would have been a learning experience for me. Where I live, it's not a popular thing to work with. I went out and got the O'Reilly Ruby book, which since I work better out of printed texts, and also because it's supposed to be very good.

I have minimal working knowledge of Ruby, but I know some Python and the two are similar enough that I could in theory pick it up. I was really excited because I thought it could give me an opportunity to get some actual, good experience under my belt, even though I'm being paid almost nothing. They did mention ASP when I went to meet with them, but it seemed as if the work I'd be doing would be minimal, but this stuff is so archaic that I don't even feel like I'm really learning anything useful at all, and I don't even feel like working on learning Ruby because I don't know if I'll even be using it.

Like I said, I'm not a good programmer, but I was working on some of the code for a fairly significant website today, and this is quite literally what some of their code looks like:

http://pastebin.com/WEBKaCWK

I hope I'm not crazy.

double sulk fucked around with this message at 01:55 on Mar 29, 2012

# ? Mar 29, 2012 01:49

New Yorp New Yorp: Jul 18, 2003; Only in Kenya.; Pillbug

Sulk posted:

http://pastebin.com/WEBKaCWK

No, you're not crazy. That is horrible and retarded.

# ? Mar 29, 2012 02:13

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

Did they seriously hard code every special offer code they have?

# ? Mar 29, 2012 02:19

stuxracer: May 4, 2006

Sulk posted:

http://pastebin.com/WEBKaCWK

I hope I'm not crazy.

Nothing wrong with putting all that poo poo into your code. It's not like offers expire.

Sorry ExPIRE

# ? Mar 29, 2012 02:22

double sulk: Jul 2, 2010

Strong Sauce posted:

Did they seriously hard code every special offer code they have?

Yes. I'm not even joking. I think most of this code is theirs, or they could have redesigned it if they had the knowledge (I don't know how because I don't know SQL/VB).

I'm talking about in IRC and what I'm getting paid is insultingly low (pick a number and reduce that) and it was supposed to be because I have no portfolio and I don't know any Ruby/Rails, but at this point I'm not doing either.

# ? Mar 29, 2012 02:25

Scaramouche: Mar 26, 2001; SPACE FACE! SPACE FACE!

Sulk posted:

Yes. I'm not even joking. I think most of this code is theirs, or they could have redesigned it if they had the knowledge (I don't know how because I don't know SQL/VB).

I'm talking about in IRC and what I'm getting paid is insultingly low (pick a number and reduce that) and it was supposed to be because I have no portfolio and I don't know any Ruby/Rails, but at this point I'm not doing either.

Well you deserve it Sulk. Because you're a Flyers fan :colbert:

Joking! You're kind of in a bind in that regard. I don't suppose they're open to having you redo it in Ruby? It sounds like it's production code so you'd have to build it in parallel which is never a good thing, but it could be a crisistunity in that respect.

# ? Mar 29, 2012 03:12

mustermark: Apr 26, 2009; "Mind" is a tool invented by the universe to see itself; but it can never see all of itself, for much the same reason that you can't see your own back (without mirrors).

AlsoD posted:

From the same page as the christmas tree notation:
code:
// ensure string is null terminated
strcat(string, "\0");

This is a loving gem.

# ? Mar 29, 2012 05:22

wwb: Aug 17, 2004

I could have actually not got as involved in this project at this point. But my spidey sense tingled after the first dev meet and greet so I stuck with it.

Today I finally [after weeks of wrangling] get into the meat of the source code. There is lots of material. But I'll just leave you all with this:

code:

        if (Request.QueryString["UserName"] != null && Request.QueryString["UserName"].ToString().Length > 0)
        {
            UserName = Request.QueryString["UserName"].ToString();
        }
        if (Request.QueryString["Password"] != null && Request.QueryString["Password"].ToString().Length > 0)
        {
            Password = Request.QueryString["Password"].ToString();
        }
        if (UserName == "" & Password == "")
        {
            lblResponse.Text = "Invalid";
            return;
        }
Response.Clear();

        ObjAdminUser = new clsAdminUser();
        dsUser = ObjAdminUser.GetResults("select * from Users where UserName = '" + UserName + "'");
        if (dsUser.Tables[0].Rows.Count > 0 && dsUser != null)
        {
            Response.Write ("Username already exists!");
        }
        else
        {
            UserID = Save_Value();
            if (UserID == 1)
            {
                Response.Write("Success");
                Session["FirstName"] = FirstName;
                Session["LastInitial"] = LastName;
                Session["UserName"] = UserName;
                Session["UserID"] = UserID;
            }
            else
                Response.Write("Failure");
        }

I'm not sure how far I want to follow this rabbit hole.

# ? Mar 29, 2012 06:33

pigdog: Apr 23, 2004; by Smythe

Strong Sauce posted:

Did they seriously hard code every special offer code they have?

I hope not to sound like a contrarian jerk... But aside from general badness and not putting start and expiry dates in the code :doh:

it's not necessarily a bad idea to hard-code promotions like that, depending on circumstances. Making a nice administration UI for something like that an order of magnitude more work. If an offer is complex, then making it configurable is even more complex. If it's a small company, the number of offers and options are limited enough to have a grasp of, and it's easy to deploy new versions, then it may very well be easier to tell the VB programmer to update the code and redeploy. As opposed to spending a lot more resources, and dealing with a lot more integration, security and maintenance issues, to develop an interface for non-technical people who may end up needing the help of technical people regardless. At the very least they seem to have some code reuse going on, and have the offers separated into distinct methods/objects. Come to think of it, that code could look far worse.

Maybe I'm just jaded because the core "god" system in my company is extremely configurable, and all sorts of cool offers can be created on the fly, but the code itself is horrible and we're reliant on a subcontractor for more than a decade to keep the mess together. The accomplishment of the year was to get all that PL/SQL code finally on version control. :hurr:

# ? Mar 29, 2012 08:23

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

pigdog posted:

I hope not to sound like a contrarian jerk... But aside from general badness and not putting start and expiry dates in the code it's not necessarily a bad idea to hard-code promotions like that, depending on circumstances. Making a nice administration UI for something like that an order of magnitude more work. If an offer is complex, then making it configurable is even more complex. If it's a small company, the number of offers and options are limited enough to have a grasp of, and it's easy to deploy new versions, then it may very well be easier to tell the VB programmer to update the code and redeploy. As opposed to spending a lot more resources, and dealing with a lot more integration, security and maintenance issues, to develop an interface for non-technical people who may end up needing the help of technical people regardless. At the very least they seem to have some code reuse going on, and have the offers separated into distinct methods/objects. Come to think of it, that code could look far worse.

Maybe I'm just jaded because the core "god" system in my company is extremely configurable, and all sorts of cool offers can be created on the fly, but the code itself is horrible and we're reliant on a subcontractor for more than a decade to keep the mess together. The accomplishment of the year was to get all that PL/SQL code finally on version control.

I agree with you, but there are tons of better ways than listing out every single discount.

At the very least there should be a separate text file with a hash of all possible "coupon" names. Then in each hash is an object that has something like, "description, type, value, num_discount_times" as strings and maybe an array "list_of_items" that contains a list of items that can use this coupon.

'description' is just a description of the coupon, 'type' is some ENUM that contains what kind of discount it is maybe, [percent_off, price_off, free], 'value' is the value based off what type is. E.g. if type = percent_off and value = 10, then its a 10% off coupon. 'num_discount_times' is another ENUM that determines how many times you should apply this coupon to an order which maybe contains [per_item, only_once]. I mean I've probably accounted for a large portion of what most companies want in their coupon/discount config.

So now that you have this hash, you have very simple if/then or switch statements in your actual controller code that can apply any type of discount written by the data in the hash, and if for example you need to handle expiration you can add a new property to the object and then rewrite code in the controller to handle expiration.

I don't think there needs to be a front-end to the discounts but writing every single possible coupon out like that either means the previous developer was extremely lazy or just not a decent developer.

Strong Sauce fucked around with this message at 09:02 on Mar 29, 2012

# ? Mar 29, 2012 08:59

X-BUM-RAIDER-X: May 7, 2008

Sulk posted:

Yes. I'm not even joking. I think most of this code is theirs, or they could have redesigned it if they had the knowledge (I don't know how because I don't know SQL/VB).

I'm talking about in IRC and what I'm getting paid is insultingly low (pick a number and reduce that) and it was supposed to be because I have no portfolio and I don't know any Ruby/Rails, but at this point I'm not doing either.

Haha goddamn. That reminds me of when I had a university group project years ago to make a javascript based online poker room, and the other guys in the group gave each card in the deck it's own variable. This meant looping over the deck was impossible and instead of going back and fixing their poo poo so it was actually maintainable, when they needed to carry out an operation on the entire deck, they copied pasted the code 52 times and changed the variable names to carry out the operation on each card individually. It goes without saying that their code was horrendously broken by the end and resulted in some long overnight sessions.

# ? Mar 29, 2012 12:48

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

Ari posted:

http://php.net/manual/en/language.variables.variable.php

I'm not sure what point you're making with this post. I'm aware of what PHP's variable variables are and what they do. My post was expressing the view that they are stupid whereas associative arrays accomplish the same thing and are not stupid.

FamDav posted:

Are those spaces between function name and args really a thing? .

What's wrong with spaces between the function name and arguments in a function definition? It makes sense to me. It makes it read more like a sentence.

Edit: Wait, I just noticed it also has spaces between function name and arguments in function invocations. Yeah that looks kind of weird to me too.

# ? Mar 29, 2012 14:46

pigdog: Apr 23, 2004; by Smythe

Sulk posted:

http://pastebin.com/WEBKaCWK

I hope I'm not crazy.

Come to think of it, this is absolutely a textbook case of code that needs refactoring, such as...

- introducing constants because gently caress me, those numbers
- sort those constants (numbers) out to bring out actual business logic in human readable fashion
- get rid of global variables
- ... wait what, these aren't just global, but global session variables? :gonk:

- Call <whateverDiscount>(Session("key"), pIdDbSessionCart) repeated a million times means abstraction that simply glares in your face
- which first demands refactoring static functions into object and methods
- and there's ever been a chance to split a piece of code up into meaningful parts that do one thing, then stuff like

code:

...
        End Select
End If
                                                                                   
'then, check stored session key code
'add key code specific offer functionality
....

is as big of a hint as there can be.

Etc.

However, I don't hate that piece of code. There are some good things in it. I like how they've used nice and meaningful function names, and have grouped up offer-specific functionality in them. So for all its faults and obsession with offer codes, the code, and the business logic it's trying to convey, is almost readable even at this state. With a good refactoring it could probably flow rather nicely.

Meanwhile, since the offers are separated into methods that work on (presumably) the shopping basket object, they can contain arbitrarily complex rules. Not just static discount of x% or $x, but stuff like "on Fridays offer every 3rd item free to all grocery items as long as the basket has a Pepsi in it, but only once a hour".

The biggest functional deficiency seems to be the lack of automatic expiry and application of offers, which would apparently be clumsily solved by deploying new versions whenever anything changes.

edit: Come to think of it, the functions implementing the discounts, such as xpTillerDiscount50(), may actually include the relevant time constraints in them, and only do anything if the offer isn't expired.

pigdog fucked around with this message at 16:30 on Mar 29, 2012

# ? Mar 29, 2012 15:07

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

Right. Going to a model where you have coupons in a database seems to be the wrong way to approach this. Coupons are pretty unique things, so having a list of "coupon types" isn't going to work out in the real world -- I would bet that you're going to have a lot of one-time coupon types, and at that point you're better off just specifying everything in the code itself. Of course, that code there isn't good by any means, you could certainly do better.

# ? Mar 29, 2012 15:33

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

"...one of the most highly regarded and expertly designed C++ library projects in the world."

# ? Mar 29, 2012 18:52

Internet Janitor: May 17, 2008; "That isn't the appropriate trash receptacle."

"The preprocessor isn't a powerful enough metaprogramming facility? Bullshit, I just have to-"

# ? Mar 29, 2012 19:01

That Turkey Story: Mar 30, 2003

Suspicious Dish posted:

"...one of the most highly regarded and expertly designed C++ library projects in the world."

Don't blame Boost, that's the only way you can do it in the preprocessor!

E: Also, that includes compiler workarounds.

# ? Mar 29, 2012 19:03

Dicky B: Mar 23, 2004

What were you expecting from a library designed specifically for working with the C preprocessor? It's going to be horrible no matter what.

I would say the library itself is the horror but presumably it has its uses

# ? Mar 29, 2012 19:11

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

Dicky B posted:

What were you expecting from a library designed specifically for working with the C preprocessor? It's going to be horrible no matter what.

I'm quite sure horribleness transcends language.

# ? Mar 29, 2012 20:55

hobbesmaster: Jan 28, 2008

Internet Janitor posted:

"The preprocessor isn't a powerful enough metaprogramming facility? Bullshit, I just have to-"

I like to imagine the entire library being written due to a conversation like this in a bar...

# ? Mar 29, 2012 21:30

shodanjr_gr: Nov 20, 2007

Suspicious Dish posted:

"...one of the most highly regarded and expertly designed C++ library projects in the world."

What the christ is this supposed to do?

# ? Mar 30, 2012 09:30

Look Around You: Jan 19, 2009

shodanjr_gr posted:

What the christ is this supposed to do?

According to this, it's supposed to allow for preprocessor metaprogramming, to reduce the amount of boilerplate code involved in template metaprogramming and generic programming in C++. By moving it to the preprocessor.

:stare:

# ? Mar 30, 2012 09:53

shodanjr_gr: Nov 20, 2007

Look Around You posted:

According to this, it's supposed to allow for preprocessor metaprogramming, to reduce the amount of boilerplate code involved in template metaprogramming and generic programming in C++. By moving it to the preprocessor.

You know, when I read stuff like this I'm kinda torn. On the one hand I'm impressed by the flexibility of C++, the preprocessor and STL. On the other hand, :stare:

# ? Mar 30, 2012 12:14

Dicky B: Mar 23, 2004

This example is pretty nifty http://codepad.org/tU1ZUCJ5

If you can't tell, the FOR_EACH at the end is saying "run this CATCH macro for each of the tokens in BUILTIN_TYPES", and generates a catch() statement for each of them.

# ? Mar 30, 2012 13:26

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

Philip Greenspun was right.

# ? Mar 30, 2012 13:34

SavageMessiah: Jan 28, 2009; Emotionally drained and spookified; Toilet Rascal

GHC-Tuple posted:

Manuel says: Including one more declaration gives a segmentation fault.

Ha. And I always thought of GHC as being pretty drat good.

# ? Mar 30, 2012 14:41

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

SavageMessiah posted:

Ha. And I always thought of GHC as being pretty drat good.

It is. GHC segfaulting on terrible code is a feature.

# ? Mar 30, 2012 15:15

That Turkey Story: Mar 30, 2003

If you think preprocessor metaprogramming is a horror, you can check out my "horror" contributed to Boost for representing binary literals in C++98/03 and I guess technically C though I haven't tested it:

The docs.
The code.

Now that C++11 has user-defined literals, I'll probably update the implementation for compilers that can support it, but that gigantic macro is still going to be there for old compilers.

Edit: To be clear, the reason why preprocessor metaprogramming libraries are so hairy is because the preprocessor can neither recurse nor loop. Since it can't, the libraries have to be written with seemingly redundant macros. Boost.Preprocessor exists as a way to simulate algorithms and datastructures so that higher-level code can "loop" and do other high-level things without needing to have any redundancy themselves.

Keep in mind that the only thing you can do inside of a macro definition is expand other macros, concatenate tokens, turn arguments into strings, and ... welp that's basically it, and that last one doesn't really help during preprocessing (note that I didn't say add, subtract, do bitwise operations, branch, etc. because even those have to be simulated). From those simple pieces of functionality you can produce a library that simulates several different datastructures and algorithms -- Paul Mensonides's Chaos library even has lambda functions.

That Turkey Story fucked around with this message at 18:34 on Mar 30, 2012

# ? Mar 30, 2012 17:48

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

I've always thought of C++ as being like a futuristic, rocket-powered wheelchair. It's got all sorts of really cool equipment and features and abilities, but you've always got the feeling in the back of your mind that you're still crippled in some way.

# ? Mar 30, 2012 18:25

Adbot: ADBOT LOVES YOU

# ? May 15, 2024 17:58

Fiend: Dec 2, 2001

wwb posted:

code:

        if (Request.QueryString["UserName"] != null && Request.QueryString["UserName"].ToString().Length > 0)
        {
            UserName = Request.QueryString["UserName"].ToString();
        }
        if (Request.QueryString["Password"] != null && Request.QueryString["Password"].ToString().Length > 0)
        {
            Password = Request.QueryString["Password"].ToString();
        }
        if (UserName == "" & Password == "")
        {
            lblResponse.Text = "Invalid";
            return;
        }
Response.Clear();

        ObjAdminUser = new clsAdminUser();
        dsUser = ObjAdminUser.GetResults("select * from Users where UserName = '" + UserName + "'");
        if (dsUser.Tables[0].Rows.Count > 0 && dsUser != null)
        {
            Response.Write ("Username already exists!");
        }
        else
        {
            UserID = Save_Value();
            if (UserID == 1)
            {
                Response.Write("Success");
                Session["FirstName"] = FirstName;
                Session["LastInitial"] = LastName;
                Session["UserName"] = UserName;
                Session["UserID"] = UserID;
            }
            else
                Response.Write("Failure");
        }

I'm not sure how far I want to follow this rabbit hole.

let me see...

So, passing credentials in a querystring
Only requiring Username to authenticate
Passing input unchecked to SQL.
Looks pretty solid to me!

?UserName='delete%20from%20Users

# ? Mar 30, 2012 19:49

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 »