current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have a script which is having too many modules. The script is working perfectly, but the only issue is it is taking too much time and I need to reduce the complexity of the script.

I have a source file (it can have multiple rows of data, each column separated by a '|'). The script should check all the rows for all the columns and find the columns which is having incorrect data (e.g., null in a non-null column, space in a not space column, alphabets in a number only column, and any value which is not a valid value for that column).

Now I have a master file, which has those column names and the position of the occurrence of that column in the source file with some indicators which will decide what all we have to perform on that column.

Example of master file:

MBR_SRC_SYS,15,H,N,N,N,1,FAC|NSC|WGS|ACE|CS9|CHP
CDH_AMT,27,H,1,1,1,N,N

There are 15 columns in my source file will be MBR_SRC_SYS and 'H' Indicates I have to perform a hard error check. Later is the indicator for the other checks like not null, not space, number format, and valid value and the last contains the valid values this particular column can have.

Format of master file:

Clm Name,Position,HardErrorCheck,NullCheck,SpaceCheck,NumberCheck,ValidValueCheck,ValidValues

Example of source file:

DTL|CLM| |RMB_CLM_2015_V01|RMBFCSNSC|15135NSC|rmb_fcsdmsclm_n.dat|rmb_fcsdmsclm_n.trg|NSC
2015093QA01109920150514 4 3132PD|WFS|2015093QA011099|2015/05/14
04:31:32|131|99|NAP||27002|6MB09|          | |2014/10/04
00:00:00|2015/05/15 00:00:00|2015/05/15 00:00:00|2015/05/15
00:00:00|2015/05/15
 00:00:00|97.26|0.00|0.00|0.00|0.00|0.00|0.00|0.00|0.00|0.00|0.00|0.00|0|0|WIFE
|A|SMITH                          |603T40775           |270022222     
|603T40775           |20       |F|1966/12/02
00:00:00|SUB|MD|SN|G2001|NTINCR|MFFFF| |91|91| |NSC|NAIIH11X|H| | | |
| | | | | | | | |2015/05/15 00:00:00| | | | | | | | | | | | | |
|W|D|N|P|PF|OP|N|CHK|0001025000| 7021693440|351159676      |Y| |I~IN~
~ ~ ~ | ~ ~ | | | | | | | | |0.00|0.00|0.00| | | | | | | | | | | | | |
| | |

What I am doing is having 2 while loops. With one loop I am reading the lines of the source file. Now inside this while loop after reading I am having another while loop to read the master file and do all the necessary checks.

Suppose my source file is having 500 records with 100 columns. Total loop count will be 500*100 = 50000 iterations. This is too much, because my source file can have as many as 5 million records.

I am new to Unix, so I am not sure, but scripting languages like PHP have a timeout and will definitely not be able to run this long. So what should be the solution for this?

#Log file creation

#exec 1> $CODE/WCC_FOA_RMBHUB/logs/RMB_HARD_ERROR_CHECK_ROWMAJOR_$(date +"%Y%m%d_%H%M%S").log 2>&1

SrcFilePath=$PMDIR/SrcFiles/WCC_FOA_RMBHUB
ListFilePath=$CODE/WCC_FOA_RMBHUB/lists

SrcFileName=$1
HardErrorFile=HARD_ERR_RMB_TEST_FILE.dat
MasterFile=$2

NullCheck()
{
value="$1"

if [ ! -n "$value" ] ; then

    return 10
else
    return 1

fi
}

SpaceCheck()
{
value="$1"
NullCheck "$value"
t=$?
if [ "$t" != "10" ]; then

value=$(echo $value)

    if [ ! -z "$value" -a "$value" != " " ]; then
        return 1
    else
        return 10
    fi
else
    return 1
fi

}

NumberCheck()
{
value="$1"
if [ $value -eq $value 2>/dev/null ]; then
    return 1
else
    return 10
fi

}

