CryoBB - Create your own Bulletin Board

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Project Update

Hey everyone,

Scott (s.dot) been down for a couple of weeks with some sort of "viral syndrome", so send him a nice PM. :)

He passed CryoBB over to me (at least temporarily). I've been intermittently making changes as I get time, and I'm really happy with the results. Here's what I've gotten done.

CryoBB_Tag

CryoBB_Tag has been broken into two classes: CryoBB_Tag, and CryoBB_Tag_Parameter. The new system allows developers to create different types of tags, as long as the ultimately extend CryoBB_Tag. This also moved some code out of Base and into Parameter.

CryoBB_Processor

Developer-defined content processing is different. Instead of passing a callback function, you pass your own object that extends CryoBB_Processor. This is a more OO approach, and allows multiple processing methods. Yes, I know you're thinking "why didn't he make it an interface?". Well, that would mean the dev has to create all required methods, even ones he/she doesn't want.

Code Readability

The regular expression match array has been turned into a stdClass. This makes the code in CryoBB_Base much more understandable (e.x. instead of $match[1], $match->tag).

Parsing Method Change

The way it parses the input has been transformed. Before, it would just replace the match with str_replace. To account for tags who's contents should not be parsed, they were removed and replaced with temporary markers, then restored later. This required a great deal of code. Now, it records the match's offset and length (PREG_OFFSET_CAPTURE), and makes the replacements with substr_replace. This change alone cut down 119 lines of code. Still in the "unstable" branch, as it has not been thoroughly tested.


Anyone interested in this project, I encourage you to check out the changes.

-Jonah
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: CryoBB - Create your own Bulletin Board

Post by Christopher »

Great work Jonah!
(#10850)
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Thanks, Christopher. S.dot did a really good job of getting this thing started :)

Some more changes.
  • Generic code reduction. Simplification of logic and increase in performance all around.
  • CryoBB_Match. This replaces the stdClass object used before. Logic was building up in Base that just didn't belong there, and the data format of the match was getting too complicated.
  • All setter functions I remembered to change now return $this for daisy chaining.
All changes have been reintegrated into the trunk, and the latest revision (now version 2.0) is available in the Downloads section (tar.bz).

Enjoy!
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Just created a set of unit test cases. This should make development easier.

P.S. Is everything cool now Josh? Can I keep programming? :)
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

Jonah, things are looking amazing. I know we've collaborated a little bit on the changes but the work credit and generally most credit should definitely go to you!

I was thinking.. this will severely slow down the speed of the library, but, perhaps greatly improve stability and usage. What if we iterate through all of the defined tags and check explicitly for that tag using a regexp?

