Page 3 of 6

Re: CryoBB - Create your own Bulletin Board

Posted: Tue Dec 21, 2010 3:13 pm
by Jonah Bron
@Josh: I followed what you listed above and cleaned up the tests. One more question: ought I to keep the test strings and match assertions in class constants?

Re: CryoBB - Create your own Bulletin Board

Posted: Tue Dec 21, 2010 5:07 pm
by Jonah Bron
About the CryoBB_Base make() method: should I make it accept some sort of config array instead of three different optional arguments?

Re: CryoBB - Create your own Bulletin Board

Posted: Tue Dec 21, 2010 9:55 pm
by s.dot
I don't think there are any additional options that could be passed to it.
So I wouldn't go with an array just yet unless more options are needed.

If in the future it gets more complicated I would use a config array or a CryoBB_Config object

Right now it is very simple if I want to parse bb code, emoticons, one or the other, or neither. For example, on PHPBB3 I can choose to disable bbcode and disable smilies below my post.

As it is I would just do the following to simulate that behavior.

Code: Select all

$formattedInput = !empty($_POST['input'])
    ? $base->make($_POST['input'], isset($_POST['useBBCode'], isset($_POST['useSmilies']))
    : '';
I'm not sure about having $isXHTML in the parameter. Does anyone really want their line breaks to be < br > instead of < br />?
I guess it doesn't hurt to leave it in there though.

Another idea..

Instead of setProcessor(), how about addProcessor(). This way multiple processors can be added and applied.

Also, the problem still exists where if i were to type in [ b ] [ i ] text [ /b ] more text [ /i ] the italics tag wouldn't even get parsed. I think this is why there is a need to do a second pass OR (preferably), do an explicit preg_match_all() for each defined tag.

Doing the latter would make the regexp's simpler and easier to maintain while eliminating the problem. Of course if this change was made it would require a significant rewrite of the base. Perhaps this could be done, and then CryoBB rewritten from scratch to get around some hooks we've been using to make tags and matches compatible with the base.

I am willing to do a lot of the work if in agreement.

Thoughts?

Re: CryoBB - Create your own Bulletin Board

Posted: Tue Dec 21, 2010 11:03 pm
by Jonah Bron
s.dot wrote:I don't think there are any additional options that could be passed to it.
So I wouldn't go with an array just yet unless more options are needed.
I was thinking of starting on paragraph support, so it may get more complicated.
s.dot wrote:Instead of setProcessor(), how about addProcessor(). This way multiple processors can be added and applied.
Sounds okay, then we'd add a method called removeProcessor().
s.dot wrote:Also, the problem still exists where if i were to type in [ b ] [ i ] text [ /b ] more text [ /i ] the italics tag wouldn't even get parsed. I think this is why there is a need to do a second pass OR (preferably), do an explicit preg_match_all() for each defined tag.
I still agree with what Benjamin said: "Garbage in, garbage out.". It really just makes things over-complicated to try to fix user error.
s.dot wrote:Doing the latter would make the regexp's simpler and easier to maintain while eliminating the problem. Of course if this change was made it would require a significant rewrite of the base. Perhaps this could be done, and then CryoBB rewritten from scratch to get around some hooks we've been using to make tags and matches compatible with the base.
I think it's very maintainable as it is now, at least now that it's commented. Broken up as it is, it's very understandable.

I just don't think we need an architecture change for now.

Re: CryoBB - Create your own Bulletin Board

Posted: Tue Dec 21, 2010 11:08 pm
by s.dot
I still agree with what Benjamin said: "Garbage in, garbage out.". It really just makes things over-complicated to try to fix user error.
I don't know if I call that garbage. It's a very easy error to make and probably a pretty popular one.
Your call, though :)

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 12:22 pm
by Jonah Bron
Well lets get the opinion of the masses. How about it, everyone?

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 1:04 pm
by Christopher
What do current BBcode parsers do? It seems like the one used here leave unparsed tags for the user to deal with. Comments are editable and previewable, so it is not much of a problem.

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 1:13 pm
by Benjamin
I stand by the Garbage In, Garbage Out. One job of a programmer is to deal with unexpected input though. So if this is something you guys are building and releasing as a package of some sort it wouldn't be a bad thing for it to handle mis-matched/mis-nested tags. This does add a level of complexity though and with complexity comes performance hits and bugs. It's important to note that browsers themselves have complex algorithms built right in to handle this already. If the conversion can be done while preserving the presence of all of the tags the browser itself could be the error handler. Ideally the end user would be notified of any issues detected before the data is saved so that they can be fixed in the first place.

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 1:58 pm
by josh
Jonah Bron wrote:@Josh: I followed what you listed above and cleaned up the tests. One more question: ought I to keep the test strings and match assertions in class constants?
Nope, actual & expected strings are really important to understanding the test (see my mantra).

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 2:12 pm
by Jonah Bron
Christopher wrote:What do current BBcode parsers do? It seems like the one used here leave unparsed tags for the user to deal with. Comments are editable and previewable, so it is not much of a problem.
That's how it works now.
Benjamin wrote:I stand by the Garbage In, Garbage Out. One job of a programmer is to deal with unexpected input though. So if this is something you guys are building and releasing as a package of some sort it wouldn't be a bad thing for it to handle mis-matched/mis-nested tags. This does add a level of complexity though and with complexity comes performance hits and bugs. It's important to note that browsers themselves have complex algorithms built right in to handle this already. If the conversion can be done while preserving the presence of all of the tags the browser itself could be the error handler. Ideally the end user would be notified of any issues detected before the data is saved so that they can be fixed in the first place.
This is how it would work with Scott's design change proposal. It would parse invalidly detected tags as well as valid ones. It would work by running one tag match at a time, instead of all at once.
josh wrote:Nope, actual & expected strings are really important to understanding the test (see my mantra).
Alright, thanks. I'll leave it as it is.