ValidCheck()
{
value="$1"


# Replace space with 'SPACE'

SpaceCheck "$value"
t1=$?
if [ "$t1" == "10" ]; then
    value="SPACE"
fi

ValidFromMapping="$2"
needle="|"
NumberOfDelimeter=$(echo $ValidFromMapping |  awk 'BEGIN{FS="|"} {print NF}')
v=1
vf=0

while [ $NumberOfDelimeter -gt 0 ]
do

    #echo YES
    ValidValue=$(echo "$ValidFromMapping" | cut -d'|' -f$v)
    #echo "Checking with the value $ValidValue."
    if [ "$ValidValue" == "$value" ]; then
        vf=1    
    fi

    v=$(expr $v + 1)
    NumberOfDelimeter=$(expr $NumberOfDelimeter - 1)

done

if [ $vf -eq 1 ]; then
    #echo "Match Found"
    return 1
else
    #echo "No Match Found"
    return 10
fi

}
SoftErrThreshHold=0

        #echo "Errrr... We got a hard error check column. We have to check $ColumnName in the whole god dam source table. This column is available in column $ColNum .We got to do it brah !!!\n"
        while read RowOne
        do
            HardErrorFlag=1
            SoftErrorFlag=1
            RecordColumn=$(echo "$RowOne" | cut -d'|' -f1)

            #echo "Started New Row"

            if [ "$RecordColumn" == "DTL" ]; then

                        KeyColumn=$(echo "$RowOne" | cut -d'|' -f9)
                        ErrorText="$KeyColumn |HARD ERROR OCCURRED IN THE FOLLOWING COLUMN(S):"
                        SoftErrorText="$KeyColumn |SOFT ERROR OCCURRED IN THE FOLLOWING COLUMN(S):"
                        while read line
                        do
                            ColumnName=`echo $line | cut -d',' -f1`
                            ColNum=`echo $line | cut -d',' -f2`
                            ErrorCheck=`echo $line | cut -d',' -f3`


                            NullCheckStatus=`echo $line | cut -d',' -f4`
                            SpaceCheckStatus=`echo $line | cut -d',' -f5`
                            NumberCheckStatus=`echo $line | cut -d',' -f6`
                            ValidCheckStatus=`echo $line | cut -d',' -f7`
                            ValidValues=`echo $line | cut -d',' -f8`



                            # Hard Error Check - 

                            if [ "$ErrorCheck" == "H" ]; then

                                #echo "Doing Hard Error"
                                IsNull=1;
                                IsNumber=1;
                                IsSpace=1;
                                IsValid=1;

                                HardErrorCheckColumnValue=$(echo "$RowOne" | cut -d'|' -f$ColNum)
                               #echo "Value is $HardErrorCheckColumnValue."

                                #Funtions Return 10 if it is NULL/SPACE/NOT A NUMBER

                                #echo $NullCheckStatus
                                if [ "$NullCheckStatus" == "1" ]; then 
                                    NullCheck "$HardErrorCheckColumnValue"
                                    IsNull=$?
                                fi

                                #echo $NumberCheckStatus
                                if [ "$NumberCheckStatus" == "1" ]; then 
                                    NumberCheck "$HardErrorCheckColumnValue"
                                    IsNumber=$?

                                    NullCheck "$HardErrorCheckColumnValue"
                                    IsNull=$?

                                    SpaceCheck "$HardErrorCheckColumnValue"
                                    IsSpace=$?

                                fi

                                #echo $SpaceCheckStatus
                                if [ "$SpaceCheckStatus" == "1" ]; then 
                                    SpaceCheck "$HardErrorCheckColumnValue"
                                    IsSpace=$?
                                fi

                                #echo $ValidCheckStatus

                                if [ "$ValidCheckStatus" == "1" ]; then
                                    ValidCheck "$HardErrorCheckColumnValue" "$ValidValues"
                                    IsValid=$?                              
                                fi

                                if [ $IsNull -eq 10 ] || [ $IsNumber -eq 10 ] || [ $IsSpace -eq 10 ] || [ $IsValid -eq 10 ]; then

                                    HardErrorFlag=10

                                    if [ $IsNull -eq 10 ]; then
                                        ErrorText=$ErrorText" $ColumnName is Null or Invalid; "
                                    fi
                                    if [ $IsNumber -eq 10 ]; then

                                        ErrorText=$ErrorText" $ColumnName is not a valid number; "
                                    fi
                                    if [ $IsSpace -eq 10 ]; then
                                        ErrorText=$ErrorText" $ColumnName is Space or Invalid; "
                                    fi

                                    if [ $IsValid -eq 10 ]; then
                                        ErrorText=$ErrorText" $ColumnName is not a valid value; "
                                    fi

                                fi  
                            fi

                            # Soft Error Check -

                            if [ "$ErrorCheck" == "S" ]; then
                                #echo "Doing Soft Error"
                                IsSValid=1
                                IsSNull=1;
                                IsSNumber=1;
                                IsSSpace=1;


                                SoftErrorCheckColumnValue=$(echo "$RowOne" | cut -d'|' -f$ColNum)
                                #echo "Value is $SoftErrorCheckColumnValue."

                                #Funtions Return 10 if it is NULL/SPACE/NOT A NUMBER

                                #echo $NullCheckStatus
                                if [ "$NullCheckStatus" == "1" ]; then 
                                    NullCheck "$SoftErrorCheckColumnValue"
                                    IsSNull=$?
                                fi

                                #echo $NumberCheckStatus
                                if [ "$NumberCheckStatus" == "1" ]; then 
                                    NumberCheck "$SoftErrorCheckColumnValue"
                                    IsSNumber=$?

                                    NullCheck "$SoftErrorCheckColumnValue"
                                    IsSNull=$?

                                    SpaceCheck "$SoftErrorCheckColumnValue"
                                    IsSSpace=$?

                                fi

                                #echo $SpaceCheckStatus
                                if [ "$SpaceCheckStatus" == "1" ]; then 
                                    SpaceCheck "$SoftErrorCheckColumnValue"
                                    IsSpace=$?
                                fi


                                #echo $ValidCheckStatus

                                if [ "$ValidCheckStatus" == "1" ]; then
                                    ValidCheck "$SoftErrorCheckColumnValue" "$ValidValues"
                                    IsSValid=$?
                                fi

                                if [ $IsSNull -eq 10 ] || [ $IsSNumber -eq 10 ] || [ $IsSSpace -eq 10 ] || [ $IsSValid -eq 10 ]; then

                                    SoftErrorFlag=10


                                    if [ $IsSNull -eq 10 ]; then
                                        SoftErrorText=$SoftErrorText" $ColumnName is Null or Invalid; "
                                    fi
                                    if [ $IsSNumber -eq 10 ]; then

                                        SoftErrorText=$SoftErrorText" $ColumnName is not a valid number; "
                                    fi
                                    if [ $IsSSpace -eq 10 ]; then
                                        SoftErrorText=$SoftErrorText" $ColumnName is Space or Invalid; "
                                    fi

                                    if [ $IsSValid -eq 10 ]; then
                                        SoftErrorText=$SoftErrorText" $ColumnName is not a valid value; "

                                    fi

                                    SoftErrThreshHold=$(expr $SoftErrThreshHold + 1)
                                fi

                            fi

                        done <$ListFilePath/$MasterFile

                        if [ $HardErrorFlag -eq 10 ]; then
                            echo $ErrorText

                        fi

                        if [ $SoftErrorFlag -eq 10 ]; then
                            echo $SoftErrorText

                        fi
            fi
        done <$SrcFilePath/$SrcFileName

