Oracle: Ass first design pattern: Just "Orrible"

Posted on 7/13/2006 @ 8:54 AM in #Vanilla .NET by | Feedback | 1768 views

Pardon the title, but this is quite Orrible.

You've heard of "Head first design patterns" .. right?

Well, welcome to Oracle - the home of Ass first design patterns. (See cover) but NSFW (Not safe for work)

But I saw something so awful that I just had to blog about it.

So I ran into this sample from otn.oracle.com.


Web Service using ODP.NET Sample
This sample application demonstrates how ODP.NET can be used in a .NET Web Service. This application also shows how ASP.NET DataGrid can be used in conjunction with .NET Web Services and ODP.NET.
Download Now (ZIP, 175KB)
Readme
Source


Can I just say, YUCK. Check this out, here are the awful downsides of the supplied "sample" & "ideal" code, straight from "otn.oracle.com" (ooh !!).

1. So there is no layered design. The OracleConnection sits as a private member variable of the webservice. Fine this is a sample code thingie - thats allright, frictionless surface example, I understand that, this is pardonable, but wait it gets better.
2. Of course the webservice (ASMX) that runs on a webserver, that holds the OracleConnection as a "protected shared" global to the class variable - it's connection opening and closing - is controlled by the remote client using two WebMethods - OpenConnection/CloseConnection. (And the code of these two makes me puke)
3. Here is a minor issue which I personally could give a rats head about "Variable Naming", as long as it's not horrific, but who names their methods in Camel case? And half their variables in hungarian and the rest of the half in god knows what. But if the core code were functionally okay, and understandable enough - I'd say - who cares. But WTF is "ocb"? Oh OracleCommandBuilder - wait this is getting bettter worse as I type.
4. So there are two WebMethods "OpenConnection" and "CloseConnection" right? Oh wait it gets bettter worse. The CloseConnection is called from the client side ASPX. You think that is bad? Oh you haven't seen the real crappy part yet - The CloseConnection WebMethod is called using the OnClick of a button on an ASPX. So not only is the connection open for - well - if you went for lunch for instance, but the connection remains open if I close the browser and go home, and then your session times out (20 minutes), and then two cycles of garbage collector run (approx 8-10 minutes), and finally the OracleConnection is finalized. YUCK.
5. So how does CloseConnection work? Check this code snippet out -

<WebMethod()> Public Function closeConnection() As Boolean
  Try
      con.Dispose()
      con.Close()
  Catch ex As Exception
  Finally
      'Set connected to false
      connected = False
  End Try
  closeConnection = connected
End Function

So Dispose is before close, and if an exception occurs - just ignore it? eat up the exception/do nothing with it?
6. And whats up with that "connected = False" . Oh wait, that is a global boolean - oh this is getting even worse. Why not use .. umm .. the CONNECTIONSTATE !!
7. And, oh look there is a WebMethod that runs DDL - Yes sir, Create Table/Drop Table, right over a web service.
8. And why are all the global variables "Protected Shared" ???? Oh wait, I know why - because the code sucks !!!
9. And whatsup with "Creating an ocb - Oh wait that is OracleConnectionBuilder" - and then not using it - TWICE? Oh god !!!
10. Oh look, no parameterized SQL, concatenation of strings --- Ooooh !! I love the smell of Injection attack this time of night.
11. And I love their connection string -

conStr = New String("User Id=" + username + ";Password=" + password + ";Data Source=" + datasource + ";")

can u say - "STRINGBUILDER" ??
12. Oh wait dude, look at the full code -

 <WebMethod()> Public Function OpenConnection(ByVal username As String, ByVal password As String, ByVal datasource As String) As Boolean
     Try
         'Create OracleConnection instance
         con = New OracleConnection()
         'Create the connect string
         conStr = New String("User Id=" + username + ";Password=" + password + ";Data Source=" + datasource + ";")
         'Set the connect String
         con.ConnectionString = conStr
         'Try to open the connection
         con.Open()
         'Set connected to true
         connected = True
     Catch ex As Exception
         'An error ocurred. Set connected to false
         connected = False
     End Try
     'Return the connection status True or False
     OpenConnection = connected
 End Function

So let me get this straight, the remote user is passing in a userName, passWord, and dataSource over a webservice? OH wow, this code is smelling worse than an anchovy's smelly bundt (cake). Seriously, this code is making me puke. And the exception is being eaten, and again that "connected" boolean.

