how to avoid duplicated code?

Hi, i’m italian, sorry for my bad english.
I wrote a code for the adjustment of a clock but i repeated the same code for each button
i’ve three button:
1 increase the value of hour, minute, day, etc…
2 decrease the value of hour, minute, day, etc…
3 “ok”. confirm value modified and move to the next value to be changed
This is a part of the code:

//-------------------------FUNZIONE PER MODIFICA ORA E DATA-----------------------------
void modifica_ora(){
  lcd.clear();
  lcd.setCursor(0, 0);
  lcd.print("Imposta data e ora");

  while (true){
    
    Puls_Inc = digitalRead(51);
    Puls_Dec = digitalRead(49);
    Puls_Ok  = digitalRead(45);

//-----------------------------INCREMENTA ORA--------------------------------  
    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 0){
      Set_HR = Set_HR + 1;
       if (Set_HR >23){
        Set_HR = 0;
        }
     RTC.set(DS1307_HR,Set_HR);  
     Puls_Inc_Old = Puls_Inc;
      }  
    if (Puls_Inc == LOW && Puls_Inc_Old == HIGH){    
     Puls_Inc_Old = Puls_Inc;
      }

//----------------------------DECREMENTA ORA-----------------------------------            
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 0){
      Set_HR = Set_HR - 1;
       if (Set_HR <0){
        Set_HR = 23;
        }
     RTC.set(DS1307_HR,Set_HR);  
     Puls_Dec_Old = Puls_Dec;
      }  
    if (Puls_Dec == LOW && Puls_Dec_Old == HIGH){    
     Puls_Dec_Old = Puls_Dec;
      }
      
      
//-----------------------------INCREMENTA MINUTI--------------------------------  
    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 1){
     Set_MIN = Set_MIN + 1;
       if (Set_MIN >59){
        Set_MIN = 0;
        }
     RTC.set(DS1307_MIN,Set_MIN);  
     Puls_Inc_Old = Puls_Inc;
      }  
    if (Puls_Inc == LOW && Puls_Inc_Old == HIGH){    
     Puls_Inc_Old = Puls_Inc;
      }

//----------------------------DECREMENTA MINUTI-----------------------------------            
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 1){
      Set_MIN = Set_MIN - 1;
       if (Set_MIN <0){
        Set_MIN = 59;
        }
     RTC.set(DS1307_MIN,Set_MIN);  
     Puls_Dec_Old = Puls_Dec;
      }  
    if (Puls_Dec == LOW && Puls_Dec_Old == HIGH){    
     Puls_Dec_Old = Puls_Dec;
      }      
      
//-----------------------------INCREMENTA DATA--------------------------------  
    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 2){
     Set_DATA = Set_DATA + 1;
       if (Set_DATA >31){
        Set_DATA = 0;
        }
     RTC.set(DS1307_DATE,Set_DATA);  
     Puls_Inc_Old = Puls_Inc;
      }  
    if (Puls_Inc == LOW && Puls_Inc_Old == HIGH){    
     Puls_Inc_Old = Puls_Inc;
      }

//----------------------------DECREMENTA DATA-----------------------------------            
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 2){
      Set_DATA = Set_DATA - 1;
       if (Set_DATA <0){
        Set_DATA = 31;
        }
     RTC.set(DS1307_DATE,Set_DATA);  
     Puls_Dec_Old = Puls_Dec;
      }  
    if (Puls_Dec == LOW && Puls_Dec_Old == HIGH){    
     Puls_Dec_Old = Puls_Dec;
      }  
      
//-----------------------------INCREMENTA MESE--------------------------------  
    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 3){
     Set_MESE = Set_MESE + 1;
       if (Set_MESE >12){
        Set_MESE = 0;
        }
     RTC.set(DS1307_MTH,Set_MESE);  
     Puls_Inc_Old = Puls_Inc;
      }  
    if (Puls_Inc == LOW && Puls_Inc_Old == HIGH){    
     Puls_Inc_Old = Puls_Inc;
      }