Gathering all of the tags in one regex might be allowing for too much room for error to occur, and doing it separately would account for the invalid nesting (though it wouldn't fix invalid nesting, it would at least parse all of the tags).

Thoughts?
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

It's my opinion that it's great just the way it is. The output is *always valid XHTML, and the number of situations in which one would need overlapping tags is absurdly miniscule.

* unless the person writing text puts inline tags outside of block tags

Edit: just realized that the tag needs to be cleaned with preg_quote().
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

The tag doesn't need to be cleaned because it is being checked with ctype_alnum, and regexp delimiters must be non-alphanumeric characters.

I was thinking the reason for individual regexps for each tag would be to allow for something like:

[ quote="[ url ]http://www.php.net[ /url ]"]something[ /quote ]

I think I might be in shape to start working on this again (taking your direction, since you've took it in an exciting new direction!). I absolutely love how you put the match data and operations in Match.php.. brilliant.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Oh, yeah. It totally blows up if you do that...

Input

Code: Select all

[quote=[url=http://google.com]bobby[/url ]]hello[/quote]
Output

Code: Select all

<div><strong>[url=http://google.com wrote:</strong></div><blockquote>bobby[/url]]hello</blockquote>
Edit:

Realized why; it's because of the regex.

[quote=[u]Jonah[/u]]Hello, world![/quote ]

The red square brace is detected as the end of the quote tag. So, the quote is by: [u

And [u said: Jonah[/u]]Hello, world!
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: CryoBB - Create your own Bulletin Board

Post by josh »

No matter what I'll be able to break it. Stop trying to code for other programmers and code for the users ;-) the users aren't going to legitimately screw it up that bad, and if they do... by that time all original intent will be lost.

If you need it to be that foolproof to foolish users, I'd suggest a WYSIWYG editor and running the HTML thru htmlpurifier. To try and correct erroneous bbcode would be an exercise of advanced artificial intelligence. The best thing to probably do would be simply detect it, and notify the user.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

josh wrote:the users aren't going to legitimately screw it up that bad, and if they do... by that time all original intent will be lost.
That is a good point.

@s.dot: I think we might be able to fix it easier if we put the (?R) into the parameter too. I'll post it in the Regex section.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

I've been working on putting the recursion into the parameter, and it's almost done. You can see what I've done so far at the thread: viewtopic.php?p=637936
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Okay, the regular expression now fully supports BBCode inside of the parameter. Exciting! Everything works perfectly now. Not only that, but the expression is partially commented now, too. I also went through the code again and reorganized some of the logic, added more comments, and removed a bit of code smell (correct term?). All unit tests pass.

It's all in the trunk. Be sure to update your working copy! :)
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: CryoBB - Create your own Bulletin Board

Post by josh »

The comments above the test methods are more aptly suited for an assertion message on the assertion itself, so its reported in the results if it fails.

Also the context you write these should begin with "should" IMO, in order to document behavior. For example, your comment. "// test tag with parameter". This should be an assertion message, not a comment, and should read "should convert bbcode attribute to an HTML one" (something along those lines).


'// assert with parameter' & '// assert without parameter' you verify these both in one test, which is easy to see why they have similar setup logic. In this case a "test utility method" is more desirable than the "god test". The "god test" achieves a good result w/o duplicating logic. However a "test utility method" is more elegant IMO

Example psuedo-test

Code: Select all

// TEST METHODS (one assertion per method highly desired)
function testShouldDoMath()
{
  $a = $this->setupA();
  $b = 5;
  $this->assertEquals( 3, foo(5,$a), 'should do math');
}

function testShouldGrammar()
{
  $a = $this->setupA();
  $b = 'bar';
  $this->assertEquals( '6bar', foo2(5,$a), 'should do grammar');
}


//  TEST UTILITY METHOD
function setupA()
{
 ... really. long. redundant. logic...
}
}
Feel free to add me as a committer and I'll try and clean up the tests.

In the event your setup is more complicated, you could pass the objects back in an array and use list(), use multiple test utility methods, or better yet sprout a new test class. Each test class would contain setup logic in the setup method, never in the test methods themselves. test methods themselves should be very minimal code, only what is necessary to describe an interaction, disregarding "behind the scenes" setup.

Just remember this mantra:

if its important to understanding the test, its important that it is in the test.
If its NOT important to understanding the test, its important that it is NOT in the test.


Sloppy code, production or test code, causes more than just not pretty code. Further reading http://xunitpatterns.com/Test%20Smells.html

I give these unit tests an overall A- my only critiques are mostly minor, and its better than what I see on average.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Wow, thanks for reviewing the tests Josh :). I'll implement your advice as soon as I get time. I'll have to look up how to do assertion messages in SimpleTest.
josh wrote:if its important to understanding the test, its important that it is in the test.
If its NOT important to understanding the test, its important that it is NOT in the test.
That's a good "mantra" to remember. I will keep that in mind.
josh wrote: Feel free to add me as a committer and I'll try and clean up the tests.
Certainly, PM me your email and I'll add you to the project.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

Truly amazing work Jonah.
I just set up a custom tag that parses [rainbowtext][/rainbowtext] LOL, it uses the processor to apply a rainbow swatch of colors sequentially to each letter.
Once this thing is stable it'll be out of this world.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
Post Reply