Mark J. Nelson wrote: > >>>> Please review: >>>> 421 webrev: clean up global vars vs. function params >>>> http://cr.opensolaris.org/~nbush/scm-migration/421/webrev/ >>> >>> It seems like you should either remove all of the typeset >>> declarations from build_old_new, or from each of the >>> build_old_new_blah functions that it calls. But you never reference >>> them in build_old_new, so it seems like the only use of those >>> typesets is to be inherited by the called functions, and you're >>> explicitly redefining them in each place. My preference, I think, >>> would be for build_old_new to be the single point where these are >>> defined; then you don't even end up passing args at all to >>> build_old_new_blah, but each such function gets a well defined set of >>> parameters. >>> >>> Instead of moving the olddir/newdir mkdir statements outside of the >>> loop, why not just remove them entirely? The semantics of mkdir -p >>> will cause them to be created implicitly by the mkdir statements >>> inside the loop already. >> >> I agree with your suggestions. Here's a new webrev; please >> note the new URL: >> http://cr.opensolaris.org/~nbush/scm-migration/421/webrev.v2/ > > This looks good. > > It's good enough, or if you felt like polishing a little bit more, you > could also typset OLDDIR/NEWDIR variables in build_old_new, because each > called function repeatedly uses the same constructs > > $WDIR/raw_files/old/$PDIR > $WDIR/raw_files/new/$DIR
With variables set this way it would need to have $OLDDIR/$PF or $NEWDIR/$F to identify the file in the raw_files directory, but it would still need to have $PWS/$PDIR/$PF or $CWS/$DIR/$F to identify the file in the actual SCM directory. It's shorter, but it seems less readable to me -- needing to use a separate variable for the subdirectory in one place but not the other. However, it makes sense to typeset the original olddir/newdir in build_old_new, then the build_old_new_* variants can be mostly restored to the original versions, and a bit more code can be moved into build_old_new: http://cr.opensolaris.org/~nbush/scm-migration/421/webrev.v3/ --Nathan