if [ $SoftErrThreshHold -eq 0 ]; then
        echo "No Soft Error was found in the file"
else
        echo "Total Count Of SofErrors Found : $SoftErrThreshHold"
fi
share|improve this question
    
potentially unpopular opinion: Any business logic in bash will 'feel' complicated and unmaintainable. Why not use a different programming language? For example: many languages have CSV modules, so you'll no longer have to do your own parsing. –  Gerard May 25 at 13:59
    
Even i know it can be done easily with many other languages. But this is how my requirement is and i have to do it on UNIX only. –  Tushar Kesarwani May 25 at 14:42

1 Answer 1

up vote 2 down vote

This is extremely overcomplicated Bash code, violating good UNIX principles in terms of using exit codes.

Get rid of NullCheck

This function is literally causing more harm than help. Here's a code using NullCheck:

NullCheck "$value"
t=$?
if [ "$t" != "10" ]; then

This code, without NullCheck, is actually exactly the same thing:

if [ "$value" ]; then

There are so many related problems here:

  • The convention is 0 exit code for success, and 1 is the most common exit code for failure. The NullCheck method using 1 and 10, respectively, for these purposes is very confusing. All users of NullCheck must remember this, and effectively memorize a poor design choice that goes against good UNIX practices

  • The implementation of NullCheck is 12 lines of messy code. The result of the function is not usable directly, users much check the value of $?, which is extra hassle, and typically at least an extra line of code, in contrast with a direct "$value" check as above