One problem with the proposed change is nested tags of the same tag. For example:
Bob wrote:
Mike wrote:How's it going?
Great!
It will be more complicated than greedy/non-greedy.

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 3:03 pm
by josh
Thanks for commit access, it is a privilege. I just invested an hour cleaning up as I saw fit. Check out r33-48

Basically I was able to do a bit of exploratory testing, adding more unit tests, cleaned up existing unit tests, cleaned up existing test utility methods. Re-factored nesting in production code (revision 40). Implemented my idea of calling it an "attribute" (ala HTML attribute, instead of your guy's term parameter which conflicts with programming terms).

revision 42-48
I also took a big ugly method from CroBB/Base, made sure it was well unit tested, and moved it to a new class (sprout class). I then proceeded to untangle the spaghetti logic in that method extracting out individual methods for each procedural bit of business logic, or kludge. These "kludges" are blocks of code prefixed by a comment, So take a look at Base.php the makeTag() method, to me it was hard to read. Now look at the new MakeTag class, which has methods devoted to each individual calculation (single responsibility principle)

Let me know what you guys think. I also recommend you let me migrate us to phpunit, it will be easier to manage as the project grows IMO. Kudos to you guys for having unit tests, so I could come in, with 0 experience and make these changes in 1 hour.

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 3:16 pm
by Christopher
The Masked Refactorer has struck again!!! :drunk:

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 3:31 pm
by josh
Refactoring is hard enough without the mask! hahah thats funny man

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 3:45 pm
by Jonah Bron
josh wrote:Thanks for commit access, it is a privilege. I just invested an hour cleaning up as I saw fit. Check out r33-48
Wow, your changes really look great! It's a privilege to have "da man" working on the project :D
josh wrote:Basically I was able to do a bit of exploratory testing, adding more unit tests, cleaned up existing unit tests, cleaned up existing test utility methods. Re-factored nesting in production code (revision 40). Implemented my idea of calling it an "attribute" (ala HTML attribute, instead of your guy's term parameter which conflicts with programming terms).
I haven't looked at the tests yet, but I will soon.
josh wrote:I also took a big ugly method from CroBB/Base, made sure it was well unit tested, and moved it to a new class (sprout class). I then proceeded to untangle the spaghetti logic in that method extracting out individual methods for each procedural bit of business logic, or kludge. These "kludges" are blocks of code prefixed by a comment, So take a look at Base.php the makeTag() method, to me it was hard to read. Now look at the new MakeTag class, which has methods devoted to each individual calculation (single responsibility principle)
Again, great job. It was a bit big for me too. I actually wouldn't have ever thought to sprout it as another class...
josh wrote:Let me know what you guys think. I also recommend you let me migrate us to phpunit, it will be easier to manage as the project grows IMO. Kudos to you guys for having unit tests, so I could come in, with 0 experience and make these changes in 1 hour.
I still can't believe you did all that in an hour. Thanks, great work. Later I'll look into PHPUnit.
Christopher wrote:The Masked Refactorer has struck again!!! :drunk:
+1!

Re: CryoBB - Create your own Bulletin Board

Posted: Wed Dec 22, 2010 5:03 pm
by s.dot
Simply beautiful!

re: my position on the tag matching
if this were a popular package or a commercial package, the number one complaint would be improperly nested tags not parsing
i'm not of the position to fix the improper nesting (garbage in, garbage out), but I'm in the position to parse a full tag (improperly nested or not)

re: the regex (quote for example)
the not matching the same tag can be done on a per-tag basis. It's not magical in the match-all-tags regexp ;P

Those are just my opinions. I am also in agreeance with the opinion of the masses!

But, PHPBB will parse this:

Code: Select all

[b]im bold[i] and italic[/b] yeah?[/i]
As this:
im bold and italic yeah?

Whereas currently CryoBB will parse it as this (html version):

Code: Select all

<strong>im bold[i] and italic</strong> yeah?[/i]
Leaving the [ i ] through [ /i ] (a full tag match) unparsed