depwl9992 / anomalyjobs

Automatically exported from code.google.com/p/anomalyjobs
0 stars 0 forks source link

EVENT Code Review #101

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
While compiling plugins for 6.0 release, noted several issues with the
EVENT plugin that need to be fixed if it's going to go with the release. 
Comments relate to line numbers in /branches/dev/plugins/event.txt as-of r304.

My edits:
- if(foo,bar) --> ifelse(foo,bar,) for TM3 compat
- FN_FLEXWIDTH where appropriate
- Lines 8-66 added. Review. If TPS bucket not needed, remove those checks.
- Flag setting switches at the end of each bucket

Line 4:  Claims interaction with TPS bucket, but I do not see TPS bucket
mentioned at all, except in the help attribute(s).  This informational
header needs to be a lot more detailed as-to what the plugin actually does,
so that users can determine whether they want to install it or not.

Line 87:  Refers to me/MAP_NAME which may exist on the TPS and RP buckets
(per full install) but you do not install it on EVENT.  Need to either add
it or just refer to %va/MAP_NAME as in other lines.

Line 118:  Looks like the help from RP was duplicated here but not updated
to reflect the purpose of EVENT.  Also see Line 4 comment, as these are
related.

Lines 191-206. This seems like a lot of convoluted steps for something
simple. Buried at line 196 is an iter() to produce a list of non-dbrefs,
but the list is never used; only the last element is stored as %qp and the
list's strlen() is checked > 0.  You could just do a map(%va/MAP_PMATCH,%0)
and use member(#-1) to locate the position of the offending player (if
any).  See the +job/query command for an idea (and use MAP_SOURCE if you
want to include +jgroups in the PLAYER list!)

Line 207: looks like a map(%va/MAP_PMATCH,%0) to me! If you did this prior
to the block of code above you could just use %q9.

Lines 209-217: Why not just setdiff(setunion(%q8,%q9),setinter(%q8,%q9)) ?

Line 229: Recommend #-1* for cross-platform compatibility as not all error
messages will be the same but all will probably start with #-1.

Lines 253-270, 271, and 273-279:  See comments for 191-206, 207, and 209-217.

Similar comments to above for equivalent lines on the RP bucket.

And finally, should the EVENT bucket be PUBLIC?

Original issue reported on code.google.com by widdis@gmail.com on 15 Feb 2010 at 2:31

GoogleCodeExporter commented 9 years ago
School is killing me this semester. I will indeed get to this, but I am on 
marginal time as it stands.

Original comment by Fleety...@gmail.com on 1 Mar 2010 at 5:47

GoogleCodeExporter commented 9 years ago

Original comment by widdis@gmail.com on 3 Mar 2010 at 2:31

GoogleCodeExporter commented 9 years ago
Most of the problems you have with the code (centered mostly the 
PROCESS_PLAYERS)
is copied directly from the RP and EVENT buckets. We should update the other 
buckets.

Okay, Line 4: It interfaces with the TPS bucket via receiving a job from 
it, as it shares some information between SUMMARY and MYSUMMARY. 
Information set in the TPS (such as plot summary, plot owner (which becomes 
the Contact person), etc) stage of the job passes to the EVENTS bucket. I 
figure 
how it interfaces would be docs-included.

Line 87: Fixed to %va.

Line 118=I forgot to update it. Will fix.

PROCESS_PLAYERS is taken directly from the RP and TPS buckets. We should update 
all of them.

PROCESS_CONTACT is just mirrored from PROCESS_PLAYERS, recommend changing when
we fix all player-setting buckets.

Yeah, EVENTS should be public. Anyone should be able to sign up for an event.

The idea of workflow with interaction is:

    The plot is conceptualized in the RP bucket. When that stage is final, it is
    transferred to the RP bucket for 'actively running tinyplots'. When you're
    ready to advertise 'hey we need some players for this upcoming event', you
    transfer to the EVENT bucket, who can then sign themselves up for it.

I'll fully admit that the event bucket isn't ready. :) We'll just slate it for
a future release (though, having more plugins now would be sweet).

Original comment by Fleety...@gmail.com on 9 May 2010 at 5:44

GoogleCodeExporter commented 9 years ago
Er, it's transferred from RP to TPS, then to EVENT.

Original comment by Fleety...@gmail.com on 9 May 2010 at 5:55

GoogleCodeExporter commented 9 years ago

Original comment by Fleety...@gmail.com on 9 May 2010 at 6:12

GoogleCodeExporter commented 9 years ago

Original comment by widdis@gmail.com on 17 Aug 2010 at 10:53