To get rid of NullCheck, you would need to rewrite all uses. The result will be simpler and more intuitive, but still a lot of tedious work. For example this kind of code:

NullCheck "$SoftErrorCheckColumnValue"
IsSNull=$?
# ...
if [ $IsSNull -eq 10 ]

... could be rewritten as:

IsSNonNull="$SoftErrorCheckColumnValue"
# ...
if [ ! "$IsSNonNull" ]

Notice how one line is saved, and no more magic number 10. However, the logic had to be reversed, from thinking in terms of IsSNull to IsSNonNull.

Cleaning up SpaceCheck

Here's a simpler implementation of this function:

SpaceCheck() {
    test $(echo $1)
}

This exits with success (code 0) if the parameter contains non-whitespace characters, otherwise it exits with failure (code 1).

Instead of using the original SpaceCheck function like this:

SpaceCheck "$value"
t1=$?
if [ "$t1" == "10" ]; then
    value="SPACE"
fi

You could use like this:

if ! SpaceCheck "$value"; then
    value=SPACE
fi

Notice the ! there: success of SpaceCheck means the parameter contained non-space characters.

This usage of ! would not be possible with the original exit codes of 1 and 10. Non-0 exit codes all mean failure, so ! SpaceCheck with the old implementation is always false.

Improving performance

Bash scripts are not meant to do complicated things. Complicated Bash scripts are significantly harder to write well than programs written in higher level languages. To keep complexity down, it's absolutely crucial to write Bash scripts well, and this script would take a lot of work to get there.

My suggestions above should give you some good ideas that you can apply to other parts of the script as well. Before thinking about performance, I'd rather you rewrite the script by applying these ideas, and post the revised version as a new question.

That being said, Bash is the wrong tool for this job. To @Gerard's suggestion in a comment, you responded:

this is how my requirement is and i have to do it on UNIX only

However, at the very least, you can use awk, since your script is already using it, and it would be much better. awk is perfectly well-suited for processing | delimited files.

So my bottom-line suggestion is to abandon this script entirely. My usual recommendation for complex scripts that push Bash' limits is Python. If that's not an option for you, then try awk or perl instead.

share|improve this answer
    
Your SpaceCheck implementation does not seem to be robust. For example, what if $1 is a = b or 1 -lt 0? –  Gareth Rees Jun 17 at 10:51
    
I think my implementation behaves as the original, nothing more, nothing less. Do you disagree? –  janos♦ Jun 17 at 12:01
    
If so, the original was wrong too. Something like echo -n "$1" | grep -q [^[:space:]] is needed. –  Gareth Rees Jun 17 at 12:59
    
Given the implementation, I assumed the intention was to check that the parameter has something other than whitespace, and I guessed that the function is poorly named. I admittedly didn't try to see beyond for the ultimate purpose. Since the posted code is assumed to be fully working, I assumed that implementation == intention, and chose to disregard naming. –  janos♦ Jun 17 at 13:26

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.