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!

«‹›1502 »

Medieval Medic: Sep 8, 2011

Kim Jong III posted:

Just don't do that when you're getting paid to write code

I am quite tired at the moment, my brain is not working too well, I know it is a horror but what exactly did I do wrong? I am guessing I should have probably put it in a separate class dedicated to managing Paso.

A MIRACLE posted:

or this

code:

foreach
{
  sqlConnection.open();
  //  ...
  sqlConnection.close();
}

This one is particularly bad, I agree, I actually just noticed it now that I pasted it to the forum. Everyone else in the class was in 3 man groups, and I am alone, so all my time went to debugging and making sure it works.

# ? Dec 9, 2011 03:00

Adbot: ADBOT LOVES YOU

# ? May 2, 2024 01:21

Janitor Prime: Jan 22, 2004; PC LOAD LETTER

What da fuck does that mean; Fun Shoe

http://en.wikipedia.org/wiki/SQL_injection

# ? Dec 9, 2011 03:15

EpicCodeMonkey: Feb 19, 2011

Medieval Medic posted:

I am quite tired at the moment, my brain is not working too well, I know it is a horror but what exactly did I do wrong?

You're constructing the query string using values supplied elsewhere - that are then interpreted as part of the query depending on the values entered. It's a classical SQL injection vulnerability as the user could supply values such that it performs arbitrary operations including data modification or even deletion.

And yes, don't open/close the database a billion times - open it once then close it at the end. If you're worried about cleanup, use a "using" statement, or whatever similar construct your language supplies (if any).

Edit: The solution is, I believe, to use Parameterised Queries, where the query string is fixed with placeholders for the various user supplied components, and the user data then applied separately afterwards through parameter lists to ensure that the user can't alter the query.

EpicCodeMonkey fucked around with this message at 03:18 on Dec 9, 2011

# ? Dec 9, 2011 03:15

Wheany: Mar 17, 2006; Spinya^{ha^{ha^haha}ha}ha_{ha_{ha_haha}ha}ha!; Doctor Rope

Goddamn stop loving using inline styles fuuuuck!

How is this

code:

$('header_1').addEvent('mouseover', function(ev){
	this.style.cursor = 'pointer';
});
$('header_1').setStyle('text-decoration', 'underline');
$('header_1').setStyle('color', 'blue');

better than, say

js:

code:

$('header_1').addClass('clickable');

css:

code:

.clickable {
	cursor:pointer;
	text-decoration: underline;
	color:blue;
}

"Until now this was the only way to get juice from an orange"

# ? Dec 9, 2011 10:40

Deus Rex: Mar 5, 2005

Wheany posted:

Goddamn stop loving using inline styles fuuuuck!

How is this
code:
$('header_1').addEvent('mouseover', function(ev){
	this.style.cursor = 'pointer';
});
$('header_1').setStyle('text-decoration', 'underline');
$('header_1').setStyle('color', 'blue');
better than, say

js:
code:
$('header_1').addClass('clickable');
css:
code:
.clickable {
	cursor:pointer;
	text-decoration: underline;
	color:blue;
}
"Until now this was the only way to get juice from an orange"

I took over maintenance on a site that does this everywhere and I want to gouge my loving eyes out

the best is when several elements are generated server-side with identical inline styles rather than applying a class to all of them

# ? Dec 9, 2011 10:44

taqueso: Mar 8, 2004

Wheany posted:

Goddamn stop loving using inline styles fuuuuck!

"Until now this was the only way to get juice from an orange"

Does your notepad open more than 1 file at a time? Didn't think so. Got to be efficient, man.

# ? Dec 9, 2011 14:56

Johnny Cache Hit: Oct 17, 2011

EpicCodeMonkey posted:

Edit: The solution is, I believe, to use Parameterised Queries, where the query string is fixed with placeholders for the various user supplied components, and the user data then applied separately afterwards through parameter lists to ensure that the user can't alter the query.

Yes, parameterized queries are the way to go. They are completely resistant to SQL injection, plus are prepared exactly once by the SQL server, so repeated queries don't have to be parsed & planned each time. This means it is much safer and (potentially) faster.

An inferior solution would be to sanitize the query string, but it is slower and you will screw it up, because trying to remember all the annoying edge-cases and DB-specific behavior is really hard.

SQL by concatenation is never right, and according to a few friends that hire programmers, is a very fast way to fail a pre-hiring code test.

