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.
josh wrote:First I'd say you'd need to define the expected inputs & outputs you want to see in this mode, with enough scenarios to show how it should work in all situations.
Er, which mode?
josh wrote:Another would be what to do if I close a tag I never opened?
Probably close either at the end of the parent, or the end of the string.
If a tag is never opened or a tag is never closed, the regexp will not detect it and will just leave it alone.
Wondering why correct the invalid nesting instead of just parsing it?
Edit| I mean, if a parser could be wrote to correct all invalid nests and open and close tags, then that would be beyond the scope of amazing. But seems like a rather complex chore.
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.
If you can define expected inputs & outputs, I think I could wing whatever is specified. However IMO the list of specs could become rather large if we try and do too much, I think its definitely worth considering.
Perhaps before we do that, we could do one that outputs invalid XHTML (and keep that as a separate mode as well)? That will provide some value to some users, and help us better gauge the complexity. Or not?
OK, I think there are several "modes" here that have been discussed (from my understanding)
1) As it currently is
2) One that parses all tags, even if they're improperly nested, producing invalid xhtml
3) One that produces valid xhtml
4) One that fixes invalid nesting and produces valid xhtml
Have I got it straight?
1 and 2 would be great. 3 would be awesome, 4 would be asking too much (though definitely cool)
And if 3 or 4 were to be done, wouldn't that replace 1 and 2? (I mean why have 1 and 2, if 3-4 could do the same thing but with valid output)
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.
Lets start with adding number 2. We already have 1/3, all we have to do is break it out into a separate object, then create a drop-in replacement that uses method 2.
Good work. I like how you moved the regex in a text file. You probably did too much for implementing the 'non strict' one, adding the constant and all when the feature doesn't yet exist But a non-TDD programmer would probably call me silly (and I'd think the same of them)
Edit bloops you let a comment go out of date. That's why I hate comments
The way I'd recommend tackling this new strategy is to first get the old strategy under unit test. Right now its tested indirectly thru "base.php".
(@Jonah & Scott) The way I understand this currently works is the current strategy finds "positions" of matches. It then loops thru matches, replacing with replacements. Each time it does this it factors in the difference of the length of the old & new text, so that it can update the positions. (the difference in length between the matched text & the text to be substituted is subtracted from the positions of all remaining matches)
We have no unit tests that show that these sub-systems find the right positions, but we do show that the thing as a whole works, in our tests.
I'd find it really easy to do this feature, if we first had some tests that shows the inputs & outputs for finding positions with the current strategy. From there I could duplicate the tests, and modify the expected positions as per the new algorithm, from there I could duplicate & change the existing implementation and modify it to do matching differently until the tests pass, then we could refactor. I don't mean to rant about tests, just saying how I'd keep the ball rolling if I were familiar enough with the code, or if it had enough tests for me to do this in this area.
If you guys can get at least 1 test showing an input string, and the output array of positions, I could probably take it from there...
josh wrote:(@Jonah & Scott)
The way I understand this currently works is the current strategy finds "positions" of matches. It then loops thru matches, replacing with replacements. Each time it does this it factors in the difference of the length of the old & new text, so that it can update the positions. (the difference in length between the matched text & the text to be substituted is subtracted from the positions of all remaining matches)
That's exactly right.
josh wrote:I'd find it really easy to do this feature, if we first had some tests that shows the inputs & outputs for finding positions with the current strategy. From there I could duplicate the tests, and modify the expected positions as per the new algorithm, from there I could duplicate & change the existing implementation and modify it to do matching differently until the tests pass, then we could refactor. I don't mean to rant about tests, just saying how I'd keep the ball rolling if I were familiar enough with the code, or if it had enough tests for me to do this in this area.
If you guys can get at least 1 test showing an input string, and the output array of positions, I could probably take it from there...
Yes, that needs to be done. BTW, as you can see I'm back now. I'm still pretty wiped out, so I won't be able to do anything today.