Forum: Mikrocontroller und Digitale Elektronik Funktion so ok?


Announcement: there is an English version of this forum on EmbDev.net. Posts you create there will be displayed on Mikrocontroller.net and EmbDev.net.
von Ingo L. (corrtexx)


Bewertung
0 lesenswert
nicht 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?

: Bearbeitet durch User
von Micha (Gast)


Bewertung
0 lesenswert
nicht lesenswert
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.

von Peter D. (peda)


Bewertung
1 lesenswert
nicht 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.

von Eric B. (beric)


Bewertung
0 lesenswert
nicht 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.

von a.s (Gast)


Bewertung
-1 lesenswert
nicht 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.

von Arno (Gast)


Bewertung
0 lesenswert
nicht 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

von Ingo L. (corrtexx)


Bewertung
0 lesenswert
nicht 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...

von Tom (Gast)


Bewertung
0 lesenswert
nicht 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
}

von Ingo L. (corrtexx)


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

Antwort schreiben

Die Angabe einer E-Mail-Adresse ist freiwillig. Wenn Sie automatisch per E-Mail über Antworten auf Ihren Beitrag informiert werden möchten, melden Sie sich bitte an.

Wichtige Regeln - erst lesen, dann posten!

  • Groß- und Kleinschreibung verwenden
  • Längeren Sourcecode nicht im Text einfügen, sondern als Dateianhang

Formatierung (mehr Informationen...)

  • [c]C-Code[/c]
  • [code]Code in anderen Sprachen, ASCII-Zeichnungen[/code]
  • [math]Formel in LaTeX-Syntax[/math]
  • [[Titel]] - Link zu Artikel
  • Verweis auf anderen Beitrag einfügen: Rechtsklick auf Beitragstitel,
    "Adresse kopieren", und in den Text einfügen




Bild automatisch verkleinern, falls nötig
Bitte das JPG-Format nur für Fotos und Scans verwenden!
Zeichnungen und Screenshots im PNG- oder
GIF-Format hochladen. Siehe Bildformate.
Hinweis: der ursprüngliche Beitrag ist mehr als 6 Monate alt.
Bitte hier nur auf die ursprüngliche Frage antworten,
für neue Fragen einen neuen Beitrag erstellen.

Mit dem Abschicken bestätigst du, die Nutzungsbedingungen anzuerkennen.