edit: but if you are looping over a SQL query anyways you might want to stop and ask yourself if your DB has a better way to bulk insert data

Johnny Cache Hit fucked around with this message at 17:38 on Dec 9, 2011

# ? Dec 9, 2011 16:29

Clavius: Oct 21, 2007; Howdy!

Wheany posted:

"Until now this was the only way to get juice from an orange"

Nice, the cursor pointer style is on a hover event :regd09:

# ? Dec 9, 2011 16:38

Frozen Peach: Aug 25, 2004; garbage man from a garbage can

I love parameterized queries, but there's one thing that I've never been able to find a "good" solution for that's likely a coding horror that still lets me used parameterized queries.

A lot of times I need to do a rather large transaction, with several inserts, then selects that depend on results of the inserts, and it's a big complicated deal. There are three options:

A) Open a command that starts a transaction. Then make another command that does each insert/select, parameterized as is "proper", keep looping those commands until finished, and finally sending a commit transaction.

I don't like this, because I try to avoid transactions, because it requires always making sure to cancel or commit them constantly, and if something goes wrong mid-transaction and neither the commit nor the cancel get sent, then there are dangling transactions. There just seems to be too much room for error there, or maybe I'm just being paranoid.

B) Build one query string using StringBuilder, clean and sanitize data (which is easy, I have data checks everywhere in my app and sanitize everything on the fly), and then send that as one command. Each single command is automatically sent as a transaction so if 1 of the 15 queries involved fails, the whole thing fails. There's no clean up. It just works. Only problem is that it's theoretically open to injection attacks.

C) Use the StringBuilder method, but now also adding "@dataXX" where XX is the number of each insert, and end up with a shitload of parameters that get passed in. The whole thing just feels cludgey like I'm forcing parameters to work somehow they're not meant to.

So basically it comes down to transactions and dealing cleanup, sql injection risks, or parameter hell.

I tend to opt for B, just because it's the easiest of the 3... and all I do it in are in-house only apps that SQL Injection really isn't something we need to worry about much. Either way, there doesn't seem to be a good option that doesn't reek of a coding horror... unless I'm just stupid. Which could be as well.

# ? Dec 9, 2011 19:56

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

Frozen-Solid posted:

I love parameterized queries, but there's one thing that I've never been able to find a "good" solution for that's likely a coding horror that still lets me used parameterized queries.

"I don't like this, because I try to avoid transactions..." I'm not a DBA but your worries about transactions sound overblown. The use of transactions is widely recommended for a reason. I'm sure any problems with "dangling transactions" are surmountable. And are rather less of a problem, and less likely, than ending up with inconsistently applied changes to your database.

Maybe you could do what you want to do using stored procedures. Then you could put the logic behind your sequence of queries into the stored procedure and you wold only have to issue one statement, the CALL statement with all the parameters in it.

# ? Dec 9, 2011 20:06

Frozen Peach: Aug 25, 2004; garbage man from a garbage can

Hammerite posted:

"I don't like this, because I try to avoid transactions..." I'm not a DBA but your worries about transactions sound overblown. The use of transactions is widely recommended for a reason.

Yeah, I know. I'm the coding horror. :smith:

I think part of my issue is that with the transaction it means I'm making several command, and sending each one one at a time to the database for it to handle, then committing. Whereas the other ways it's one single command all in one chunk, and just letting the DB deal with it. It just seems cleaner to me to say "hey DB take care of this huge chunk" than sending single messages in quick succession.

None of the options really seem clean to me, even though the transaction one is the most "proper".

# ? Dec 9, 2011 20:40

wwb: Aug 17, 2004

Well, C is what most ORMs do when batching commands.

I'll also add it is possible to nest transactions. I myself tend to send multiple commands over a single connection, rudimentary tests indicate that is as fast as building one massive command and alot easier to code/manage. Hell, opening/closing per command doesn't hurt that much as long as you aren't writing to JET; connection pooling is awesome.

# ? Dec 9, 2011 21:42

adaz: Mar 7, 2009

This is like the 4th time I've run into a Try/Catch/finally block that has nothing in the catch. Why jesus why, it's like someone got upset that On error Resume Next doesn't exist :suicide:

# ? Dec 9, 2011 21:47

Goat Bastard: Oct 20, 2004

Frozen-Solid posted:

