User Widget
Review Request #168 - Created Feb. 15, 2011 and submitted
Information | |
---|---|
Petru Trimbitas | |
infoarena | |
Reviewers | |
hackers | |
Display a widget containing user statistics
This code is run on infoarena, not on an external website :). You have database acess, you should take you statistics from there, not scrapping the stats page.
This code is run on infoarena, not on an external website :). You have database access, you should take you statistics from there, not scrapping the stats page.
Looks pretty good. Congrats on getting the hang of the code so fast :). GG. However you have some problems with the coding style :). 1. No trailing spaces - there should be no spaces at the end of a line 2. Equations. When you write an equation you have to leave spaces between operands. Eg: $rez = $a + $b * ($c - $d) 3. Function calls. When calling a function you have to leave spaces after each comma. Eg: function_name($a, $b, $c, $d, $e) 4. English. Everything in the codebase must be in english, except output strings. Commentaries, variables, function names MUST be in English. There is also a bit of weirdness here since we didn't serve generated images until now, so I'm not sure how the design pattern should go. Usually in the controller you should just gather data from the models and the output should be generated in a view. However given the way this works, it's a bit weird because you don't have a view. I would like Bogdan to take a look on this and see what he has to say. Anyway, you did good :). Keep up the good work.
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
Remove trailing spaces
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
Put this from the beginning of the line. No need for a tab here. Basically you should delete 4 spaces (a tab) from all the lines.
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
I think you should use true/false not 1/0.
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
English here. get name. Also comments should be aligned with the code.
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
english
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
english
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
english
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
english
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
Spaces. if ($n1 + $n2 != 0)
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
spaces. $rez = $n1 / ($n1 + $n2) * 100
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
Spaces. round($rez, 2)
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
Spaces :P
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
english
-
/trunk/www/controllers/userwidget.php (Diff revision 2) -
Wrong spaces here. ($my_img, 154, 205, 50) You have to leave a space after each comma (,). No spaces before or after paranthesis.
-
/trunk/www/macros/macro_userwidget.php (Diff revision 2) -
No trailing spaces.
-
/trunk/www/macros/macro_userwidget.php (Diff revision 2) -
No spaces
Hey, sorry for taking so long. Patch looks good, nice job :) The problem Tibi mentioned with everything being done in the controller is valid though. In MVC, the *controller* is responsible with gathering all the required data and providing it to the *view*, which then renders it in a pleasant fashion. Even if the view is creating an image, not a normal HTML page, that is no problem. A few months ago we were using gnuplot to generate an image with user rating plots, so the view had to return an image. That has since been changed to using OpenFlashChart (i think), but if you want, you can check out an older revision of the code (for example revision 1000) and take a look. The controller was in www/controllers/plot.php and the views were www/views/{plot_rating,plot_distribution_gnuplot}.php
-
/trunk/www/controllers/userwidget.php (Diff revision 3) -
Coding style: // the image (space after //)
-
/trunk/www/controllers/userwidget.php (Diff revision 3) -
coding style: no space between imagecreatefromjpeg and ( Also why not widget.jpg?
-
/trunk/www/controllers/userwidget.php (Diff revision 3) -
Coding style: $numr >= 600 (spaces between operators)
-
/trunk/www/macros/macro_userwidget.php (Diff revision 3) -
Use url_userwidget here now. As a side note /infoarena2-dev/ is the IA_URL_PREFIX variable, so if you really wanted, you should have done something like IA_URL_PREFIX . "userwidget/"
Review request changed
Good job and sorry for the excessive amount of time this took. Most of my comments are coding style related, however there are 2 that are marked as issues that are the most important.
-
/trunk/www/controllers/userwidget.php (Diff revision 5) -
Spaces before and after .
-
/trunk/www/controllers/userwidget.php (Diff revision 5) -
Function comments should be in the following phpdoc style (this is a new coding style that we have not employed until now): /** * What function does * * @param ${name} {type} {description} * @return {type} {description} */
-
/trunk/www/controllers/userwidget.php (Diff revision 5) -
Coding style: add spaces after commas user_submitted_tasks($dbuser['id'], true, false);
-
/trunk/www/controllers/userwidget.php (Diff revision 5) -
How did you get this constant? You should probably be using rating_scale from 'common/rating.php'. It's bad to manipulate this type of data without using the methods provided in 'common/*', since our formulas may change at any time.
-
/trunk/www/controllers/userwidget.php (Diff revision 5) -
Controllers are not supposed to contain code about how to display a particular value. Stuff like "Rating: " . $rating needs to be in the view. Use english for all your variable names and do not use shorthand like $pbr or $pbi.
-
/trunk/www/controllers/userwidget.php (Diff revision 5) -
Space before and after = You could also use array syntax: $data = array( 'name' => $name, ... );
-
/trunk/www/url.php (Diff revision 5) -
Space before and after .
Excelent. Fix small comments and ship it. Do you have commit privileges?
-
/trunk/www/controllers/userwidget.php (Diff revision 6) -
Single space after =
-
/trunk/www/controllers/userwidget.php (Diff revision 6) -
No romanian language $rez :)
-
/trunk/www/controllers/userwidget.php (Diff revision 6) -
This does not change anything. You probably wanted to do $rez = round($rez, 2), but you are already doing it in sprintf below.
-
/trunk/www/macros/macro_userwidget.php (Diff revision 6) -
@return string Rendered HTML