21
This is a decent concept, but by looping on the cd you loose some of the value of the $OLDPWD function in the shell. For example, I often use the special construct cd - in a shell, and that changes directory to the one you were in before.
Your code will make that impossible.
I would instead recommend that you instead build up a chain of ../ string values, ...
17
You completely ignored the first points from the review your earlier question,
and they still apply here:
Whitespace is not a precious resource: your code is far too dense for
my taste
fewer semicolons, more newlines.
try to limit your line length to 90 chars for readability
I would go even further, and recommend to stay within 70 chars. ...
16
Your recommendation is to define alias up=". path/to/up" so that when you type up 3, it expands to . up 3. However, since you want to take an optional argument and affect the state of the current shell, I think you would be better off defining a shell function instead.
As it turns out, the [ -z "$1" ] special case is not necessary, since seq 1 just expands ...
16
Improve your file handling:
Don't use a hard coded file name for temporary files.
Don't spew temporary files into the current directory.
Make sure you clean up temporary files even on error.
The first two can be addressed by leveraging mktemp. The last can be addressed with a trap.
branchesfile=$(mktemp)
trap "{ rm -f $branchesfile; }" EXIT
You should ...
15
Use input redirection instead of cat | ...
Instead of cat somefile | somecommand, you should use input redirection, for example:
xgengen bash < Samples/template_bash_CMakeLists.txt | bash - > CMakeLists.txt
The advantage of this is that you're running one fewer process (no more cat).
Use modern style command substitution $(...)
Don't use old style ...
15
That looks crazy. But some constructive comments:
Did you check for ssh-copy-id script, that is usually shipped with openssh? It solves for you the first part of your problem in standard way.
The second problem is in my eyes non-existent. Do you really need your authorized_keys in all accounts? Isn't it enough for one user that can do sudo?
The third is ...
15
Bash is not well-suited for transforming JSON. But jq is. But calling jq 3 times for each line of input is certainly going to be slow.
There are several other issues too with the script. The `...` syntax is obsolete in favor of $(...); the counting can be simplified, or even better, eliminated using tail -n +5; and the repeated bulk_result.bulk would be ...
14
Overall, the code is pretty clean. Some notes about the Python part:
be consistent about the documentation string style - enclose them with triple quotes, start with an upper-case letter, end with a dot (PEP8 reference). I also think that you can use less newline breaks there.
if we would follow PEP8 importing recommendations, there should be a newline ...
14
DRY - don't repeat yourself
"Don't repeat yourself" is a popular programming principle. You could apply it here by not repeating your filename over and over.
MY_BRANCHES="5db3bb3c-718a-444c-b1ce-d90a5a0d1cb3.clc"
git branch --merged > $MY_BRANCHES
nano $MY_BRANCHES
# Trim trailing and leading whitespace etc.
sed 's/^[ \t]*//;s/[ \t]*$//' < $...
13
I see a few things that could allow you to improve your program. First, though, I don't consider myself a bash expert, so there may well be better ways of doing these things.
Use a "shebang" line
As this question points out, you should always use a "shebang" line for your bash scripts. So the first line would be:
#!/usr/bin/env bash
Pass values as ...
13
Use exit codes directly
Instead of this:
merge $SOURCE_BRANCH 'staging'
status=$?
if [ $status -ne 0 ]
then
echo "ERROR: STEP 1" >&2
exit 1
fi
You can use exit codes directly in conditions, like this:
if ! merge $SOURCE_BRANCH 'staging'
then
echo "ERROR: STEP 1" >&2
exit 1
fi
Use it this way everywhere.
Handle errors consistently
...
13
I would use a different service that provides output in a format that is good for tools (not humans (html is for displaying for humans)).
Hit this url:
https://ifconfig.co/json
You're output looks like this:
{
"ip": "154.140.296.288",
"ip_decimal": 926648668,
"country": "United States",
"city": "Seattle",
"hostname": "154-140-296-288.Bob.com"
}
...
12
Simplify
Since $# cannot be both 0 and 1 at the same time, it would be better to join the two if conditions here:
local a="$1"
if [ $# -eq 0 ]
then
a="default"
fi
if [ $# -eq 1 ]
then
git config --global user.name "$a"
git config --global user.email "$a@$a.com"
fi
But, actually, what if there are 2 or more parameters instead of 0 and 1? It ...
12
Variables
The line containing the JDK_ROOT_DIR=( … ) definition is so long that it's unreadable and unmaintainable. In fact, you listed /usr/j2se/ twice. It would help to put one entry on each line.
The switch in naming in for sub_dir in "${JDK_ROOT_DIR[@]}" is confusing. Each "root dir" is suddenly being called a "sub dir"?
Your export JAVA_HOME=$...
11
Your code is so full of irrelevant content that distracts from the purpose of the code.
The most Beautiful shell scripts are the ones that do a job, and do it well. They do it in a standard way with the least surprise. They accept input from pipes, redirects, and files, and they output in a way that is redirectable, loggable, and readable.
The following is ...
11
Bugs:
echo "$my_variable" | my_command can behave unexpectedly if the variable starts with one of the echo options. You can do my_command <<< "$my_variable" instead.
unset $IFS does not unset IFS. For that you need unset IFS.
Simplifications:
You shouldn't need to set IFS - read will save the full input to the variable anyway (see help [r]ead).
A ...
11
Concept Review
There are a number of purpose built tools to do this job already. The one that comes to mind first is 'shred'. shred is installed by default on all Redhat and Ubuntu systems (in coreutils package on both systems). It is also installed on my RaspberryPi, so, it is ubiquitous.
Shred does similar things to what your script does, but it will be ...
11
Since you are using printf to output the spaces, you may as well use a specific printf format for the output.
Here's a replacement function:
centerQ(){
textsize=${#1}
width=$(tput cols)
span=$((($width + $textsize) / 2))
printf "%${span}s\n" "$1"
}
What it does is it identifies how long the whole output should be, by adding the text width and ...
11
In my testing, constructing one Perl script and running it repeatedly is much faster (0.5s versus 3.6s) then running a new Perl instance for each replacement:
while read f; do
echo "s/$f/$f/gi;"
done < files.txt > s.pl
while read fn; do
perl -pi s.pl "$fn"
echo "$fn | $count/$lines finished..."
count=$((count+1))
done < filenames....
11
The vulnerability I see is that only the first two arguments are checked. You could check all of them:
rm() {
[[ $- == *i* ]] && for arg
do
if [[ $arg = "*" || $arg = "./*" ]]
then
# abort
fi
done
# do the rm
This kind of checking will miss other dangerous wildcards like ** or ?*. You can get ...
11
Some suggestions:
The trailing slash in the mkdir command is redundant.
$(…) is preferred over backticks for command substitution.
Why use seq in one command? They both do the same loop, so you might as well use {1..100} in both places.
Semicolons are unnecessary in the vast majority of cases. Simply use a newline to achieve the same separation between ...
10
200_success's answer uses the evil eval which in his context was unnecessary.
Update: Oh, what, bash? Let's use some brace expansion + evil things:
# This time we have to be evil. All code in this answer is CC0.
up() { [ "$1" -eq "$1" ] &>/dev/null || set -- 1; "cd \$(printf '../%.s' {1..$1})"; }
local is not a that nice solution since POSIX doesn'...
10
Arithmetic operations
If you look closely, something is a bit odd here:
for (( i = 0; i <= $limit; i+=$1 )); do
Ideas? How about: i is written without a dollar, but limit is written with a dollar... In fact you can drop the dollar from limit too, and the expression will be slightly simpler. From $1 you cannot drop it, as the meaning of 1 would be taken ...
10
Your code is fine, except for the stray method. POST at that point isn't valid, and you will try to connect to a server called POST. Just try your curl call with -Iv, you will notice two connections:
curl -Iv POST whatismyip.org
If you want to use the method, you would have to write -x POST, but to just get information, you use -x GET (which is the default ...
10
Nothing would make a reviewer happier than help you jam the networks of an innocent neighborhood. (sarcasm)
Mutually exclusive conditions
Can line match Address, Channel, etc at the same time? If not, then the multiple [[ ... ]] are needlessly evaluated, and it would be better to rewrite them using elifs:
if [[ "$line" =~ Address ]]; then
m=${...
10
Do not silently ignore invalid arguments
Your script allows to specify the source directory (and other locations). Here is the relevant part in function parseArgs():
-s|--src)
if [ -d "$2" ]; then
srcDir="$2"
fi
shift 2
;;
If --src <sourceDir> is specified ...
10
I will offer some suggestions.
Please note I have no experience with Mac OS, but I believe most of it will still apply.
Using system is not particularly bad in your case since you use static arguments in those calls excluding the possibility of command injection.
However, since your first C program is almost completely made up of system calls, you should ...
9
do ... done is a compound command; every subcommand shares the file descriptors; so teeing the the loop has the same effect as teeing each subcommand.
Two subsequent invocations of echo can be combined together.
cat $file is a dreaded UUOC.
A basename invocation can be avoided by changing directory to $FOLDER.
ls is absolutely unnecessary. The shell already ...
9
Adding checks? Just specify
set -e
at the beginning of the script. Should any command fail, the script will stop running, instead of wreaking more damage.
Using variables for repeated paths might make the script more readable.
tomcat1=/home/deploy/tomcatfirst
tomcat2=/home/deploy/tomcatsecond
sh "$tomcat1"/bin/catalina.sh stop
(BTW, shouldn't the ...
9
I don't know enough bash to offer you a technical review.
But, more philosophically: Have you considered writing this more in the style of a "program" than a "Press 1 to convert another"-type interface? Like:
~$ temperature c 30
86 ˚F
~$ temperature F 105
40 ˚C
My reasoning is that the above is easier and more flexible to use programatically, you don't ...
Only top voted, non community-wiki answers of a minimum length are eligible