I tend to opt for B, just because it's the easiest of the 3... and all I do it in are in-house only apps that SQL Injection really isn't something we need to worry about much. Either way, there doesn't seem to be a good option that doesn't reek of a coding horror... unless I'm just stupid. Which could be as well.

Yea sorry, but Option B is easily the worst of those three choices. There's nothing hard about using bind variables, and nothing hard about closing a transaction in a finally block or whatever you have in your language of choice.

# ? Dec 9, 2011 22:14

wwb: Aug 17, 2004

adaz posted:

This is like the 4th time I've run into a Try/Catch/finally block that has nothing in the catch. Why jesus why, it's like someone got upset that On error Resume Next doesn't exist

That isn't horribly bad code there -- they are cleaning up using the finally block. VB.NET didn't have using(){} until the VS2010 version AFAIK so it makes sense.

# ? Dec 9, 2011 22:18

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

wwb posted:

That isn't horribly bad code there -- they are cleaning up using the finally block. VB.NET didn't have using(){} until the VS2010 version AFAIK so it makes sense.

I believe you can have a Try...Finally without the catch in the middle, so you don't have to silently ignore exceptions.

# ? Dec 9, 2011 22:20

Opinion Haver: Apr 9, 2007

God I love machine translation.

# ? Dec 9, 2011 22:23

adaz: Mar 7, 2009

Suspicious Dish posted:

I believe you can have a Try...Finally without the catch in the middle, so you don't have to silently ignore exceptions.

Yep you most definitely can have a Try/Finally without the catch if you don't want to silently ignore the exceptions.

# ? Dec 9, 2011 22:26

wwb: Aug 17, 2004

Yeah, somehow looked past that. Catch definitely shouldn't be there . . .

# ? Dec 9, 2011 23:19

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

Luckily here we have a policy against empty catch blocks!

# ? Dec 9, 2011 23:23

pigdog: Apr 23, 2004; by Smythe

Plorkyeran posted:

Luckily here we have a policy against empty catch blocks!

Is this a trick post because that's not the biggest problem here.

# ? Dec 9, 2011 23:29

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

it's okay the average lines per method is under one hundred

# ? Dec 9, 2011 23:41

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

the biggest problem is definitely the lack of a newline at the end of the file

# ? Dec 9, 2011 23:41

Frozen Peach: Aug 25, 2004; garbage man from a garbage can

Yup. I'm the coding horror. I thought transactions were complicated and convoluted because I hadn't discovered the SqlTransaction class yet. :downs:

I've now refactored all my code to parameters and it's even more sexy than anything I've tried to date! Exciting!

Now if I could just come up with a way of doing parameters on IN statements without it being a huge, ugly, kludge. I'm going to attempt using a table variable like in this example: http://stackoverflow.com/questions/337704/parameterizing-a-sql-in-clause/337864#337864

But I don't think that'll work in SQL 2000. It's not a HUGE deal because everything I do with an IN statement is straight out of select boxes that the user can't edit. So unless someone actually hex edits my application it's safe. That still doesn't mean I like it though.

# ? Dec 10, 2011 01:12

blorpy: Jan 5, 2005

It's 2011, please stop writing raw SQL, everybody. If you would, look up an ORM for whatever framework you're using. There's literally no reason to be operating with your DB directly (unless you're the author a DB adapter for an ORM, in which case go nuts)

# ? Dec 10, 2011 01:51

jonjonaug: Mar 26, 2010; by Lowtax

Markov Chain Chomp posted:

There's literally no reason to be operating with your DB directly

Are we just talking about web design or in general because I can think of a lot of applications for interacting with a database.

# ? Dec 10, 2011 01:57

blorpy: Jan 5, 2005

jonjonaug posted:

Are we just talking about web design or in general because I can think of a lot of applications for interacting with a database.

In general, and I agree, there are plenty of cases where you should interact with a database -- through an ORM.

# ? Dec 10, 2011 02:00

baquerd: Jul 2, 2007; by FactsAreUseless

Markov Chain Chomp posted:

In general, and I agree, there are plenty of cases where you should interact with a database -- through an ORM.

There is no good reason to add the added complexity of an ORM for many simple tasks. It may be "nicer" in a way, but it adds another layer of failure and checks that need to be done.

Suppose we want to read a table and display it to the page. Why ORM?

# ? Dec 10, 2011 02:06

wwb: Aug 17, 2004

