The Code Cleanup Project

News and discussion about beta versions and CVS changes
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

The Code Cleanup Project

Postby gRegor » Thu Nov 10, 2005 5:47 pm

So I've taken it upon myself to suggest this project (but not necessarily do all of it, ;) ).

The premise is that we have a set of coding standards for plugins and such, but not even the Nucleus CORE code itself complies with these standards. This should be remedied quickly, in my opinion.

So I started to go through globalfunctions.php and did several things. Below are the major changes. Here is the zip file; devs please take a look and verify everything is ok (the changed file is working fine for me so far) and consider it for CVS. One thing I didn't do yet but should be done is adding phpXref style comments.

As time permits I plan to be updating other CORE files and posting them for consideration.

1. Control structures missing braces were fixed.

Code: Select all

if(condition)
     statement;
became:

Code: Select all

if(condition) {
    statement;
}


2. More consistent/readable spacing used for concatenation and function arguments.

Code: Select all

functionName($var1,$var2,$var3);
$string = $var1.$var2.$var3;
became:

Code: Select all

functionName($var1, $var2, $var3);
$string = $var1 . $var2 . $var3;


3. Spaces were removed and tabs used instead.

4. Code which is executed outside of functions was all put at the beginning of the file; function declarations were put at the end of the file (they had been interspersed, adding to the confusion).
Last edited by gRegor on Thu Nov 10, 2005 7:45 pm, edited 1 time in total.
— gRegor
User avatar
Legolas
Nucleus Guru
Nucleus Guru
Posts: 811
Joined: Sat Jan 31, 2004 1:20 pm
Location: Woerden, Nederland
Contact:

Re: The Code Cleanup Project

Postby Legolas » Thu Nov 10, 2005 5:51 pm

gRegor wrote:4. Code which is executed outside of functions was all put at the beginning of the file; function declarations were put at the end of the file (they had been interspersed, adding to the confusion).


I would set functions above, on the other hand, I would'nt add non-function code to a functions file :P
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Re: The Code Cleanup Project

Postby gRegor » Thu Nov 10, 2005 6:04 pm

Legolas wrote:
gRegor wrote:4. Code which is executed outside of functions was all put at the beginning of the file; function declarations were put at the end of the file (they had been interspersed, adding to the confusion).

I would set functions above, on the other hand, I would'nt add non-function code to a functions file :P

I know in most other languages this makes sense since the functions have to be declared before they're used, but since PHP isn't that way I figured it made more sense to have the immediate actions of the file listed at the top so they're easy to find and follow the logic, particularly in a 1000+ line file.

But you're right, there shouldn't be non-function code in these files, particularly in a file named globalfunctions, heh heh. We can blame karma for that, though. I just wanted to clean things up a bit, not start re-writing CORE.
— gRegor
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Thu Nov 10, 2005 7:46 pm

Zip file posted; first post edited.
— gRegor
User avatar
admun
Nucleus Guru
Nucleus Guru
Posts: 4088
Joined: Mon Oct 20, 2003 2:57 am
Location: San Francisco, CA, USA
Contact:

Postby admun » Thu Nov 10, 2005 10:23 pm

thanks for the effort!
User avatar
roel
Nucleus Guru
Nucleus Guru
Posts: 4469
Joined: Tue Apr 16, 2002 12:41 am
Location: Rotterdam, The Netherlands
Contact:

Postby roel » Fri Nov 11, 2005 9:19 am

I totally agree with admun! Great going gRegor!
Is your question not solved yet?
karma
Posts: 983
Joined: Fri Dec 07, 2001 4:25 pm
Contact:

Re: The Code Cleanup Project

Postby karma » Sat Nov 12, 2005 5:49 pm

gRegor wrote:Here is the zip file; devs please take a look and verify everything is ok (the changed file is working fine for me so far) and consider it for CVS.


Thanks gRegor, I've merged most of the cleaned code into the CVS version.

The only problem I found was that the error_reporting needs to be set as early as possible, in order not to end up with notices about undefined indices (i.e. before accessing $CONF['Self'], which might not be defined when visiting the admin area).
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Nov 16, 2005 4:55 pm

Ah, that makes sense. Noted.
— gRegor
User avatar
roel
Nucleus Guru
Nucleus Guru
Posts: 4469
Joined: Tue Apr 16, 2002 12:41 am
Location: Rotterdam, The Netherlands
Contact:

Postby roel » Mon Dec 05, 2005 4:58 pm

gRegor, this thread is now sticky. :)
Is your question not solved yet?
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Tue Dec 06, 2005 12:52 am

Just updated action.php, atom.php, and install.php. The first two were pretty minor.

With install.php I adjusted the doCheckFiles() function. Instead of a bunch (16, to be exact) of if statements checking various files, like:

Code: Select all

   if (!is_readable('install.sql') ) {
      array_push($missingfiles, 'File <b>install.sql</b> is missing or not readable');
   }

I put the filenames in an array and used a for loop instead:

