Bad performance bash, changing display brightness

Question

I have a script that changes the display brightness using xrandr. It is my first bash script. I mainly had a lot of trouble with dealing with the floating point values. I think that is one of the main reasons the performance is bad.

  1 #!/bin/bash                                                                                      
  2 # increase/decrease display brightness of xrandr of both displays                                
  3 # use:                                                                                           
  4 # $ ./change_brightness [up/down (0/1)]                                                          
  5                                                                                                  
  6 export current_brightness=$(xrandr --verbose | awk '/Brightness/ { print $2; exit }')            
  7                                                                                                  
  8 export increase=$1                                                                               
  9 export new_brightness=$current_brightness                                                        
 10                                                                                                  
 11 if [ "$increase" = "1" ]; then                                                                   
 12     if [ $current_brightness != "1.0" ]; then                                                    
 13         new_brightness="$current_brightness + 0.2"                                               
 14     fi                                                                                           
 15 elif [ "$increase" = "0" ]; then                                                                 
 16     if [ $current_brightness != "0.20" ]; then                                                   
 17         new_brightness="$current_brightness - 0.2"                                               
 18     fi                                                                                           
 19 fi                                                                                               
 20                                                                                                  
 21 new_brightness=$(bc <<< "$new_brightness")                                                       
 22                                                                                                  
 23 xrandr --output eDP-1-1 --brightness $new_brightness                                             
 24 xrandr --output DP-0 --brightness $new_brightness

The performance isn't terrible, but any improvements would be highly appreciated. Also the improvements that have nothing to do with performance.

Please don't put line numbers in code snippets. That makes it much harder for people like me to cut and paste your code — glenn jackman
– glenn jackman, Commented Nov 25, 2018 at 14:50

glenn jackman · Accepted Answer · 2018-11-25 15:34:31Z

I'm confused about your goal for this question:

performance is bad
performance isn't terrible
improvements would be appreciated
improvements have nothing to do with performance.

Nevertheless, code review notes:

you don't have to export every variable, only the ones that are needed by processes you spawn from this script.
validate inputs: why do anything if the input is neither "0" nor "1"?

Here's my take:

#!/bin/bash                                                                                      
# increase/decrease display brightness of xrandr of both displays                                
# use:                                                                                           
# $ ./change_brightness [up/down (0/1)]                                                          

current_brightness=$(xrandr --verbose | awk '/Brightness/ { print $2; exit }')            
case $1 in
    0) direction=-1;;
    1) direction=1;;
    *) echo "some error message"; exit 1;;
esac

export current_brightness direction
new_brightness=$( perl -E '
    use List::Util qw( min max );
    say max(0, min($ENV{current_brightness} + $ENV{direction} * 0.2, 1.0));
')

xrandr --output eDP-1-1 --brightness $new_brightness                                             
xrandr --output DP-0 --brightness $new_brightness

janos · Accepted Answer · 2018-11-25 22:49:03Z

Validate your inputs

As @glenn-jackman pointed out, validate your inputs. If the script argument should be either 0 or 1, then enforce that rule, by checking the value, and exit with an error if it's some other value. It's always good to fail early, for example to avoid confusing crashes later due to an unexpected value, or to avoid doing unnecessary work.

Look for corner cases

The script increases or decreases brightness in increments of 0.2, unless the value is 1.0 or 0.2. This is fragile. What if the current value is 0.9? The script will not prevent going to 1.1, 1.3, ..., or 0.1, -0.1, ..., and so on. This is a bug waiting to happen.

To correctly enforce upper and lower limits, you need an appropriate technique. As you discovered, Bash cannot do floating point math. I would recommend Awk instead. It's lightweight and universally available.

Always double-quote variables in program arguments

As a good rule of thumb, always double-quote variables used in program arguments, for example here:

if [ $current_brightness != "1.0" ]; then                                                    
...

xrandr --output eDP-1-1 --brightness $new_brightness

Write like this instead:

if [ "$current_brightness" != "1.0" ]; then                                                    
...

xrandr --output eDP-1-1 --brightness "$new_brightness"

Stack Exchange Network

Bad performance bash, changing display brightness

2 Answers 2

Validate your inputs

Look for corner cases

Always double-quote variables in program arguments

You must log in to answer this question.

Hot Network Questions

Bad performance bash, changing display brightness

2 Answers 2

Validate your inputs

Look for corner cases

Always double-quote variables in program arguments

You must log in to answer this question.

Related

Hot Network Questions