のみまくし日記

Sledge::Plugin::Affiliate - new

@_ を2行目、3行目でつかっているけど、用途が違う気がする。(ちゃんと調べてないので、間違っていたら失礼)

用途が同じでも、@_ は別の変数に代入して使ったほうが良い。

Sledge::Plugin::Affiliate - send_notice

for の中で使っている @_ は $self = shift; のあとに代入しておいたほうが、良い。

より、パラメータ受け付けるんだ〜と後からモジュールを見る人（自分も含む）に分からせるため。

まぁ、SYNOPSISに書くのでも良いかもしれない。

Sledge::Plugin::Affiliate - send_notice_for

上記と同様、@_を代入したほうがいいかなー。

まぁ、コード部分では一番最初に出てくるので別に気にしなくてもいいといえば、気にしなくてもいいかもしれないけど。

Configまわり

これは、実際のコードではすでに直っていますがkeitai_affiliate ではなく affiliate。

Configまわり2

Sledge::Plugin::Affiliate::Plugin::KTAFの「ひみつ」部分まで読まないと、わからないことだけど、（俺は心の目で読んだ）

Sledge::Plugin::Affiliate::Pluginのsendでは、config.affiliate.ua_opts を使っていて、

Sledge::Plugin::Affiliate::Plugin::KTAFではconfig.affiliate.ktaf を使ってます。

SledgeのConfig::YAML的には以下の感じ

affiliate:
  ua_opts:
    agent: ProjectAffiliateAgent
  ktaf:
    foo: bar

と ktaf と ua_opts が同じレベルなのが、ちょっとキモイ。Sledge::Plugin::Affiliate::Plugin::UaOpts というプラグインを作りたくなったときに困る。（笑い）あえりないけど。

forでぐるぐる。

send のあたりで for文でぐるぐるまわしているけど、基本的には常に1つしか使わないはずなのでなんか無駄な気がする。

プラグインが100になることはないけど。

Module::Pluggable::Fast

実際にコード書いてみてから言えって話かもしれないけど、

なんとなく、Module::Pluggableじゃなくて、UNIVERSAL::requireを使う方がすっきりするような気がする。(と俺のフォースが言っている)

未来のメモ1

Sledge::Plugin::AffiliateのSYNOPSISにありますが、

if (my $ktaf = $self->r->param('ktaf')) {
    $self->affiliate->param(ktaf => $ktaf);
}

の部分をも、Plugin化したくなるはず。

if (my $ktaf = $self->r->param('ktaf')) {
    $self->affiliate->param(ktaf => $ktaf);
} elsif (my $foo = $self->r->param('foo')) {
    $self->affiliate->param(foo => $foo);
} elsif (my $bar = $self->r->param('bar')) {
    $self->affiliate->param(bar => $bar);
}

みたいに使うので。

未来のメモ2

上記を実装する前提で、

r.param('ktaf') というのはセッションが入っているパラメータ名なので、別のアフィリエイトサイトで、かぶる可能性がある。

ktaf は無いにしても、sとかあるので、なんで、コレをキーにして使用するプラグインを判断するのは危険。

どこからきたセッションかってのをパラメータで分かるようにしておくのが一般的なのでその値をキーにする実装が良いかと。

ようは、複数のアフィリエイトを使おうとすると以下のようになるはず。

use Switch;

switch( $self->r->param('aff_type') ) {
    case 'ktaf' {
          # Sledge::Plugin::Affiliate::Plugin::KTAF
          $self->affiliate->param(ktaf => $self->r->pram('ktaf'));
    }
    case 'foo' {
          # Sledge::Plugin::Affiliate::Plugin::Foo
          $self->affiliate->param(foo => $self->r->pram('s'));
    }
    case 'bar' {
          # Sledge::Plugin::Affiliate::Plugin::Bar
          $self->affiliate->param(bar => $self->r->pram('s'));
    }
}

なんで、ここの部分を抽象化するのであれば、aff_type含めて行うほうが、堅い。

未来のメモ3

特殊なケースだけど、以下のようなパターンもあり得る。対応できるだろうか?

use Switch;

switch( $self->r->param('aff_type') ) {
    case 'ktaf' {
          # Sledge::Plugin::Affiliate::Plugin::KTAF
          $self->affiliate->param(ktaf => $self->r->pram('ktaf'));
    }
    case 'ktaf_hoge' {
          # Sledge::Plugin::Affiliate::Plugin::KTAF
          $self->affiliate->param(ktaf => $self->r->pram('ktaf'));
    }
}

未来のメモ4

このプロジェクトで使用している、アフィリエイトサイト一覧が欲しいかも。(いままでは、switch文のあたりが一覧だった)

未来のメモ5 - configにマップ?

未来メモを全部解決しよとしたら、config にマップを持たせることになると思う。それってsexy?

言い訳チックなこと

いずれにしても、俺はまだ使ってないので、使ってからもうちいどレビューするです。

某サイトで「8アフィリエイトサイトに対応しる!」って言われているので。

(とりあえず、熱いうちに言っておこうと思って)