Code: Select all

   $missingfiles = array();
   $files = array(
      'install.sql',
      'index.php',
      'action.php',
      'nucleus/index.php',
      'nucleus/libs/globalfunctions.php',
      'nucleus/libs/ADMIN.php',
      'nucleus/libs/BLOG.php',
      'nucleus/libs/COMMENT.php',
      'nucleus/libs/COMMENTS.php',
      'nucleus/libs/ITEM.php',
      'nucleus/libs/MEMBER.php',
      'nucleus/libs/SKIN.php',
      'nucleus/libs/TEMPLATE.php',
      'nucleus/libs/MEDIA.php',
      'nucleus/libs/ACTIONLOG.php',
      'nucleus/media.php'
      );

   $count = count($files);

   for ($i = 0; $i < $count; $i++) {
      if (!is_readable($files[$i]) ) {
         array_push($missingfiles, 'File <b>' . $files[$i] . '</b> is missing or not readable.');
      }
   }

Shorter, cleaner, and easy to add additional files to the array to be checked.

I also added a function "replaceDoubleBackslash" since there were several calls to str_replace() to replace double backslashes.
— gRegor
User avatar
moraes
Nucleus Guru
Nucleus Guru
Posts: 2377
Joined: Sun Dec 23, 2001 9:42 pm
Location: Curitiba, Brazil
Contact:

Postby moraes » Tue Dec 06, 2005 9:59 am

gRegor wrote: $count = count($files);

for ($i = 0; $i < $count; $i++) {
if (!is_readable($files[$i]) ) {
array_push($missingfiles, 'File <b>' . $files[$i] . '</b> is missing or not readable.');
}
}[/code]
Shorter, cleaner, and easy to add additional files to the array to be checked.

Nice improvements! Cleaner code is always welcome. :-D

Here's a tip to deal with arrays. Arrays in PHP have their own loop iterator, "foreach". So this would be even cleaner:

Code: Select all

   foreach($files as $file) {
      if (!is_readable($file) ) {
         array_push($missingfiles, 'File <b>' . $file . '</b> is missing or not readable.');
      }
   }

No need to count the array anymore. There are cases when you need to check if the argument provided is an array before use foreach, but this is not one of them.

moraes
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Thu Dec 08, 2005 5:23 pm

Good call. :)
— gRegor
User avatar
matt_t_hat
Posts: 1123
Joined: Sun Aug 21, 2005 4:45 pm
Location: UK
Contact:

Postby matt_t_hat » Fri Dec 09, 2005 12:58 pm

Astounding work. 8)

This is something that is both important and necessary.

A virtual pat on the back for this effort.
User avatar
moraes
Nucleus Guru
Nucleus Guru
Posts: 2377
Joined: Sun Dec 23, 2001 9:42 pm
Location: Curitiba, Brazil
Contact:

Postby moraes » Mon Dec 12, 2005 1:50 am

I've done some cleanup today, separating important classes that were sharing the same space:

- ITEMACTIONS from BLOG.php to its own file;
- ACTIONS from SKIN.php to its own file;
- COMMENTACTIONS from COMMENTS.php to its own file;
- BaseActions from PARSER.php to its own file;

Some lost functions from the end of file BLOG.php were moved to globalfunctions (it is commented in the end).

Also, MANAGER::getInstance() now returns a true singleton. :-D

m.
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Dec 14, 2005 7:37 pm

Awesome! I saw your message before you edited it (I think it was this one?), sorry for the goof-up with install.php in CVS. I'll do updates in smaller batches.
— gRegor
User avatar
moraes
Nucleus Guru
Nucleus Guru
Posts: 2377
Joined: Sun Dec 23, 2001 9:42 pm
Location: Curitiba, Brazil
Contact:

Postby moraes » Wed Dec 14, 2005 7:59 pm

gRegor wrote:Awesome! I saw your message before you edited it (I think it was this one?)

Yes, it was. I thought after I posted it could be misunderstood and it would be better to talk to you personally. But then I haven't saw you on ICQ and couldn't find an e-mail on your site. :-)

m.
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Wed Dec 14, 2005 8:03 pm

Heh, I'm hardly ever on ICQ and yeah... need to get the new version of my site finished up. :) No worries, I didn't take it badly or anything.
— gRegor
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Tue May 09, 2006 11:51 pm

I've hardly been around the Nucleus development community this semester due to busyness, but now that the semester is over I have a bit more free time and was wondering what the status of this is... should I keep working on this? From the sounds of it a lot of the CORE classes have been rebuilt. I guess I should just checkout the latest code and see what we've got going on. :)
— gRegor
User avatar
moraes
Nucleus Guru
Nucleus Guru
Posts: 2377
Joined: Sun Dec 23, 2001 9:42 pm
Location: Curitiba, Brazil
Contact:

Postby moraes » Wed May 10, 2006 1:44 am

gRegor, you are *very* welcome back to your project! As you may have noticed, we've changed from CVS to SVN, and the trunk is no more the focus (right now). Make a checkout of the Subversion repository, go to /branches/NewAdminArea and take a look. Most of the new methods added to the traditional classes (from ADMIN.php) are already documented and formatted, but still there are *a lot* to do.

I'll move the branch to the trunk this week (Legolas merged both, and he is now travelling in Italy), and make it usable (it is not right now), but you'll know when it is done.

cheers!
moraes
User avatar
gRegor
Posts: 738
Joined: Tue May 14, 2002 3:17 am
Location: Bellingham, WA
Contact:

Postby gRegor » Thu Aug 31, 2006 2:00 am

I'm back at this... at least momentarily. :)

bookmarklet, forgotpassword, xml-rss just done. nothing major, but I'm working my way through the file structure.
— gRegor

Return to “Core Development”