1
\$\begingroup\$

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.

\$\endgroup\$
  • 5
    \$\begingroup\$ Please don't put line numbers in code snippets. That makes it much harder for people like me to cut and paste your code \$\endgroup\$ – glenn jackman Nov 25 '18 at 14:50
1
\$\begingroup\$

I'm confused about your goal for this question:

  1. performance is bad
  2. performance isn't terrible
  3. improvements would be appreciated
  4. 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 
\$\endgroup\$
1
\$\begingroup\$

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"
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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