For purely educational purposes, let's break down nano2's previous script and see where the problems lie.
First, as requested, let's format it better. Without changing any of the actual commands, here's how I would format it. All operating blocks are indented with tabs to the same level, and related code blocks are separated by blank lines.
I also moved the
do and
then keywords on the same line as the
for and
if keywords. It cuts down on clutter and helps line up the block better, IMO.
Code:
#!/bin/bash
fn2 (){
array1=( $@ )
array2=( $@ )
cnt=${#arr[@]}
echo "the count is " $cnt
for (( idx = 0 ; idx < cnt ; idx++ )); do
echo ${array2[idx]}
echo ${array1[idx]}
echo "the count ifor array2 "${#arr[@]}
done
list2="${array2[*]}"
cnt=${#array1[@]}
for ((i=0; i <cnt ; i++ )); do
pref=${array1[i]}
if grep -wq $pref <<< "$list2"; then
preflist2+="$pref "
list2="${list2/$pref }"
fi
done
array3=($preflist2$list2)
echo array1:
echo "${array1[*]}"
echo array2:
echo "${array2[*]}"
echo array3:
echo "${array3[*]}"
}
fn2 "${array1[@]}""${array2[@]}"
Now starting at the top:
Code:
fn2 (){
array1=( $@ )
array2=( $@ )
This sets both arrays, the global arrays, mind you, to the same values -- the entire "
$@" function input. We don't really want them to be the same, do we? I think you want
array1 to have the first input and
array2 to have the second input. A simple technique is to use
$1 and
$2 as Juako did, but that relies on word-splitting the input, which means it would break if any of the values contained spaces.
Code:
cnt=${#arr[@]}
echo "the count is " $cnt
Juako has already discussed the problems here. The array name is wrong, and the quotes can, and indeed should, surround the variable as well (remember, word-splitting occurs after variable expansion, unless quoted).
Code:
for (( idx = 0 ; idx < cnt ; idx++ )); do
echo ${array2[idx]}
echo ${array1[idx]}
echo "the count ifor array2 "${#arr[@]}
done
First of all, this whole section is really superfluous except as a way to check your inputs, since you print out the raw arrays at the end of the function anyway.
All this loop does is print out the entire contents of each array, and the count,
one entry per line, interlaced. See the output lines at the end for how to print everything out more cleanly.
Also, the final echo will simply print the same count number over and over. You don't want that, do you? Especially since you've already echoed it once outside the loop.
Code:
list2="${array2[*]}"
I'll comment later about using a scalar variable as a way to process a list. Otherwise no real problems here.
Be aware that this overwrites the
cnt that was set before. It might be better to save the two array lengths to separate variables, so that you don't get them confused (although recycling a variable has its benefits too).
Code:
for ((i=0; i <cnt ; i++ )); do
pref=${array1[i]}
A superfluous use of a variable (
pref), but no problems per-se.
Code:
if grep -wq $pref <<< "$list2"; then
This works too. But calling on the external grep is completely unnecessary when the shell has everything you need to compare the strings built-in. Also, remember to quote the
pref variable to avoid word-splitting.
Code:
preflist2+="$pref "
list2="${list2/$pref }"
It's at this point where I believe you really should be saving the output to another array. Scalar variables are not really suited for saving lists of things. But I'll save my recommended solution for a follow-up post.
Also, this technique removes duplicated entries if they match the default list, but keeps duplicates of the non-default entries. How the script should handle duplicates is something you still need to define for us.
Code:
array3=($preflist2$list2)
Again, no problems here, but only because you ensured that the entries were separated by spaces in the lines above it. If there were no spaces at the end of preflist2, then the final word would be combined with the first word of list2, and you'd get a false entry like "cowrat". So put a space between the variables.
Also, since the array setting relies on word-splitting, it would again fail if individual entries contained spaces. This is exactly why I recommend always using arrays for all lists.
The line would also read more cleanly if you spaced it out better:
array3=( $preflist2 $list2 ).
Code:
fn2 "${array1[@]}""${array2[@]}"
Again, the only real problem with this line is that you never defined
array1 and
array2 before executing the function, so you're passing it nothing. But also, you have to be clear about how the function will use the input, which brings us back to the first two lines of
fn2. How can the function know where to separate the input it receives into
its array1 and
array2?
And that goes back to my previous post where we must be clear about the difference between a function-local array and a global array of the same name.
Now if you a) define the global arrays first, b) declare the function's arrays to be local, and c) set it so that local array1 is set from local $1 and local array2 is set from local $2...then it would be possible to use the
${array[*]} form to send the global arrays into the local arrays reasonably safely.
But this, again, relies on word-splitting the input, and so is not the strongest method to use.
Anyway, to end this lesson, Your function will work as expected if you make the following changes:
Code:
fn2 (){
local array1 array2
array1=( $1 )
array2=( $2 )
...
}
globalarray1=( dog cat lion cow )
globalarray2=( lion rat mouse dog zebra pet cat bird cow calf sheep )
fn2 "${globalarray1[*]}" "${globalarray2[*]}"
...well mostly as expected. The part where it echos the arrays is still broken, due to the bad
cnt variable. I suggest you simply remove that section entirely. But you should get the desired output otherwise.
Still, it's far from ideal due to the other weaknesses I've pointed out. Give me some time and I'll see about posting a version that takes all of this into account.
