Funktion so ok?

OP #4987651
Lesenswert?

Hallo,

ich habe folgende Funktion:
1
uint8_t CheckValidFrequency ( void )
2
{
3
  static uint8_t GridErrorCounter = 0;
4
  
5
  if ( Frequency.ist > ISLAND_GD_FREQUENCY_MIN && Frequency.ist < ISLAND_GD_FREQUENCY_MAX ){
6
    GridErrorCounter = 0;
7
    return GD_OK;
8
  }
9
  
10
  /* Set "!" to Display to show that a grid error accoured */
11
  Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
12
  
13
  /* Grid Error, but count before acting */  
14
  if ( GridErrorCounter < GD_ERROR_COUNT_MAX ) GridErrorCounter++;
15
  else return GD_ERROR;
16
  
17
  return GD_OK;
18
}
Die Funktion prüft, ob der Parameter Frequency.ist im zulässigen Bereich 
liegt. Dafür hat die Funktion 2 Rückgabewerte, jedoch drei 
Austrittspunkte:
1. Frequenz ok => GD_OK
2. Frequenz nicht ok, aber noch nicht lang genug nicht ok => GD_OK
3. Frequenz lang genug nicht ok => GD_ERROR

Normalerweise hat ja eine Funktion nur einen Austrittspunkt oder geht 
das so in Ordnung?
#4987668
Lesenswert?

Ich bevorzuge:
1
  if ( GridErrorCounter < GD_ERROR_COUNT_MAX )
2
    GridErrorCounter++;
3
  else
4
    return GD_ERROR;
oder
1
  if ( GridErrorCounter >= GD_ERROR_COUNT_MAX )
2
    return GD_ERROR;
3
  GridErrorCounter++;

Mehrere Returns bei mehreren Abbruchbedingungen finde ich auch klarer.
#4987669
Lesenswert?

Hängt von der im Projekt festgelegten "Coding Guidelines" ab. Ich habe 
lieber Funktionen mit nur einen Austrittspunkt un würde dies so angehen:
1
uint8_t checkValidFq(void)
2
{
3
    static uint8_t GridErrorCounter = 0;
4
  
5
    uint8_t retval = GD_ERROR;
6

7
    if ( Frequency.ist > ISLAND_GD_FREQUENCY_MIN 
8
      && Frequency.ist < ISLAND_GD_FREQUENCY_MAX ) {
9
        GridErrorCounter = 0;
10
        retval = GD_OK;
11
    } else {
12
        /* Set "!" to Display to show that a grid error accoured */
13
        Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
14
  
15
        /* Grid Error, but count before acting */  
16
        if ( GridErrorCounter < GD_ERROR_COUNT_MAX ) 
17
        {
18
            GridErrorCounter++;
19
            retval = GD_OK;
20
        }
21
    }
22

23
    return retval;
24
}
(und ich würde noch
* Frequency.ist der Funktion als Parameter mitübergeben
* GD_OK, GD_ERROR usw in einem enum typedef-en und das als 
Rückgabetype angeben
* checken ob es nicht Frequency.ist >= ISLAND_GD_FREQUENCY_MIN sein 
muss.
Gast #4987674
Lesenswert?

Das geht so in Ordnung, wenn ihr alle so programmiert.

Bei dieser Funktion wäre es mit einem Austrittspunkt aber 
übersichtlicher.

Da Du z.B. mit geschweiften klammern "{}" sehr sparsam bist, nehme ich, 
dass Du nicht mit Lint oder Compiler-Warnungen arbeitest. Daher wird es 
auch kaum jemanden interessieren (also kein Peer-Review, Misra, ...)
1
uint8_t CheckValidFrequency ( void )
2
{
3
static uint8_t GridErrorCounter = 0;
4
uint8_t ret;
5

6
   if(   Frequency.ist > ISLAND_GD_FREQUENCY_MIN 
7
      && Frequency.ist < ISLAND_GD_FREQUENCY_MAX)
8
   {
9
      GridErrorCounter = 0;
10
      ret = GD_OK;
11
   }
12
   else
13
   {
14
      /* Set "!" to Display to show that a grid error accoured */
15
      Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
16
      /* Grid Error, but count before acting */
17
      if(GridErrorCounter < GD_ERROR_COUNT_MAX) 
18
      {
19
         GridErrorCounter++;
20
         ret = GD_OK;
21
      }
22
      else
23
      {
24
         ret = GD_ERROR;
25
      }  
26
   }
27
   return ret;
28
}
Das explizite Setzen von ret in jedem Pfad erlaubt es statischen 
Analysetools festzustellen, ob man ein bewusstes setzen vergessen hat. 
Mit GD_OK als Default entfielen 2 Zeilen.

Nochmal: Ich habe nichts gegen mehrere Austrittspunkte. Zumal es 
Anwendungen gibt, die Code und Schachteltiefe enorm reduzieren. Das ist 
aber hier nicht der Fall.
Gast #4987675
Lesenswert?

...und viel länger sollte die Funktion aus Gründen der Übersicht IMHO 
auch nicht werden, wenn sie mehrere return-Statements enthält. Oder 
anders ausgedrückt: Längere Funktionen als ca. eine Bildschirmseite 
sollten IMHO nur dann mehrere return-Statements haben, wenn man das 
sowohl auf der ersten als auch auf der letzten Bildschirmseite der 
Funktion offensichtlich sieht. Etwa im Stil eines "Guards": 
http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement

Sonst besteht zu leicht die Gefahr, dass man bei der Wartung nicht alle 
return-Statements findet.

MfG, Arno
OP #4987679
Lesenswert?

Danke erstmal

Eric B. schrieb:
> * Frequency.ist der Funktion als Parameter mitübergeben
Ok, habe ich gemacht, auch wenns nicht nötig ist, spart 2 Byte Flash ;)

> * GD_OK, GD_ERROR usw in einem enum typedef-en und das als
> Rückgabetype angeben
Nee, ich mag enums nicht gerne

> checken ob es nicht Frequency.ist >= ISLAND_GD_FREQUENCY_MIN sein
> muss.
Ja ok, is n Argument...
Gast #4987682
Lesenswert?

Statt ret o.ä. mitzuschleppen, kann man einmal am Ende prüfen, ob die 
Fehlerbedingung gegeben ist:
1
uint8_t CheckValidFrequency ( void )
2
{
3
    static uint8_t GridErrorCounter = 0;
4
    if (Frequency.ist > ISLAND_GD_FREQUENCY_MIN && Frequency.ist < ISLAND_GD_FREQUENCY_MAX)
5
    {
6
        /* note: Flags.GridErrorDisplay is not reset here because ... */
7
        GridErrorCounter = 0;
8
    }
9
    else
10
    {
11
        /* Set "!" to Display to show that a grid error accoured */
12
        Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
13
        if ( GridErrorCounter < UINT8_MAX) // prevent overflow
14
            GridErrorCounter++;
15
    }
16

17
    return (GridErrorCounter < GD_ERROR_COUNT_MAX) ? GD_OK : GD_ERROR;
18
}

Antwort schreiben

Bitte melde dich an, um einen Beitrag zu schreiben.

oder

Mit Google-Account einloggen

Die Registrierung ist kostenlos und dauert nur eine Minute.

Jetzt registrieren