I can think of lots of neat set-based operations that are horribly ugly or expensive in an ORM. I can think of lots of lightweight apps that do just fine without the ceremony of an ORM. I can think of lots of apps I made worse by adding an ORM. Choose the right tool for the job -- and sometimes the right tool is raw sql.

If I was to make any brash-assed blanket statement about data access in 2012, it would be that your default answer for data storage should not be relational. Document databases make so much more sense for most apps and generally dodge the whole issue of needing or not needing an ORM in the first place.

@Frozen-Solid -- sql in clauses are kinda sucky to deal with. Presuming you don't have a realistic danger of hitting the length limits, the best way I've seen to deal with them is to generate the SQL statement in on the client, take a collection as a parameter and loop through that to create parameter names and bind the parameters to the command.

EDIT:
Holy poo poo. Speaking of coding horrors, lets see Joel's way of creating a in query, from Frozen-Solid's SO link:

Joel Splosky posted:

select * from Tags
where '|ruby|rails|scruffy|rubyonrails|'
like '%|' + Name + '|%'

What the christ. I should suck at computer so I can go sell a bug tracker.

Please use the 2nd answer. Works great

wwb fucked around with this message at 02:22 on Dec 10, 2011

# ? Dec 10, 2011 02:13

blorpy: Jan 5, 2005

baquerd posted:

There is no good reason to add the added complexity of an ORM for many simple tasks. It may be "nicer" in a way, but it adds another layer of failure and checks that need to be done.

Suppose we want to read a table and display it to the page. Why ORM?

The rule of thumb is to prefer more abstraction, not less, unless you have a very particular reason for doing so.

# ? Dec 10, 2011 02:28

blorpy: Jan 5, 2005

wwb posted:

I can think of lots of neat set-based operations that are horribly ugly or expensive in an ORM. I can think of lots of lightweight apps that do just fine without the ceremony of an ORM. I can think of lots of apps I made worse by adding an ORM. Choose the right tool for the job -- and sometimes the right tool is raw sql.

See above. This is C versus assembly all over again. Having more abstraction makes everyone's lives better. Also, for what it's worth, some ORMs do actually include the ability the make nearly full use of what you can do with raw SQL. If yours doesn't, why not just patch it?

By the way "thinking of" apps that shouldnt have an ORM sounds suspiciously like premature optimization to me. This is pretty much always the case when you start talking about sacrificing portability and maintainability for "performance"

quote:

If I was to make any brash-assed blanket statement about data access in 2012, it would be that your default answer for data storage should not be relational. Document databases make so much more sense for most apps and generally dodge the whole issue of needing or not needing an ORM in the first place.

Completely irrelevant because I wasn't talking about data access, I was talking about SQL.

# ? Dec 10, 2011 02:33

Johnny Cache Hit: Oct 17, 2011

Markov Chain Chomp posted:

It's 2011, please stop writing raw SQL, everybody. If you would, look up an ORM for whatever framework you're using. There's literally no reason to be operating with your DB directly (unless you're the author a DB adapter for an ORM, in which case go nuts)

ORMs are classic examples of what Joel Spolsky means when he says all non-trivial abstractions leak.

That's not to say that ORMs aren't, in very many cases, the best thing since sliced bread. Especially if you are doing CRUD webapps. Oh god, ORM's are great at CRUD. But when the ORM is generating nasty, slow, or completely broken SQL you're going to spend time peeling back abstractions, and that takes time.

Hell, for Django's ORM, consider:

code:

# Don't do this to see if something exists...
if some_queryset:
    pass

# ... do this...
if some_queryset.exists():
    pass

#... BUT not every time; sometimes, do this:
if some_queryset:
    #why?
    for x in some_queryset:
        print x.foo

And that's just something to worry about with the CRUD apps :smith:

# ? Dec 10, 2011 02:39

Cubiks: Aug 31, 2005; I wish I had something witty to put here...

Markov Chain Chomp posted:

The rule of thumb is to prefer more abstraction, not less, unless you have a very particular reason for doing so.

I'd say it would be better to use a simpler solution, not just add more abstraction. Unless you really enjoy enterprise java with its endless nested ThingFactoryFactoryImplementationFactories. Can't get much more abstraction!

I'll admit, use ORM when it makes sense: when you have very clear Object to Table mappings and no possibility of strange queries now or in the future. But as wwb says, trying to do any useful, optimized SQL (i.e. one of the reasons you use a relational db in the first place) is really painful in most ORMs I've seen.

# ? Dec 10, 2011 02:39

