patternrubyMinor
Checking for rvm Ruby versions
Viewed 0 times
rvmcheckingrubyforversions
Problem
As it is going to be used as part of some automation and not just at the command line I was wondering if it could be improved at all.
Improve would mean any of:
Improve would NOT mean:
Improve would mean any of:
- handle sad cases better
- handle error better
- report errors better
- better able to work on multiple linux OS including OSX
Improve would NOT mean:
- shorter code
- short variable names
- Linux shell version specific tricks or shortcuts
- allowing for rvm not being present (checked elsewhere)
function make_sure_rvm_rubies_installed() {
rvm list > $TMP/local_ruby_versions.txt
local_ruby_versions=(1.8.7 1.9.3)
for version in "${local_ruby_versions[@]}"
do
cat $TMP/local_ruby_versions.txt | grep -q $version
version_search_result=$?
if [ $version_search_result = 0 ]
then
echo "Required RVM Ruby version $version confirmed as present on this machine"
else
echo "*** EXITING SMOKE TEST *** - not all required ruby versions are present in RVM"
echo "Please install RVM ruby version: $version and then re-run this program"
exit
fi
done
if [ $? = 0 ]
then echo "All required Ruby Versions confirmed as present locally"
fi
}
make_sure_rvm_rubies_installedSolution
The name
but it's a bit too much.
Maybe
You didn't indent the body of the function.
It's better to indent the inner part of large
No need to use the
Instead of
No need to save the exit code of the
you can do this directly:
The
The script could be reusable if you
to give a chance to the caller to recover gracefully if necessary.
(In your example use case it doesn't matter, but this is a tip for the future.)
I'm not sure what you're expecting from the
When a version is not found the original loop exits the entire script (I recommended above to return from the function).
So in case of failures this
Otherwise the last command to evaluate is an
which should just succeed,
so this
Although arrays are cool, and it's great that you can use them,
in simple situations like this I would use a normal variable:
You create the file temporary file
make_sure_rvm_rubies_installed describes very well what the function does,but it's a bit too much.
Maybe
check_installed_rubies would be good enough.You didn't indent the body of the function.
It's better to indent the inner part of large
{ ... } blocks for readability.No need to use the
function keyword when declaring functions, though you can if you like.Instead of
cat ... | grep it's better to omit the cat and use the file as a parameter of grep.No need to save the exit code of the
grep in the version_search_result variable,you can do this directly:
if grep -q $version $TMP/local_ruby_versions.txt
then
echo "Required RVM Ruby version $version confirmed as present on this machine"
else
echo "*** EXITING SMOKE TEST *** - not all required ruby versions are present in RVM"
echo "Please install RVM ruby version: $version and then re-run this program"
exit
fiThe
exit there is a bit unexpected.The script could be reusable if you
return 1 instead,to give a chance to the caller to recover gracefully if necessary.
(In your example use case it doesn't matter, but this is a tip for the future.)
I'm not sure what you're expecting from the
$? check in this code:for version in "${local_ruby_versions[@]}"
do
# ...
done
if [ $? = 0 ]
then echo "All required Ruby Versions confirmed as present locally"
fiWhen a version is not found the original loop exits the entire script (I recommended above to return from the function).
So in case of failures this
if is not reached.Otherwise the last command to evaluate is an
echo,which should just succeed,
so this
if condition seems pointless, you can omit it.Although arrays are cool, and it's great that you can use them,
in simple situations like this I would use a normal variable:
local_ruby_versions='1.8.7 1.9.3'
for version in $local_ruby_versions; do ...; doneYou create the file temporary file
$TMP/local_ruby_versions.txt but you don't clean it up when finished.Code Snippets
if grep -q $version $TMP/local_ruby_versions.txt
then
echo "Required RVM Ruby version $version confirmed as present on this machine"
else
echo "*** EXITING SMOKE TEST *** - not all required ruby versions are present in RVM"
echo "Please install RVM ruby version: $version and then re-run this program"
exit
fifor version in "${local_ruby_versions[@]}"
do
# ...
done
if [ $? = 0 ]
then echo "All required Ruby Versions confirmed as present locally"
filocal_ruby_versions='1.8.7 1.9.3'
for version in $local_ruby_versions; do ...; doneContext
StackExchange Code Review Q#69319, answer score: 4
Revisions (0)
No revisions yet.