PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Sat Aug 08, 2020 6:01 pm

All times are UTC - 5 hours




Post new topic Reply to topic  [ 77 posts ]  Go to page Previous  1, 2, 3, 4, 5, 6  Next
Author Message
PostPosted: Tue Dec 21, 2010 4:13 pm 
Offline
DevNet Master
User avatar

Joined: Thu Mar 15, 2007 6:28 pm
Posts: 2765
Location: Redding, California
@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?


Top
 Profile  
 
PostPosted: Tue Dec 21, 2010 6:07 pm 
Offline
DevNet Master
User avatar

Joined: Thu Mar 15, 2007 6:28 pm
Posts: 2765
Location: Redding, California
About the CryoBB_Base make() method: should I make it accept some sort of config array instead of three different optional arguments?


Top
 Profile  
 
PostPosted: Tue Dec 21, 2010 10:55 pm 
Offline
Tranquility In Moderation
User avatar

Joined: Sun Feb 06, 2005 8:18 pm
Posts: 5001
Location: Indiana
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.

Syntax: [ Download ] [ Hide ]
$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?

_________________
- 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.


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 12:03 am 
Offline
DevNet Master
User avatar

Joined: Thu Mar 15, 2007 6:28 pm
Posts: 2765
Location: Redding, California


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 12:08 am 
Offline
Tranquility In Moderation
User avatar

Joined: Sun Feb 06, 2005 8:18 pm
Posts: 5001
Location: Indiana

_________________
- 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.


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 1:22 pm 
Offline
DevNet Master
User avatar

Joined: Thu Mar 15, 2007 6:28 pm
Posts: 2765
Location: Redding, California
Well lets get the opinion of the masses. How about it, everyone?


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 2:04 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13592
Location: New York, NY, US
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.

_________________
(#10850)


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 2:13 pm 
Offline
Site Administrator
User avatar

Joined: Sun May 19, 2002 10:24 pm
Posts: 6887
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.

_________________
Image


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 2:58 pm 
Offline
DevNet Master

Joined: Wed Feb 11, 2004 4:23 pm
Posts: 4872
Location: Palm beach, Florida


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 3:12 pm 
Offline
DevNet Master
User avatar

Joined: Thu Mar 15, 2007 6:28 pm
Posts: 2765
Location: Redding, California


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 4:03 pm 
Offline
DevNet Master

Joined: Wed Feb 11, 2004 4:23 pm
Posts: 4872
Location: Palm beach, Florida
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.


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 4:16 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13592
Location: New York, NY, US
The Masked Refactorer has struck again!!! :drunk:

_________________
(#10850)


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 4:31 pm 
Offline
DevNet Master

Joined: Wed Feb 11, 2004 4:23 pm
Posts: 4872
Location: Palm beach, Florida
Refactoring is hard enough without the mask! hahah thats funny man


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 4:45 pm 
Offline
DevNet Master
User avatar

Joined: Thu Mar 15, 2007 6:28 pm
Posts: 2765
Location: Redding, California


Top
 Profile  
 
PostPosted: Wed Dec 22, 2010 6:03 pm 
Offline
Tranquility In Moderation
User avatar

Joined: Sun Feb 06, 2005 8:18 pm
Posts: 5001
Location: Indiana

_________________
- 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.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 77 posts ]  Go to page Previous  1, 2, 3, 4, 5, 6  Next

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 4 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group