blorpy: Jan 5, 2005

Kim Jong III posted:

ORMs are classic examples of what Joel Spolsky means when he says all non-trivial abstractions leak.

That's not to say that ORMs aren't, in very many cases, the best thing since sliced bread. Especially if you are doing CRUD webapps. Oh god, ORM's are great at CRUD. But when the ORM is generating nasty, slow, or completely broken SQL you're going to spend time peeling back abstractions, and that takes time.

Hell, for Django's ORM, consider:
code:
# Don't do this to see if something exists...
if some_queryset:
    pass

# ... do this...
if some_queryset.exists():
    pass

#... BUT not every time; sometimes, do this:
if some_queryset:
    #why?
    for x in some_queryset:
        print x.foo
And that's just something to worry about with the CRUD apps

I didn't say you wouldn't have to know any SQL or not understand how an ORM maps to SQL. It's still useful to understand how it works. It's just way better to use the ORM in actual code, though (and small bits of sql where the ORM won't let you do something)

# ? Dec 10, 2011 02:43

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

baquerd posted:

Suppose we want to read a table and display it to the page. Why ORM?

It saves time and effort. How many times have you written code that:

1) Sets up a SQL command
2) Executes the command and gets back a dataset (with proper exception handling, etc)
3) Parses the dataset into an object model of some kind

# ? Dec 10, 2011 02:44

blorpy: Jan 5, 2005

Cubiks posted:

I'd say it would be better to use a simpler solution, not just add more abstraction. Unless you really enjoy enterprise java with its endless nested ThingFactoryFactoryImplementationFactories. Can't get much more abstraction!

Strawman. I can easily counter that with "well then why aren't you writing all your programs in raw assembly?". Yawn.

quote:

I'll admit, use ORM when it makes sense: when you have very clear Object to Table mappings and no possibility of strange queries now or in the future. But as wwb says, trying to do any useful, optimized SQL (i.e. one of the reasons you use a relational db in the first place) is really painful in most ORMs I've seen.

Just one example. Please, that's all I'm asking.

Also, nothing forbids you from using both.

# ? Dec 10, 2011 02:45

npe: Oct 15, 2004

If we're just tossing out blanket statements for all applications out of our rear end, then I'll use this opportunity to say your application shouldn't be concocting *any* queries on the fly ever - not even via an ORM. Use stored procedures and marshal your complex types. Keep the SQL in the db! :mad:

(If this is not appealing to you then you should be using a document store and not a relational database anyhow.)

*Note: I don't actually believe this. As it turns out, many application domains are completely different from each other and there are very different considerations for each!

# ? Dec 10, 2011 02:48

Johnny Cache Hit: Oct 17, 2011

npe posted:

If we're just tossing out blanket statements for all applications out of our rear end, then I'll use this opportunity to say your application shouldn't be concocting *any* queries on the fly ever - not even via an ORM. Use stored procedures and marshal your complex types. Keep the SQL in the db!

I've seen this in practice.

:smithicide:

# ? Dec 10, 2011 02:49

blorpy: Jan 5, 2005

npe posted:

If we're just tossing out blanket statements for all applications out of our rear end, then I'll use this opportunity to say your application shouldn't be concocting *any* queries on the fly ever - not even via an ORM. Use stored procedures and marshal your complex types. Keep the SQL in the db!

(If this is not appealing to you then you should be using a document store and not a relational database anyhow.)

*Note: I don't actually believe this. As it turns out, many application domains are completely different from each other and there are very different considerations for each!

Actually, as it turns out, an ORM is basically always the right choice unless you have enough experience to know different, and in that case you're not someone who's asking questions about SQL on the internet.

# ? Dec 10, 2011 02:50

Adbot: ADBOT LOVES YOU

# ? May 2, 2024 01:21

npe: Oct 15, 2004

Kim Jong III posted:

I've seen this in practice.

I wasn't seriously advocating that it be used everywhere (like, it would be stupid for a web app).

However, it can be very appropriate if you are already committed to a significant amount of application logic residing in the database to begin with (which is true for applications that rely heavily on certain Oracle components, for example). At my current job we do it and it's very easy, since I'm able to treat the database as just another callable component just like any other library. The implementation of the table structures is completely hidden from me, and whenever there's some whack-rear end restructuring of things, my changes (if any) are reduced to contract changes in the API.

# ? Dec 10, 2011 02:59

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!

«‹›1502 »