Error message

You need to log in or create an account to access this page.

As I was reviewing the token generation function, I noticed 3 things that are very easy optimizations and quite so:

1. There is one regex that used 'i' and a few lines below the entire buffer is converted to lower case

2. the HTTP[S], FTP... protocols where tested before the lower case, there is certainly no reason to not check for HTTP:// as well (or even Http://)

3. The variable_get() in the loop would always return the same value and could be optimized with a variable which got to be faster

Thank you.
Alexis

CommentFileSizeAuthor
#2 spam-6.x-baysian_optimization.patch2.24 KBAlexisWilke
spam-6.x-baysian_optimization.patch2.24 KBAlexisWilke

Comments

gnassar’s picture

gnassar commented

All makes sense.

Quick fix on a typo, line 14 of the patch file:

+ $sanitized = drupal_strtolower($tring);

should be

+ $sanitized = drupal_strtolower($string);

(good reason to run with E_NOTICE enabled, right?) ;)

AlexisWilke’s picture

AlexisWilke commented
StatusFileSize
new spam-6.x-baysian_optimization.patch2.24 KB

Yes... 8-)

There is a fixed patch, just in case.

jeremy’s picture

jeremy commented
Status: Needs review » Reviewed & tested by the community

Looks good.

BTW: Any improvements/bug fixes made to the 6.x spam_filter_bayesian.module are likely candidates for patches against the 7.x bayesian.module which I intend to have the 7.x spam module depend on. http://drupal.org/project/dail

gnassar’s picture

gnassar commented

As much as I may hate to say it (and trust me, I *hate* to say it), that probably means we need to start the 7.x-dev branch ASAP. (Between that, and D7 launching in nine days...)

jeremy’s picture

jeremy commented

I think we should hold off on a 7.x branch until we have another stable/tested 6.x release. It doesn't make much sense to branch an unstable and rapidly changing code base imo. The bayesian.module I was referring to already exists for 7.x at the above link, and should simplify the code in the spam_filter_bayesian module when ported to 7.x.

AlexisWilke’s picture

AlexisWilke commented
Status: Reviewed & tested by the community » Fixed

Jeremy,

How could they have a Bayesian filter and not the spam filter? Is there a special implementation of the Bayesian functionality in that dail module? Just wondering... 8-)

Note that you can hijack this issue and set it to the dail project if you want to.

(and possibly the fix at line 118 and around?)

Thank you.
Alexis

jeremy’s picture

jeremy commented

> Is there a special implementation of the Bayesian functionality

It's a generic bayesian implementation to allow it to be used by other modules, not just the spam module. There will still need to be a spam_filter_bayesian module to hook it into the spam api.

gnassar’s picture

gnassar commented

Well, I can't say I object to that. I was hoping to wait for D7 until another stable release. (Which I think we're getting close to. I just submitted a patch for the last really-old-and-untouched bug fix. Exciting!) :)

But if we're going to do that -- would you object for perhaps just taking that D6 stable release and making the D7 port a straight port of that, with no major changes (like moving Bayesian support to another module)? We're going to have our hands full as it is with the port, I think. I'd rather just get what we have as stable here over to D7, and then start making big changes.

Alexis: commit link?

AlexisWilke’s picture

AlexisWilke commented

http://drupal.org/cvs?commit=471294
http://drupal.org/cvs?commit=471272 (for #1009234: Notices on line 118 of Bayesian filter)
http://drupal.org/cvs?commit=471244 (for #1009234: Notices on line 118 of Bayesian filter)

I guess I should update the CHANGELOG.txt too... 8-P

Here: http://drupal.org/cvs?commit=471358

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.