Initial impression of openBD source code

Tags: OpenBD

Well I am not going to sugar coat this, in any way shape or form.

After having a good play with the openBD code now, have made some very nice tags that I would like to release.

But I am worried, this is a product that has been around for a number of years. But it is so not polished it is not funny. This is not to have a dig at New Atlanta, but to question some of the decisions made in openBD (previously BDJ2EE). The first is there has been no effort in minimising the amount of CSS / JS that is generated by openBD, this can become a fairly hefty price at some point in time.

Secondly, I can see that the interpretor is fast. But I can see total refactoring is needed. The main point is why they did not use the tried and true TLD approach. As you can see that could be the biggest mistake made, unless their is another way around it. For example consider this example.

<cfloop index="count"> </cfloop>

This is valid syntax, however the error message and checking for this tag is a little of. The message I get is this

Invalid attribute: Must contain at least one; QUERY/FROM/CONDITION/LIST

Well, it is not an invalid attribute. It is just missing the other required attributes. And doesn't reflect this.

And last, but not least. Is the reuse of objects, from what I can see so far is that the code has been thrown together as a project. And has not evolved into anything spectacular as of yet. Again this is not a dig at NewAtlanta and the team, but it does highlight some very serious code resuse problems.

For example, in openBD if you want to create a new Array you would do this.

cfArrayData myArray = new cfArrayData();

So what if I want to insert data into an element position within that Array, point blank you have to write more code than you should need to write.

However, the problem comes in code reuse. I know that with refactoring of code, you want to minimise the class as much as possible in size. But the approach that I see is more of a hinderance than anything that I could call good.

I know for a fact, that in Coldfusion 6/7/8 if I want to create an array. I just need to do this.

Array myArray = new Array();

And if I want to insert at a particular position?

I would just

Array.ArrayInsertAt(); or Array.ArrayAppend();

I think it is debatable, but in my opion if you have functionality that is there why not leverage it to its full potential?

I am not saying that openBD doesn't have potential, but I am surprised to find how bad the code really is.

Sorry guys, I am used to things being where they should be. And an object that is an Array, should have all methods like ArrayAppend() ArrayInsertAt(), ArrayLen to name a few in the same object that it belongs too.

Now correct me if I am wrong, New Atlanta's current approach is wrong. And I would love to see this become one of the things that is addressed fairly quickly.

TweetBacks


