Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
NewPoints 1.4 Review

Please note that this is pretty much a negative criticism post, rather than a balanced review as mentioned in this thread. Also be aware that stuff posted here may be highly subjective.
Please feel free to criticise this post, however.

Plugin Reviewed: NewPoints
Plugin Version: 1.4 (last updated 1st June 2010)
Plugin Author: Pirata Nervo
Author Message
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,357
Joined: Jan 2008
Post: #1
NewPoints 1.4 Review
This is a plugin "review" requested by Pirata Nervo.


This is quite a large plugin, so I will not go over everything.  In general, the code looks quite good - there doesn't appear to be any outright issues.  But as these reviews focus on negative aspects, I'll put up my concerns here.

Concerns
  • On newpoints.php, the menu should, ideally, highlight the user's current location so that the user is aware of where they are.  Alternatively, a breadcrumb showing the user's location (which isn't present either).
  • A number of templates on newpoints.php are uncached.
  • As NewPoints appears to support multiple languages, language phrases shouldn't be hardcoded, as in this example in newpoints.php:

    PHP Code:
    $value = $setting['value']." Chars";

  • I'm a bit concerned with how user input is handled, mainly with the plugin reporting back to the user if invalid input is supplied.  It appears to actually validate and sanitise the input well, however it's not always reported back to the user.

    When the user tries to donate points to another user (newpoints.php, under the do_donate action), we have the following piece of code:

    PHP Code:
    1
    2
    3
    4
    5
    6
    	// make sure the amount of points is not negative
    	$amount = abs(floatval($mybb->input['amount']));
    	
    	// do we have enough points?
    	if ($amount <= 0 || $amount > floatval($mybb->user['newpoints']))
    		error($lang->newpoints_invalid_amount);

    The use of the abs() function is a little questionable.  Basically, if a user enters a negative amount to donate, the plugin will make it positive, and allow the input through (as long as you have enough points and it's not "-0").  I would ideally let the check which follows handle this invalid input, so that the user is notified that they entered an invalid input.

    Then we have the code:

    PHP Code:
    1
    2
    3
    4
    5
    6
    7
    8
    	// send points
    	newpoints_addpoints($mybb->user['uid'], -($amount));
    	if ($uid > 0)
    		newpoints_addpoints($uid, $amount);
    	elseif ($username != "")
    		newpoints_addpoints($username, $amount, 1, 1, true);
    	else
    		error();

    This handles what happens when the user enters a UID or username to donate to.  Unfortunately, no validation of either is performed, so if you happen to mistype a username or UID, you'll lose points, but no-one will gain them.  Ideally the system should check whether the user in question exists before attempting to donate to them, and telling the user if the user doesn't exist.  Also, if you fail to enter either, you'll get a generic error message rather than one explaining your problem.

  • A small bug in the code:

    PHP Code:
    	redirect($mybb->settings['bburl']."/newpoints.php", $lang->sprintf($lang->newpoints_donated, newpoints_format_points($amount)));
    	
    	$plugins->run_hooks("newpoints_do_donate_end");

    The redirect() function calls exit, so the plugin hook will never actually be executed.

  • A small "bug": in a number of places, $db->query() is used instead of the more correct $db->write_query().  (yeah, not really an issue for a large majority of cases, and difficult to debug and identify without actually looking through code)
  • The main plugin file, inc/plugins/newpoints.php is rather large (nearly 2000 lines) and is loaded on every page.  Probably not a big issue, but perhaps it could be modularised a bit more, so less code is loaded on every page load.
  • Some of the functions shouldn't be declaring arguments as optional for example:

    PHP Code:
    1
    2
    3
    4
    5
    6
    function newpoints_get_usergroup($gid = 0)
    {
    	global $db;
    	
    	if (!$gid)
    		return;

    PHP Code:
    1
    2
    3
    4
    5
    6
    function newpoints_getrules($type = '', $id = 0)
    {
    	global $db;
    	
    	if (!$type || !$id)
    		return false;

    Not supplying a GID will cause the function to return null - thus making the parameters optional seems a little odd.

  • Instances of $_SERVER['PHP_SELF'] should be replaced with $current_page for consistency reasons.

Note that, as mentioned above, I have only gone through some code, primarily newpoints.php and some of inc/plugins/newpoints.php

Commendations
An interesting feature is that there are a lot of plugin hooks spread throughout newpoints.php, which can make developing plugins for NewPoints relatively easy, which is a nice feature.

My Blog
(This post was last modified: 06-29-2010 01:37 PM by ZiNgA BuRgA.)
06-19-2010 01:39 PM
Find all posts by this user Quote this message in a reply
Pirata Nervo Offline
Member
***
Posts: 235
Joined: Jan 2008
Post: #2
RE: NewPoints 1.4 Review
Thank you Smile
I have a question though, I can't check MyBB's code right now but what's the difference between $db->query and $db->write_query?

Not sure how I missed that redirect call ahah, that's why it's always good to have someone else to review the code.
I'll follow your suggestions and release 1.5 once I've done all modifications.
06-20-2010 05:09 AM
Find all posts by this user Quote this message in a reply
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,357
Joined: Jan 2008
Post: #3
RE: NewPoints 1.4 Review
No worries - hope my suggestions helped.

$db->write_query is for multi-server setups - it sends the query to the master MySQL server, as opposed to the slaves.

My Blog
06-20-2010 10:05 AM
Find all posts by this user Quote this message in a reply
Pirata Nervo Offline
Member
***
Posts: 235
Joined: Jan 2008
Post: #4
RE: NewPoints 1.4 Review
Oh alright, thank you Smile
06-20-2010 09:07 PM
Find all posts by this user Quote this message in a reply
Pirata Nervo Offline
Member
***
Posts: 235
Joined: Jan 2008
Post: #5
RE: NewPoints 1.4 Review
I changed the way the menu works. In 1.4 it loads the menu() function from each plugin.
In 1.5 I switched to a run_hook_by_ref which passes the default $menu as an argument and can then be altered by any plugin.

I believe this is better than loading all plugins again (one to load the plugins and another one to load the menu), right?
06-25-2010 07:43 PM
Find all posts by this user Quote this message in a reply
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,357
Joined: Jan 2008
Post: #6
RE: NewPoints 1.4 Review
Can't remember what code you're using, but if it's require_once, then, obviously, the files will be loaded once anyway.

My Blog
06-25-2010 07:52 PM
Find all posts by this user Quote this message in a reply
Pirata Nervo Offline
Member
***
Posts: 235
Joined: Jan 2008
Post: #7
RE: NewPoints 1.4 Review
Oh yeah it is, there is no different then apart from going through a loop again and browsing the directory.
06-26-2010 02:34 AM
Find all posts by this user Quote this message in a reply


Forum Jump: