xSpy v0.4 Review

[ZiNgA BuRgA]

This is a plugin "review" requested by flash.tato.


No outright issues were found.  Just a few concerns and suggestions:

• I think it would be a good idea to start "spying" immediately when the page is loaded.  I don't really see any other purpose of why you'd go to a blank page
• There is a 15 second window when you initially start "spying", which may not be terribly useful for smaller boards.  I guess the purpose of this plugin isn't really to provide a history, so perhaps this is desired behaviour, though, maybe, it might be a good idea if this time interval was configurable.
• Maybe I'm wrong on this, but I couldn't find anything which automatically clears old entries from the page.  Probably not a big issue however.
• Maybe draw a line in the list when the user starts/stops "spying", so they know where new/old entries are
• In the xspy_install() function, $db->write_query should be used instead of $db->query
• There is no breadcrumb on the xSpy page - perhaps there should be one?
• There's not really a fallback for users with JS disabled; arguably, this is pretty much a non-issue, though a <noscript>You must have Javascript enabled to view this page</noscript> would be preferable
• The default polling interval of 1.5 seconds is perhaps a bit odd when timestamps on the server are stored in whole seconds.  Perhaps 1 or 2 second intervals would be more appropriate, although, arguably, there's some delay with sending/receiving requests, which could fill up the 0.5 second gap.  Also, it may be a good idea to allow the polling period to be configurable by the admin, to potentially reduce server load from many requests.
• The plugin doesn't really do extensive permissions checking - it checks whether the user can view the forum or not, but doesn't check stuff like whether the thread is unapproved; arguably not a big issue, but may not be desirable
• Static data which is logged into the xspyevents table is static, thus if changed, could be inaccurate (eg thread being moved to a different forum); arguably not a big issue, but may not be desirable
  

Otherwise, the plugin looks quite solid from what I can see.  Good job flash.tato

[flash.tato]

Quote:There is a 15 second window when you initially start "spying", which may not be terribly useful for smaller boards.  I guess the purpose of this plugin isn't really to provide a history, so perhaps this is desired behaviour, though, maybe, it might be a good idea if this time interval was configurable.

Did you mean 1.5 second? Perhaps i should make the interval of the timer configurable. by adding it as a variable in the JS part.

Quote:Maybe I'm wrong on this, but I couldn't find anything which automatically clears old entries from the page.  Probably not a big issue however.

Doesn't "clear" button do the trick or maybe you meant that i should clear old entries when there are news one, or you refer on the database side?

PHP Code:

<button onclick="$('xSpy_log').update('')">{$lang->xspy_clear}</button>

---

Quote:Maybe draw a line in the list when the user starts/stops "spying", so they know where new/old entries are

Great idea!

Quote:The plugin doesn't really do extensive permissions checking - it checks whether the user can view the forum or not, but doesn't check stuff like whether the thread is unapproved; arguably not a big issue, but may not be desirable

I will be adding new checking measures, this is a serious problem, i didn't consider the approved/unapproved threads.

Quote:Static data which is logged into the xspyevents table is static, thus if changed, could be inaccurate (eg thread being moved to a different forum); arguably not a big issue, but may not be desirable

I think i will make it modular so everyone could implement new things to be spyed, and yes now i will implement builtin some moderation actives to be spied.

[ZiNgA BuRgA]

(06-25-2010 08:47 PM)flash.tato Wrote:  Did you mean 1.5 second? Perhaps i should make the interval of the timer configurable. by adding it as a variable in the JS part.

Nah, I meant the 15 second window where you start deleting entries from the table.  I dunno, but for smaller boards, one may consider something like 15 minutes more appropriate.

(06-25-2010 08:47 PM)flash.tato Wrote:  Doesn't "clear" button do the trick or maybe you meant that i should clear old entries when there are news one, or you refer on the database side?

PHP Code:

<button onclick="$('xSpy_log').update('')">{$lang->xspy_clear}</button>

---

I'm referring to auto-clearing - eg if someone just leaves the page there running, it could accumulate quite a lot of entries.  Perhaps, for example, after 100 entries, the script automatically deletes old ones on the page.

(06-25-2010 08:47 PM)flash.tato Wrote:  I think i will make it modular so everyone could implement new things to be spyed, and yes now i will implement builtin some moderation actives to be spied.

Interesting

Anyway, hope the feedback helped, and thanks for the plugin

[flash.tato]

Quote:Nah, I meant the 15 second window where you start deleting entries from the table.  I dunno, but for smaller boards, one may consider something like 15 minutes more appropriate.

Ok now i understand, but if there is 1.5 second polling in this case what is the reason to let it be stored in table? maybe this value should be computed if i make the polling seconds alterable

Quote:I'm referring to auto-clearing - eg if someone just leaves the page there running, it could accumulate quite a lot of entries.  Perhaps, for example, after 100 entries, the script automatically deletes old ones on the page.

Ok now i understand. Good idea

Quote:Anyway, hope the feedback helped, and thanks for the plugin

Surely for me your review was an occasion to learn new things and you don't have to thank me for the plugin.

Thank you very much for your time.

[ZiNgA BuRgA]

(06-25-2010 09:23 PM)flash.tato Wrote:  Ok now i understand, but if there is 1.5 second polling in this case what is the reason to let it be stored in table? maybe this value should be computed if i make the polling seconds alterable

Hmm, I think I misread something.  I thought it initially filled the output based on existing contents of the table.
Looking at it, it appears it doesn't do this - only updates with newer entries.  If so, then I guess there's not much point keeping it for longer.
Apologies for my mistake.

[flash.tato]

Zinga i'm rewriting from scratch the plugin.

What do you think about this approach?

PHP Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19

class xSpyServer { static $instance; function __construct() { self::$instance =& $this; } } function &xspy_server_get_instance() { if(xSpyServer::$instance instanceof xSpyServer) return xSpyServer::$instance; else return new xSpyServer(); }

---

The concept is that there is a class who offers methods for inserting new events, cleaning table, fetching most recent events.
My idea was to allow 3rd party plugin to add its own handler so it can add new things to be spyed but i want to be sure that the class is istantiated one time only, what do you think?

[ZiNgA BuRgA]

From what I've seen in the community, I doubt you'll get many 3rd party plugin hooking in, if any at all.

The way you want to do it is really your choice.  I personally don't see the benefit of classes here.  Singletons only really are used because PHP does not (yet) support namespaces (or maybe not), so the only real advantage is the namespace like effect (only useful for longer pieces of code, I wouldn't consider your code long enough to really take advantage of this), but has downsides like difficulty in dynamically loading methods (you probably don't need this either).

So feel free to use any way you're comfortable with.  I'd probably recommend not to do something like that, cause many plugin developers probably don't understand objects very well, so if you stick to basic functions, which is what MyBB uses, you probably have a slightly better case of getting 3rd party contribution.

[flash.tato]

The concept was something like this:

PHP Code:

1 2 3 4 5 6 7 8 9

function on_xpy_loaded() { $xspy =& xpy_server_get_instance; $id_to_be_stored = $xspy->add_handler('my_handler'); } function my_handler($info) { echo "Zinga moved post"; }

---

But i will consider your words i think i will draw some sketch about code structure.

[ZiNgA BuRgA]

Well, that, compared to something like:

PHP Code:

xspy_add_event('thanked post', 'person A thanked B for their post');

---

[flash.tato]

Should i add a global variable for storing all the handlers?
Or in the function xspy_add_event as a function that uses the class itself?

Anyway i think you're right, rethinking better there is no need for singleton in my case, maybe i should make it procedural as you say.