Comments (Moderated) | Add Comment

  • Matt Woodward's Gravatar First, let me just say that this illustrates one of the huge advantages of open source projects: people can look at the source code as you have, and we can have these discussions.

    The only other thing I'll really say at this point is that it's pretty easy to look at one particular piece of code and say how you would have done it differently, but what we don't know is all the decision-making processes that led up to the implementation. Not saying you're wrong, just saying without the whole history behind things it's impossible to see the whole picture.

    That being said, I go back to my original comment that the code is out there and available for all to see, and having discussions such as these is what will help OpenBD improve where it needs to, or at least we'll understand why things were implemented they way they were.
    # Posted By Matt Woodward | 5/22/08 7:41 AM
  • Andrew Scott's Gravatar Matt, you are so very right. The code is out there and it will be looked at with a magnifying glass, and the fact that it is out there. Shows that you are proud enough to invest the time and energy to make the product even better.

    I have been very vocal about Adobe needs to adopt the same approach, and change their business model. And given examples on how they could do that, which is along the lines of openBD and plugins:-)

    But having said that, I am proud to be of any help that I can to the project as well. If that means being critical, then you can rest assured that it is done to raise some eyebrows for a reason.
    # Posted By Andrew Scott | 5/22/08 7:46 AM
  • Adam Haskell's Gravatar What do you mean about the array object, looking at it I see get data and set data as well as addElement and addElementAt. These seem to be the methods you are looking for, I certainly could be wrong? I'd say over all I think the code is so-so my concern is more nitpicky with the way some of the code is written, run PMD or Findbugs on it and you will see.

    I would agree error reporting could be a bit better but the example above is not a good representation...Just a change in verbage would solve the issue, switching to TLD would not necessarily fix that...
    # Posted By Adam Haskell | 5/22/08 7:58 AM
  • Vince Bonfanti's Gravatar With the BD cfArrayData class, to insert or add data to the array you'd just do this:

    myArray.addElement() or myArray.addElementAt()

    Do you just not like the names of these methods? I'm confused on the point you're trying to make here.
    # Posted By Vince Bonfanti | 5/22/08 8:31 AM
  • Andrew Scott's Gravatar Adam: You might see get/set Data. And yes they are similar. But why would you duplicate the code? Why would you not have an Array class that has ArrayAppend?

    There is also a dedicated class for each of the following. arrayAppend, arrayAverage, arrayClear, arrayDeleteAt, arrayInsertAt, arrayIsEmpty.

    Why would I not have one class, with all those methods?

    Make it all consistant, if I want to insert data I don't call a method called setData. If I was to refactor this code, setData would be split into 2 methods. ArrayAppend and ArrayInsertAt. Now seeing as CFML has these as part of its language, it would be best to keep these in one class not 16 classes.

    Passing data objects back and forth, wasting memory.

    Does that make it a bit clearer:-)
    # Posted By Andrew Scott | 5/22/08 8:36 AM
  • Keith Woods's Gravatar Andrew, kudos to you for looking at the source code. I've been interested in this as well, but haven't really had the time to really crack anything open. You're a brave man for digging in!

    I'd like to ask though, that in the interest of helping out n00bs like myself, if you could please help us distinguish between issues like "The interpreter/compiler isn't doing what we think it should do," and "The actual under-the-hood implementation [the openBD server itself] needs to be refactored."

    There will always be poorly written code (needing refactoring) that works, and there'll always be well-written code that doesn't. I'm a little unclear in your points above as to whether the problems are in the actual craftsmanship/coding of the openBD project, or the server's adherence to our expected syntax rules.

    Hope this makes sense, and that you're game for elucidating a bit more!

    Thanks,

    kpw
    # Posted By Keith Woods | 5/22/08 8:36 AM
  • Joe Rinehart's Gravatar What is the "tried and true TLD approach?"
    # Posted By Joe Rinehart | 5/22/08 8:37 AM
  • Andrew Scott's Gravatar @Vince: I do not think you understood what I am saying. Instead of 16 classes, would it not be best to keep all array stuff in once class? After all Java does this with its array stuff, does it not?

    And to make it even clearer, CFML has ArrayInsertAt and now you want me to call addElementAt()? Do these 2 methods not do the same thing?

    Please read what I said, it is not a criticism. It is not about what the methods should or should not be called. It is about reducing the code as and memory as much as possible. Currently this fails on both counts.
    # Posted By Andrew Scott | 5/22/08 8:40 AM
  • Andrew Scott's Gravatar @Keith: I am in a position where we can refactor code, the problem is when do we actually stop:-) But in my opinion, if we have 2 methods that do exactly the same thing in 2 different areas, this is a classic example of refactoring the code to minimse code duplication.

    I didn't and wasn't looking for an answer from people on well you could use addElementAt() I had already worked that out. My point is that Java provides an Array Class, it has all methods and properties contained within that class.

    openBD, in its wisdom has 16 classes for all Array CFML functions. This can and should be refactored to be used within openBD to minimise the code reuse.

    If nobody understand that, then maybe people should look at Coldfusion (decompile the code) and also look at the Smith Project both of these do exactly what I refer too.

    Railo on the other hand, the last time I looked also minimised the code and utilised the fact that there is already an Array obect with a method called ArrayInsertAt(). So why would I write code and call it addElementAt()?

    Does nobody understand what code refactoring actually is, or even what minimisng code can do to an application?
    # Posted By Andrew Scott | 5/22/08 8:47 AM
  • Adam Haskell's Gravatar Oh oh oh oh I think I know what you are talking about now....rewind for a second.
    Each CFML function is its own class (this is true for other engines too if you monkey around with them you can see this). cfArrayData is an underlying implementation not ever entirely exposed to CFML in its raw form. These are really 2 different beasts we are talking about. There are some valid good reasons for each CFML function (or tag for that matter) to be its own class; the plugin architecture is one of those reasons (best I can tell).

    One more note Java passes by reference so throwing objects around is no big thing.
    # Posted By Adam Haskell | 5/22/08 8:48 AM
  • Andrew Scott's Gravatar @Joe, tried and true TLD approach is that you define what the attributes are. If you pass one that doesn't confirm to that then it should throw an error. I mean in openBD it could be a case that the code check for attributes needs to change to fix the descriptive error. But at the moment openBD has no way to tell the user they have tried to use an illegal tag attribute. I thought that was very clear. Oh well maybe not.
    # Posted By Andrew Scott | 5/22/08 8:49 AM
  • Andrew Scott's Gravatar @Adam, true about the reference. I did forget that.

    However, I have already looked at The Smith Project, I have already decompiled Coldfusion and Railo. So I do know they have one Array class with all methods inside that one class, and not 15 classes. And new methods to mimic, said cfml functions.

    Was that not clear either? Sorry, it is late here. But the point is that Java's own Array object has all necessary methods relating to an Array, Smith Project and Coldfusion do the same thing. openBD, and yes BD J2EE do not. I just don't understand that decisions to have more than one method in a class that does the same thing. Whats the point of extending and inherting?
    # Posted By Andrew Scott | 5/22/08 8:55 AM
  • Joe Rinehart's Gravatar @Andrew,

    Maybe it was me who wasn't clear. I don't know what "TLD" stands for, which was what I was asking :)
    # Posted By Joe Rinehart | 5/22/08 9:35 AM
  • Andrew Scott's Gravatar @Joe: Ok that seems to my fault as well, when you write JSP taglets. Or what is know as JSTL (Java Script Tag Library) you need to define a defintion file. This is called Tag Library Definition (TLD) I am thinking that is correct. This defintion tells the compiler, does it have a close tag, does it require to capture body content, it also sets the attributes for this tag.

    So if I do this in a JSTL

    &lt;cf:loop index=&quot;&quot;&gt;&lt;/cf:loop&gt; The error message will be clear, because the required attributes would have been defined. Coldfusion has this, and it is where experienced developers can add tags to the core language as if it was.

    &lt;cfAndrew was=&quot;maybe not&quot;&gt;
    # Posted By Andrew Scott | 5/22/08 9:41 AM
  • Keith Woods's Gravatar Could someone tell me what "TLD approach" is? Wikipedia is strangely silent.

    And, while I don't spend a lot of time decompiling *.class files, I'm pretty sure that there might be some problems with running jad and looking at that output and saying that's equivalent to what was written in the source code (now if Macrodobe would release the source...). The compiler will optimize things where it can, and who knows what exactly the source looked like. The decompiled output is just a good guess. By the same token, any discussion talking about refactoring the openBD code that doesn't include examples from the original java is probably destined to be somewhat fruitless.

    Just my $0.02.
    # Posted By Keith Woods | 5/22/08 9:48 AM
  • Ben's Gravatar So you admit you violated the Adobe license terms and decompiled their code?

    tut tut that is somewhat naughty of you is it not?
    # Posted By Ben | 5/22/08 9:49 AM
  • Andrew Scott's Gravatar @Keith: Think JSP....

    Everyone else, this has opened up more discussion than I intended. What I wanted to do, is illustrate the short comings that I have seen in openBD so far.

    As Matt as already stated, the code is now out there. And it is now us as a community that will shape this, and that as I am sure Matt and others will agree, has got to be a great thing.
    # Posted By Andrew Scott | 5/22/08 9:58 AM

Comments (Moderated) | Add Comment