Funktion so ok?

[Ingo L. (corrtexx)]

Hallo,

ich habe folgende Funktion:

uint8_t CheckValidFrequency ( void )
{
  static uint8_t GridErrorCounter = 0;
  
  if ( Frequency.ist > ISLAND_GD_FREQUENCY_MIN && Frequency.ist < ISLAND_GD_FREQUENCY_MAX ){
    GridErrorCounter = 0;
    return GD_OK;
  }
  
  /* Set "!" to Display to show that a grid error accoured */
  Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
  
  /* Grid Error, but count before acting */  
  if ( GridErrorCounter < GD_ERROR_COUNT_MAX ) GridErrorCounter++;
  else return GD_ERROR;
  
  return GD_OK;
}

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?

[Micha (Gast)]

Ingo L. schrieb:
> Normalerweise hat ja eine Funktion nur einen Austrittspunkt oder geht
> das so in Ordnung?

Spricht nix dagegen. Solange die Funktion das tut was du willst.

[Peter D. (peda)]

Ich bevorzuge:

  if ( GridErrorCounter < GD_ERROR_COUNT_MAX )
    GridErrorCounter++;
  else
    return GD_ERROR;

oder

  if ( GridErrorCounter >= GD_ERROR_COUNT_MAX )
    return GD_ERROR;
  GridErrorCounter++;

Mehrere Returns bei mehreren Abbruchbedingungen finde ich auch klarer.

[Eric B. (beric)]

Hängt von der im Projekt festgelegten "Coding Guidelines" ab. Ich habe 
lieber Funktionen mit nur einen Austrittspunkt un würde dies so angehen:

uint8_t checkValidFq(void)
{
    static uint8_t GridErrorCounter = 0;
  
    uint8_t retval = GD_ERROR;

    if ( Frequency.ist > ISLAND_GD_FREQUENCY_MIN 
      && Frequency.ist < ISLAND_GD_FREQUENCY_MAX ) {
        GridErrorCounter = 0;
        retval = GD_OK;
    } else {
        /* Set "!" to Display to show that a grid error accoured */
        Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
  
        /* Grid Error, but count before acting */  
        if ( GridErrorCounter < GD_ERROR_COUNT_MAX ) 
        {
            GridErrorCounter++;
            retval = GD_OK;
        }
    }

    return retval;
}

(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.

[a.s (Gast)]

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, ...)

uint8_t CheckValidFrequency ( void )
{
static uint8_t GridErrorCounter = 0;
uint8_t ret;

   if(   Frequency.ist > ISLAND_GD_FREQUENCY_MIN 
      && Frequency.ist < ISLAND_GD_FREQUENCY_MAX)
   {
      GridErrorCounter = 0;
      ret = GD_OK;
   }
   else
   {
      /* Set "!" to Display to show that a grid error accoured */
      Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
      /* Grid Error, but count before acting */
      if(GridErrorCounter < GD_ERROR_COUNT_MAX) 
      {
         GridErrorCounter++;
         ret = GD_OK;
      }
      else
      {
         ret = GD_ERROR;
      }  
   }
   return ret;
}

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.

[Arno (Gast)]

...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

[Ingo L. (corrtexx)]

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...

[Tom (Gast)]

Statt ret o.ä. mitzuschleppen, kann man einmal am Ende prüfen, ob die 
Fehlerbedingung gegeben ist:

uint8_t CheckValidFrequency ( void )
{
    static uint8_t GridErrorCounter = 0;
    if (Frequency.ist > ISLAND_GD_FREQUENCY_MIN && Frequency.ist < ISLAND_GD_FREQUENCY_MAX)
    {
        /* note: Flags.GridErrorDisplay is not reset here because ... */
        GridErrorCounter = 0;
    }
    else
    {
        /* Set "!" to Display to show that a grid error accoured */
        Flags.GridErrorDisplay = GD_ERROR_LCD_TIME;
        if ( GridErrorCounter < UINT8_MAX) // prevent overflow
            GridErrorCounter++;
    }

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

[Ingo L. (corrtexx)]

Wieder mal erstaunlich wie viele Lösung es für so eine triviale Funktion 
geben kann...