//----------------------------DECREMENTA MESE-----------------------------------            
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 3){
      Set_MESE = Set_MESE - 1;
       if (Set_MESE <0){
        Set_MESE = 12;
        }
     RTC.set(DS1307_MTH,Set_MESE);  
     Puls_Dec_Old = Puls_Dec;
      }  
    if (Puls_Dec == LOW && Puls_Dec_Old == HIGH){    
     Puls_Dec_Old = Puls_Dec;
      }    
      
//-----------------------------INCREMENTA ANNO--------------------------------  
    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 4){
     Set_ANNO = Set_ANNO + 1;
       if (Set_ANNO >99){
        Set_ANNO = 0;
        }
     RTC.set(DS1307_YR,Set_ANNO);  
     Puls_Inc_Old = Puls_Inc;
      }  
    if (Puls_Inc == LOW && Puls_Inc_Old == HIGH){    
     Puls_Inc_Old = Puls_Inc;
      }

//----------------------------DECREMENTA ANNO-----------------------------------            
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 4){
      Set_ANNO = Set_ANNO - 1;
       if (Set_ANNO <0){
        Set_ANNO = 99;
        }
     RTC.set(DS1307_YR,Set_ANNO);  
     Puls_Dec_Old = Puls_Dec;
      }  
    if (Puls_Dec == LOW && Puls_Dec_Old == HIGH){    
     Puls_Dec_Old = Puls_Dec;
      }

how to avoid duplicated code?
even if my English is not perfect, I hope you understand me :-/

Whenever you copy and paste code, it's time to think about creating a function, instead. You have similar code, except for adding or subtracting, to change the hours, minutes, and seconds. Create a function, ChangeHour(), that takes a +1 or -1 argument, and adjust hours based on that argument. Do the same for minutes and seconds. Then, look at the similarities in those functions. See if you can create a single function to handle hours, minutes, or seconds.

Thanks PaulS for the reply!!! I had imagined that i need to use a function, but i've much problems. Could you give me an example? I don't understand how to use one function for more buttons.

Start with the code that increments or decrements hours:

    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 0){
      [glow]Set_HR = Set_HR + 1;
       if (Set_HR >23){
        Set_HR = 0;
        }
     RTC.set(DS1307_HR,Set_HR);[/glow]
     Puls_Inc_Old = Puls_Inc;
      }
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 0){
[glow]      Set_HR = Set_HR - 1;
       if (Set_HR <0){
        Set_HR = 23;
        }
     RTC.set(DS1307_HR,Set_HR);[/glow]  
     Puls_Dec_Old = Puls_Dec;
      }

The highlighted code either increments the hour variable or decrements it, assures that the resulting value is still in its proper range, and then sets the new time.

That code could go in a function:

ChangeHour(byte incOrDec)
{
   Set_HR += incOrDec;
   if(Set_HR > 23)
      Set_HR = 0;
   else if(Set_HR < 0)
      Set_HR = 23;
   RTC.set(DS1307_HR,Set_HR);
}

Now, the function can be called:

    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 0){
      ChangeHour(+1);
     Puls_Inc_Old = Puls_Inc;
      }  
    if (Puls_Inc == LOW && Puls_Inc_Old == HIGH){    
     Puls_Inc_Old = Puls_Inc;
      }

//----------------------------DECREMENTA ORA-----------------------------------            
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 0){
      ChangeHour(-1);
     Puls_Dec_Old = Puls_Dec;
      }  
    if (Puls_Dec == LOW && Puls_Dec_Old == HIGH){    
     Puls_Dec_Old = Puls_Dec;
      }

