We launched new forums in March 2019—join us there. In a hurry for help with your website? Get Help Now!
MODX Community Forums Archive

Looking for Some Feedback

    • 12556
    • 103 Posts
    iamdave Reply #1, 17 years, 8 months ago
    I just wrote my first custom snippet and it works fine but I’d like some feedback on my code. How can I improve it? Are there any obvious problems with it? I have a solid background in OOP concepts but my experience lies entirely with Java, C++ and ActionScript. I know very little about OOP in PHP. Any suggestions about how to make it more object-oriented and less procedural would be appreciated as well.

    The purpose of the snippet is to populate a document with a series of images pulled from directories on the server. There are 12 projects and 12 corresponding directories. The -3 adjustment is there because the corresponding document IDs are 4-15 but the project numbers are 1-12. The $project variable is passed in the snippet call. The snippet is called, imaginatively enough, ProjectImages. Thanks in advance!

    <?php
$piPath = 'assets/templates/bdr/project-images/project'.($project - 3);
$piDir = $modx->config['base_path'].$piPath;
$piIter = 0;
$piOutput = '<div class="slideShowLinks">';
$piImgs = '';

if($piHandle = opendir($piDir)){
	while(false !== ($piFile = readdir($piHandle))){
		if($piFile != "." && $piFile != ".."){
			$piOutput .= '<a href="javascript:;" rel="'.$piIter.'">'.($piIter + 1).'</a> ';
			$piImgs .= '<img src="/modx-0.9.6.3/'.$piPath.'/'.$piFile.'" alt="'.$piFile.'" />';
			$piIter++;
		}
	}
}
$piOutput .= '</div><div class="projectImageWindow"><div class="projectImages">'.$piImgs.'</div></div>';
unset($piPath, $piDir, $piIter, $piImgs, $piHandle, $piFile);
return $piOutput;
?>

    Dave

      • 3749
      • 24,544 Posts
      BobRay Reply #2, 17 years, 8 months ago
      For a small, utility snippet with no functions, I wouldn’t worry about OOP.

      My only comments would be that: 1) You should probably handle the case where $project is undefined in the snippet call (see below); 2) It will fail if there are any extraneous files in the image directory (e.g. .svn or FTP log files). If you’re sure that can’t happen, it’s not really a problem; 2) The fact that you are depending on the relationship between document ID number and directory names seems iffy, although it should work if you’re careful. I’m just thinking of a case where you deleted a document by mistake and had to recreate it. It would get a new document ID and things would get kind of complicated.

      $project = isset($project) ? $project : 1;


      Otherwise, it looks very well done to me. smiley
        Did I help you? Buy me a beer
        Get my Book: MODX:The Official Guide
        MODX info for everyone: http://bobsguides.com/modx.html
        My MODX Extras
        Bob's Guides is now hosted at A2 MODX Hosting
        • 12556
        • 103 Posts
        iamdave Reply #3, 17 years, 7 months ago
        Cases 1 and 2 are both unlikely in this situation but I’ve added the suggested error checking anyway.  Can’t be too careful, right?  As for the thing with the document IDs, I agree, it is iffy but I’ve yet to come up with a better way of doing it.  I started a thread elsewhere about how to get Wayfinder to output an iterator so that no matter what the document IDs are, the links will still have rel values that proceed consecutively from 0 to 11 but I still haven’t found a solid way to do that.  Thanks for the input!
          • 7231
          • 4,205 Posts
          dev_cw Reply #4, 17 years, 7 months ago
          Another option for naming the dir is to use the alias. The advantage to using the doc id is that it is permanent whereas the alias and other settings can change. Maxi uses the doc id for gallery folder names.
            [font=Verdana]Shane Sponagle | [wiki] Snippet Call Anatomy | MODx Developer Blog | [nettuts] Working With a Content Management Framework: MODx
            Something is happening here, but you don&#39;t know what it is.
            Do you, Mr. Jones? - [bob dylan]
            • 12556
            • 103 Posts
            iamdave Reply #5, 17 years, 7 months ago
            In this situation it’s better for me to use a number. I’m using the rel attribute in a calculation. I would prefer it start with 0 but I haven’t found a way to do that yet and using the document ID will work if I adjust for the offset because the documents are in consecutive order. If that were to ever change it would break all to hell but it’s not something that I’m anticipating changing.

            I just had a thought. The documents have a menu index. Is this a value I can access via a placeholder in Wayfinder? Like, [+wf.menuIndex+] or something? That would be perfect.