Okay, in about a 2 page code sample, in 5 minutes, I could spot 12 serious shortcomings. And I yet haven't even looked at the client code in depth. Now here is the icing on the cake.

Check the title of the page I got this sample from -- "Advanced ODP.NET Samples". I am sorry Oracle, but if this is the quality of your "advanced code samples" on OTN, I am scared to ask what might be inside ODP.NET.

Oh and don't think I found this page at an obscure corner on Oracle.Com. Try this -

1. Go to http://otn.oracle.com
2. On the top menu click on Sample Code -> ODP.NET
3. Click on "Advanced ODP.NET Samples", what I described is Example #2, the rest aren't much better either.

This code has seriously damaged my fragile mind. Sahil must get some sleep. Oh my, this has to be a dream!!

Sound off but keep it civil:

Older comments..


On 7/13/2006 9:01:57 AM Adam said ..
You're definitely correct, this code is really bad. Though you're off again on the StringBuilder comment (the compiler turns that into string.Concat which is better than StringBuilder), so there's 11 problems.


On 7/13/2006 9:30:07 AM Ilan Assayag said ..
I half laughed - half cried all the way through reading this post. If it wasn't so sad I'd say I really enjoyed it ;-)


Thanks man!


On 7/13/2006 10:37:07 AM Eber Irigoyen said ..
that's way too funny... perfect candidate for the daily WTF


On 7/13/2006 11:40:03 AM noname said ..
the examples are just to explain how to use ODP.NET and not built to implement the best practices in code

I saw millions of webcasts on msdn and they all assign username/passsword for sqlserver as "sa"


take it easy dude


On 7/13/2006 12:26:56 PM Jun Meng said ..
That may be a trick from Oracle: Look, how ugly .NET is ... so Java is better than .NET


On 7/13/2006 2:18:10 PM Sahil Malik said ..
Adam -

Will the compiler convert it into string.concat even if there are variables in the middle? I'm guessing no - but maybe you can verify.

Sahil


On 7/13/2006 2:18:35 PM Sahil Malik said ..
Ilan - I agree man, this code sample is laughably atrocious :)


On 7/13/2006 2:19:04 PM Sahil Malik said ..
Eber,

How do I nominate this blogpost for the Daily WTF?

SM


On 7/13/2006 2:19:59 PM Sahil Malik said ..
noname'd Orrible employee - Get a life man, I have worked with MS Press, and I am working with MSDN online. They are anal about such issues. Can you point me to a code sample on msdn.microsoft.com that is this "Orrible" ?


On 7/13/2006 2:20:27 PM Sahil Malik said ..
Jun, so the above code is Larry Ellison's idea for "World Peice" ? .. Thought so :)


On 7/13/2006 3:14:08 PM Adam said ..
Yeah, I pointed that out in your StringBuilder post a few days ago. A single-line string concatenation using the + operator whether it's constants or variables will use the string.Concat method.


On 7/13/2006 7:42:48 PM Kent Boogaart said ..
I, um . . . wow. Regardless of whether they demonstrated web services effectively or not, I think it’s downright irresponsible to do so in such a hideous manner. It’s analogous to teaching first grade mathematics using bad grammar.


On 7/14/2006 3:07:22 AM Sahil Malik said ..
Allrighty Adam - bear with me on this. Even though that is string.Concat, the C# 2.0 compiler won't take advantage of allocating multiple strings together as one string if there are variables in the mix.

Also note, this is a C# 2.0 specific thing.

Here is an example in 2.0 -

This code -

string username = "test";


string longString = "This" + "is" + "a " + username + ".";

Turns into -

.locals init (


[0] string text1,


[1] string text2)


L_0000: nop


L_0001: ldstr "test"


L_0006: stloc.0


L_0007: ldstr "Thisisa "


L_000c: ldloc.0


L_000d: ldstr "."


L_0012: call string string::Concat(string, string, string)

So the "Thisisa" was turned into a single literal, but the variable "username" remained on it's own, which was string::Concat'ed afterwards.

Now in the above example -

conStr = New String("User Id=" + username + ";Password=" + password + ";Data Source=" + datasource + ";")

It would be a lot better if they were to read that out of a .config file. But ignoring connection-string specific reasons, even in the above, in C# 2.0, you would have a highly fragmented string operation going on (just because the compiler will not clump together strings that are held in other variables).

So in effect, you will end up declaring upto 8 strings. IMO that's yucky.

Not to mention, in .NET 1.1 StringBuilder is a handsdown better choicea anyway.