Notice, too, that you reset Puls_Dec_Old whenever the button state changes. It can be reset even if the button state does not change. The thing is that you want to detect when the button state is not the same as it was. It isn’t like you need to do something different when the button is released. You only care when it is pressed now but wan’t before. So that whole block of code could look like:

    if (Puls_Inc == HIGH && Puls_Inc_Old == LOW && Move_Set == 0){
      ChangeHour(+1);
      }  
     Puls_Inc_Old = Puls_Inc;

//----------------------------DECREMENTA ORA-----------------------------------            
    if (Puls_Dec == HIGH && Puls_Dec_Old == LOW && Move_Set == 0){
      ChangeHour(-1);
      }  
     Puls_Dec_Old = Puls_Dec;

You can make similar changes to the blocks of code that set minutes, seconds, days, months, and years, too.

There is a button library that handles much of the complexity of debouncing buttons (which it doesn’t appear that you are handling) and keeping track of the changes in the button states.

If you were to use the button library, your code could then be even shorter.

What Paul wrote is true, but your code isn't the best candidate to use functions because it isn't all equal. However, you can restructure your code a little to make it more readable and gets rid of most duplication (as usual, completely untested)

//-------------------------FUNZIONE PER MODIFICA ORA E DATA-----------------------------
void modifica_ora(){
    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print("Imposta data e ora");

    while (true){

        Puls_Inc = digitalRead(51);
        Puls_Dec = digitalRead(49);
        Puls_Ok  = digitalRead(45);

        // Check what button has been pressed
        int button = 0;
        if (Puls_Inc == HIGH && Puls_Inc_Old == LOW) {
            button = 1;
        }
        else if (Puls_Dec == HIGH && Puls_Dec_Old == LOW) {
            button = 2;
        }
        // These can always be set
        Puls_Inc_Old = Puls_Inc;
        Puls_Dec_Old = Puls_Dec;

        // Do what one needs to do to the clock
        if (button) {
            switch (Move_Set) {
                case 0:    // Oral
                    switch (button) {
                        case 1:    // Increment
                            Set_HR = (Set_HR + 1) % 24;
                            break;
                        case 2:    // Decrement
                            Set_HR = (Set_HR + 23) % 24;
                            break;
                    }
                    RTC.set(DS1307_HR,Set_HR);  
                    break;
                case 1:    // Minuti
                    switch (button) {
                        case 1:    // Increment
                            Set_MIN = (Set_MIN + 1) % 60;
                            break;
                        case 2:    // Decrement
                            Set_MIN = (Set_MIN + 59) % 60;
                            break;
                    }
                    RTC.set(DS1307_MIN,Set_MIN);  
                    break;
                case 2:    // Data
                    switch (button) {
                        case 1:    // Increment
                            Set_DATA = (Set_DATA + 1) % 32;
                            if (! Set_DATA) { Set_DATA = 1; }
                            break;
                        case 2:    // Decrement
                            Set_DATA = (Set_DATA + 31) % 32;
                            if (! Set_DATA) { Set_DATA = 31; }

                            break;
                    }
                    RTC.set(DS1307_DATE,Set_DATA);  
                    break;
                case 3:    // Mese
                    switch (button) {
                        case 1:    // Increment
                            Set_MESE = (Set_MESE + 1) % 13;
                            if (! Set_MESE) { Set_MESE = 1; }

                            break;
                        case 2:    // Decrement
                            Set_MESE = (Set_MESE + 12) % 13;
                                      if (! Set_MESE) { Set_MESE = 12; }
                            break;
                    }
                    RTC.set(DS1307_MTH,Set_MESE);  
                    break;
                case 4:    // Anal
                    switch (button) {
                        case 1:    // Increment
                            Set_ANNO = (Set_ANNO + 1) % 100;
                            break;
                        case 2:    // Decrement
                            Set_ANNO = (Set_ANNO + 99) % 100;
                            break;
                    }
                    RTC.set(DS1307_YR,Set_ANNO);  
                    break;
            }
        }
    }
}

Note the use of the modulo operator (%) to handle the wrap around. of 0.

Korman

thanks guys!!! now i do some test :wink: