Hi Karen, * Karen Tung (Karen.Tung at Sun.COM) wrote: > 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.
Ok, I'll make this change. >>> - 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. So after thinking about this some more I agree that explicitly passing in HASH_TYPES to computer_hash() is the better approach. I'll make this change and send out another mail when I've updated the webrev. Thanks Karen! -- Glenn
