1
\$\begingroup\$

How is the logic, efficiency and can it be improved?

This is a snippet from my .bashrc file, I just wrote it.

The code is supposed to do this:

Backup my .bash_history file if it is 1000 lines or less from the defined size of HISTSIZE=100000 and HISTFILESIZE=100000 lines, then backup the file.

  1. Check if .bash_history is 99000 lines or more.
  2. Check if ~/.bash_history.old exists if it doesn't use that filename.
  3. Increment i one digit larger than is already used by filenames.(filename-4.old)
  4. Check if there are more than 20 files already backed up and warn if it is.
  5. Set the new filename into a variable.
  6. Check if the last file with a digit in it's filename is older than the original.
  7. cp the file to new_name

Here is the code:

# Count the number of lines in `/.bash_history and check if it's equal or more than a thousand
if (( $(wc -l < ~/.bash_history) >= 99000 ))
then
    name=~/.bash_history
    # Here is the suffix of the new files and the ones I check for
    old=.old
    # -e FILE ==> True if FILE exists.
    if [[ ! -e ~/.bash_history.old ]]
    then
       printf "%s\n" "##################################################" ".bash_history will be cleared soon, backing up....!" "##################################################" 
       # Here I copy $name which is ~/.bash_history and create backup file
       cp $name ~/.bash_history.old
    else
        # i is the increment in the filenames to be checked and created
        i=0
        # $name$old is ~/.bash_history.old
        if [[ -e $name$old ]]
        then
            # Here I count how many copies there are with digits in the filename
            while [[ -e "$name-$i$old" ]]
            do
                let i++
            done
        fi
        # if there are 20 files already backed up then I need to archive them somewhere else
        if [[ "$i" -ge 20 ]]
        then
            printf "%s\n" "********************************************************" "You need to arhive your history files they are mounting up!!!" "**************************************************************"
        fi
        new_name=$name-$i$old
        minus=$(( i - 1 ))
        if [ $name -nt "$name-$minus$old" ]
        then
            printf "%s\n" "##################################################" ".bash_history will be cleared soon, backing up....!" "##################################################"
            cp ~/.bash_history "$new_name"
        fi
    fi
fi

This is the result from shellcheck.net:

Line 16:
                let i++
                ^-- SC2219: Instead of 'let expr', prefer (( expr )) .
\$\endgroup\$

closed as off-topic by yuri, pacmaninbw, Graipher, Heslacher, esote Jun 7 at 0:56

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – yuri, pacmaninbw, Graipher, Heslacher, esote
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ This question is off-topic because you don't know whether it works. We are happy to review working code. \$\endgroup\$ – dfhwze Jun 5 at 6:50
  • \$\begingroup\$ @dfhwze "how well it works" != "whether it works". "The code is supposed to do this" is debatable. \$\endgroup\$ – l0b0 Jun 5 at 6:51
  • 1
    \$\begingroup\$ @l0b0 "Does my code work and can it be improved?" and "The code is supposed to do this:" tend to make me think "how well it works" is out of the question. \$\endgroup\$ – dfhwze Jun 5 at 6:53
  • 2
    \$\begingroup\$ That's probably something OP can help us with…? \$\endgroup\$ – l0b0 Jun 5 at 7:01
  • \$\begingroup\$ @Heslacher I cleaned the question and added comments to the code....... Is it any better like this then or what? \$\endgroup\$ – somethingSomething Jun 7 at 23:04
3
\$\begingroup\$

First, logrotate is a tool built to do exactly this sort of thing. I would recommend using that to achieve the effectively the same extremely reliably and with lots of configuration options. That said, some suggestions on the code as written:

  • ShellCheck is great; I would recommend following its recommendation to use (( i++ )).
  • wc -l counts the number of lines, not bytes or kilobytes, which seems to be what you want.
  • old, minus and i are not helpful names; I have to read and understand all the code in the context in order to understand what they mean.
  • It looks like you only ever replace the last of the 20 files once you have 20 backups.
  • You have five instances of ~/.bash_history, even though one of them is the value of a variable. I would pull that variable out and reuse it everywhere.
  • Use More Quotes™ - it's good for you and the code.
  • Rather than a special unnumbered backup file (~/.bash_history.old), why not just start numbering the backups immediately? That way you can get rid of at least two checks for whether that file exists ([[ ! -e ~/.bash_history.old ]] and its inverse, [[ -e $name$old ]]).
\$\endgroup\$

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