Glenn Lagasse wrote: >> generate_checksums: >> >> - Most of the shell scripts I read have the functions defined for >> the script right after any statically defined values. The compute_hash() >> function is kinda in the middle of the file, can you move it? >> > > I wouldn't say it's in the middle. The bulk of the file prior to it's > location is comment/license text. I suppose I could move it above the > argument check. Would that be acceptable? > > Yes, that would be fine. >> - I think it is better to pass in $IMAGE and $HASH_TYPES as arguments >> to the compute_hash() function. That way, the function we are absolutely >> clear on exactly which value is being used, and it doesn't depend on things >> getting set correctly before the function is called. >> > > I'll accept passing in $IMAGE since that changes but HASH_TYPES are > static in the script and I don't see any reason to 'pass' them in as > arguments. Unless I were to move the hash_type checking inside the > compute_hash() function and I don't think that buys us anything and it > would make the flow awkward (I don't believe checking to see if the > manifest included any hashes is best done inside compute_hash() which > means we need to set HASH_TYPES outside the function anyway). If > HASH_TYPES isn't set correctly (the only possible output is that it's > empty), we'll exit the script in the check specified hash types portion > before ever calling compute_hash(). > > compute_hash() is certainly not the place to check and make sure the hash types are valid. Right now, the script is simple, so, one can easily see that the hash types are set and computed before calling compute_hash(). However, if this script becomes more complicated in the future, where it is not so easy to spot and make sure, we might accidentally call compute_hash() without having a valid value for $HASH_TYPES. That's why, as good software engineering practice, we should always pass in all the values that a function needs for computation, instead of depending on things being set external to the function. In any case, if you don't want to pass it in, please at least put a comment for this function to say something about that it assumes the HASH_TYPES variable is set prior to calling the function.
--Karen
