Codeschnipsel zusammenfassen (Übersichtlichkeit)

[Matthias L. (Gast)]

Hallo, ich brauche mal einen Tip:

Ich habe eine switch-case Schleife im Main. In jedem Case ist sehr viel 
Code untergebracht. Gibt es jetzt eine Möglichkeit, den irgendwie 
zusammenzufassen?

void main (void)
{
 ...
 switch ( x )
 {
 case 0:
   // hier liegt viel Code
 case 1:
   // hier liegt viel Code
 case 2:
   // hier liegt viel Code
 ...
}

Ich weiß, ich kann eine neue *.h und *.c Datei anlegen, und dort 
Funktionen für jeden case hinterlegen, aber da habe ich keine 
Möglichkeit, auf die Varaiblen zuzugreifen, die ich sonst in der case 
zur Verfügung habe. (lokale von main und globale von main.c)

Also ich bräuchte sowas wie #define, aber eben mit viel code..

Jemand eine Idee?

Thx

[Stefan (Gast)]

Makro?

[Stefan B. (stefan)]

Entweder Code so umschreiben, dass die Variablen auch in Funktionen 
zugänglich sind. Im switch dann die Funktionen zum Abarbeiten des case 
aufrufen.

Oder einen Editor mit Folding benutzen. Dann kann na die grossen 
Codeteile bei der Bearbeitung wegklappen und sich auf den wichtigen 
Codeausschnitt konzentrieren.

Oder mit #define arbeiten und \ am Zeilenende benutzen, um das Makro in 
der nächsten Zeile fortzusetzen.

[yalu (Gast)]

Du kannst den Code in den einzelnen Cases in Funktionen packen, die die
benötigten Variablen als Argumente entgegen nehmen.

Damit keine Effizienzverluste entstehen: Die Funktionen definierst du
als static, entweder in der gleichen Quellcodedatei, in der auch main
steht, oder in einer zweiten Datei, die von da includet wird. Dann wird
der Compiler diese Funktionen inlinen, was im gleichen Code wie bei
deiner jetzigen, unübersichtlichen Variante resultiern sollte.

[Matthias L. (Gast)]

>als Argumente entgegen nehmen

Das sind zu viele. Ich hab jetzt alle in eine globale Struktur 
geschmissen...


>in der gleichen Quellcodedatei..

dann ist die unübersichtlichkeit nur weiter oben...


>zweiten Datei, die von da includet wird..

So habe ich das jetzt angefangen. Eine c/h Datei mit je einer Funktion 
für einen case-Fall..

[STK500-Besitzer (Gast)]

>Das sind zu viele. Ich hab jetzt alle in eine globale Struktur
>geschmissen...

Was spricht dagegen einen Pointer auf diese Struktur zu übergeben?

[StinkyWinky (Gast)]

Du könntest den Code pro Case in eine Datei auslagern, dann an der 
entsprechenden Stelle includen:

void main (void)
{
 ...
 switch ( x )
 {
 case 0:
   #include "code_case_0.c"
 case 1:
   #include "code_case_1.c"
 case 2:
   #include "code_case_2.c"
 ...
}

Auf diese Art müsste nichts umgestellt werden.

[Matthias L. (Gast)]

>Du könntest den Code pro Case in eine Datei auslagern, dann an der
>entsprechenden Stelle includen:


Das geht so? Das wäre genau die Lösung...

[was-willst-du (Gast)]

geht das so?

 Ist ne ganz schön freche Frage für jemanden der keine Idee hat.

[Klaus F. (kfalser)]

Das geht, ist eine sehr schlechte Idee. Kein Editor der Welt kann kann 
dann für dich Variablen umbenennen oder alle Stellen finden, an denen 
eine Variable verwendest.
Noch schlimmer wird es, wenn du vergisst irgendwo eine Klammer zu 
schließen, dann kommt der Syntaxfehler womöglich noch im falschen File.

Die beste Empfehlung die man dir geben kann, ist wahrscheinlich das 
Programm so umzustukturieren, dass man an diesen Stellen Funktionen 
aufrufen kann. Die Tatsache, dass viele Parameter übergeben werden 
müssen, deutet eher auf ein schlechtes Design hin.
Das Zusammenfassen zu einer Struktur ist sicher eine gute Idee, warum 
dann nicht mehrere in denen die Variable, die irgendwie logisch 
zusammengehören, zusammengefasst werden.
Zur Not gibt es auch globale Variablen, diese müssen nicht übergeben 
werden.

[Matthias L. (Gast)]

>dass man an diesen Stellen Funktionen

So hab ichs jetzt auch gemacht..


>dass viele Parameter übergeben

Naja, etwa 5 Varaiblen sind bisher. Aber viele Konstanten ;-(


>Struktur ist sicher eine gute Idee

Ich hab alle diese Variablen in einer globalen Struktur..

[... (Gast)]

wenn sie global sind, warum übergibst du sie dann explizit? das kostet 
nur stack und damit zeit

[Karl H. (kbuchegg)]

Matthias Lipinsky wrote:
>>dass man an diesen Stellen Funktionen
>
> So hab ichs jetzt auch gemacht..
>
>
>>dass viele Parameter übergeben
>
> Naja, etwa 5 Varaiblen sind bisher. Aber viele Konstanten ;-(

Ohne jetzt dein Programm zu kennen, könnte es sein, dass deine 
Funktionen dann ganz einfach zuviel machen, wenn du sie mit einer 
Unmenge an Konstanten steuern musst.

>>Struktur ist sicher eine gute Idee
>
> Ich hab alle diese Variablen in einer globalen Struktur..

Auch das deutet auf ein Problem hin. Variablen werden dann zu einer 
Struktur zusammengefasst, wenn diese Variablen thematisch 
zusammenghören. Scope, also ob global oder funktionslokal, ist aber kein 
derartiger Themenkreis. Themenkreis weäre zb. Datum oder Name oder 
Adresse oder sowas in der Richtung.

[Matthias L. (Gast)]

>nmenge an Konstanten steuern musst.

Damit meine ich eher Texte im Flash:
uint8_t[]    TXT_MENU1   PROGMEM   =  "Hallo Welt";
...

Diese verwende ich in allen case-Fällen.


>wenn diese Variablen thematisch zusammenghören.

Das tun sie. Es sind jetzt zwei structs. Eine für die Schrittkette 
(Zustand, Zeit,..), und eine zweite für das, was in der/den cases 
gemacht werden soll: Menu bauen und auswerten.

Abends kann ich mal einen case posten.

[Karl H. (kbuchegg)]

Matthias Lipinsky wrote:

> Abends kann ich mal einen case posten.

Wird wohl das Vernünftigste sein. So ins Blaue hinein ist es immer 
schwer irgendwelche Anregungen zu geben. Dazu gibt es einfach zuviele 
Möglichkeiten.

[Matthias L. (Gast)]

>Wird wohl das Vernünftigste sein..

Im Anhang ist der Auszug der Funktion main_menu, nachdem ich das 
ausgelagert habe..

//-- Hauptmenu ----------------------------------------------------------
case 2:
  main_menu ();
  break;

...

[Karl H. (kbuchegg)]

Matthias Lipinsky wrote:
>>Wird wohl das Vernünftigste sein..
>
> Im Anhang ist der Auszug der Funktion main_menu, nachdem ich das
> ausgelagert habe..

Und? Was gefällt dir daran jetzt nicht? Da sind doch gar keine 
riesengroßen Parameterlisten.


Darf ich trotzdem ein paar Vorschläge machen? (... die jetzt mit deiner 
unmittelbaren Fargestellung so nichts zu tun haben)

Mir ist in deinem Code Schnipsel aufgefallen, dass hier immer wieder die 
Sequenz

    strcpy_P ( &au8TempStr[0], AU8TXT_MAIN1 );
    lcd4x20_show ( /* u8Line  */ 0b0001,
                   /* u8Col   */ 1,
                   /* pu8Data */ &au8TempStr[0],
                   /* u8Len   */ strlen_P(AU8TXT_MAIN1) );

vorkommt.

* Gewöhn dir das   & ...[0]   wieder ab. Das bringt nichts an Klarheit 
und ist nur unnötiger Tippaufwand. Der Name eines Arrays alleine 
fungiert aqls Zeiger auf das erste Array Element

* Die tatsächliche Länge eines Strings s in eine Funktion mitzugeben ist 
meistens Unfug. Vor allen Dingen, wenn du die Stringlänge sowieso 
mittels strlen bestimmst. So was sollte dir immer eine Warnung sein, 
dass dieser Parameter eigentlich unnötig ist. Die Funktion kann sich die 
Länge selbst bestimmen, wenn sie sie benötigt.

* Kopiere Strings niemals um, wenn es nicht unbedingt sein muss!
Das Problem dabei: Wieviel Speicher reservierst du denn für die Kopie? 
Wenn du es richtig machen willst, musst du den Speicher dynamisch 
allokieren, und das wollen wir auf einem µC eigentlich vermeiden wenn es 
geht.

* Nichts gegen Kommentare. Aber streu deine Kommentare nicht mitten in 
den Code rein. Das hindert den Lesefluss extrem.

Wo liegt in deinem Code-Schnipsel das Problem? Du warst zu faul, dir 
eine Funktion zu machen, die einen String aus dem Flash ausgeben kann. 
So wie du ein lcd4*20_show hast, hättest du dir noch ein lcd4*20_show_P 
machen sollen. Die Funktion kriegt: Zeile, Spalte, Zeiger auf String 
(und keine Längenangabe! Das solltest du auch bei deiner lcd4*20_show 
noch korrigieren)

void lcd4_20_show_P( uint8_t row, uint8_t col, const char* text )
{
  char c = (char)( pgm_read_byte( text++ ) );
  while( c != '\0' )
  {
    lcd4x20_put( col++, row, c );
    c = (char)( pgm_read_byte( text++ ) );
  }
}

Diese Funktion holt sich den String direkt aus dem Flash, wobei sie der 
üblichen C-Konvention folgt, dass der String bei einem '\0' Zeichen 
endet (was anderes macht strcpy_P auch nicht). Aber: Sie benötigt keinen 
Zwischenspeicher und kann daher auch keinen Zwischenspeicher überlaufen! 
Auch benötigt sie die tatsächliche String Länge nicht, ist daher 
schneller, als wenn vorher mittels strlen_P die Länge festgestellt 
werden muss.

Dein Aufruf

    strcpy_P ( &au8TempStr[0], AU8TXT_MAIN1 );
    lcd4x20_show ( /* u8Line  */ 0b0001,
                   /* u8Col   */ 1,
                   /* pu8Data */ &au8TempStr[0],
                   /* u8Len   */ strlen_P(AU8TXT_MAIN1) );

verkürzt sich daher zu

    lcd4x20_show_P( 0b0001, 1, AU8TXT_MAIN1 );

abgesehen davon, dass dadurch der Code kürzer wird, hast du auch noch 3 
zusätzliche Vorteile: Der Code ist sicherer, weil du kein Array 
überlaufen kannst. Er ist schneller, weil in deiner Originalversion der 
String im Flash 2-mal durchlaufen wird (1-mal für den strcpy_P und 
einmal für den strlen_P) während er in der sichereren Version nur 1-mal 
durchlaufen wird (bei der eigentlichen Ausgabe, die in deiner Version 
aus dem Zwischenspeicher gemacht wird). Und du brauchst die Stringlänge 
für den Aufruf nicht explizit bestimmen, hast also das Verwenden der 
Funktion für den Aufrufer einfacher gemacht.

Hier hast du also einen Fall, in dem du durch geschicktes Einführen von 
Funktionen nur Vorteile hast. Und solche Dinge solltest du mal suchen. 
Mit der Zeit kriegt man nämlich auch einen Blick dafür, welche 
'Transformation' in Funktionen sich lohnen und welche nicht.

Wenn du jetzt noch ein paar #define einführst um die magischen Zahlen 
0b0001 und 1 wegzukriegen

#define MENUTEXT1_ROW   1
#define PARAMETER_ROW  2

#define MENUTEXT1_COLUMN 1

....

    lcd4x20_show_P( MENUTEXT1_ROW, MENUTEXT_COLUMN, AU8TXT_MAIN1 );

dann liest sich der Funktionsaufruf schon fast wie englischer Prosatext: 
Gib den Text AU8TXT_MAIN1 auf dem Lcd in der Spalte MENUTEXT_COLUMN und 
in der Zeile MENUTEXT_ROW aus.
Und damit sind die Kommentare völlig überflüssig geworden! Und das ist 
gut, denn diese Kommentare sind von der Sorte: sie erzählen mir nichts, 
was nicht eh schon im Code auch steht.

Dadurch vereinfacht sich zb

    //-- Hauptmenu aufbauen -------------------------------------
    lcd4x20_show_P( MENUTEXT1_ROW, MENUTEXT_COLUMN, AU8TXT_MAIN1 );
    lcd4x20_show_P( MENUTEXT2_ROW, MENUTEXT_COLUMN, AU8TXT_MAIN2 );
    lcd4x20_show_P( MENUTEXT3_ROW, MENUTEXT_COLUMN, AU8TXT_MAIN3 );

Aus ursprünglich 17 Zeilen sind 3 geworden, ohne dass die Klarheit 
darunter gelitten hätte

[Matthias L. (Gast)]

>Was gefällt dir daran jetzt nicht?

So ist das schon ok, in der Funktion. Nur vorher, alles im case, war 
Mist.


>Gewöhn dir das   & ...[0]
OK.

>Die tatsächliche Länge eines Strings
Der Funktion lcd4x20_show keine Länge mitgeben? Hm.. Ja, könnte ich 
ändern. Aber so kann ich festlegen, wielang die Ausgabe auf dem Display 
sein soll (falls der Text länger ist..)

>Kopiere Strings niemals um..
Naja, da müsste ich aber die lcd_.._show Funktion komplett im Konzept 
ändern..

>Wieviel Speicher reservierst du denn für die Kopie?
Eine Zeile, also 20Zeichen+Null.

>Wo liegt in deinem Code-Schnipsel das Problem?
Keins. Es ging mir nur um den vielen Code innerhalb eines case.

>noch ein lcd4*20_show_P
Sehr gute Idee. Wieso ist mir das nicht eingefallen? Werde ich tun.

>um die magischen Zahlen
Hm.. Stimmt. Das kann ich bei der Funktion, die den Cursor positioniert, 
auch gleich nutzen ;-)


>Aus ursprünglich 17 Zeilen sind 3 geworden, ohne dass die Klarheit
>darunter gelitten hätte
Vielen Dank für die Infos. Ich werde das entsprechend anpassen.


>Und solche Dinge solltest du mal suchen.
>Mit der Zeit kriegt man nämlich auch einen Blick dafür, welche
>'Transformation' in Funktionen sich lohnen und welche nicht.

Die fehlt mir eben noch. Deshalb frage ich ja meist in GCC-Forum. Bin 
eben nur Hobby-C-Programmierer. Beruflich programmiere ich SPS, und da 
läuft das etwas anders..

[Karl H. (kbuchegg)]

Matthias Lipinsky wrote:
>>Was gefällt dir daran jetzt nicht?
>
> So ist das schon ok, in der Funktion. Nur vorher, alles im case, war
> Mist.


Man könnte sich jetzt noch Verallgemeinerungen überlgen.
Wenn ich Zeit hab, schreib ich heut am Abend noch was drüber.


>>Die tatsächliche Länge eines Strings
> Der Funktion lcd4x20_show keine Länge mitgeben?

Yep.

> Hm.. Ja, könnte ich
> ändern. Aber so kann ich festlegen, wielang die Ausgabe auf dem Display
> sein soll (falls der Text länger ist..)

Frage: Machst du das irgendwo?
Wenn nein: Du hast hier eine mögliche Flexibilität, die im Grunde 
niemand benutzt. Der Preis, den du dafür zahlst ist: Alle anderen 
Ausgaben, die dieses Feature nicht benutzen, zahlen dafür mit Laufzeit. 
Auch ein strlen ist nicht kostenlos!

Das errinnert mich an einen endlos komplizierten Mechanismus den mein 
Kollege und ich vor Jahren in unser Programm eingebaut haben. Wir haben 
in Summe 2 Mannmonate dafür verbraucht. In 15 Jahren hat niemand je die 
Flexibilität benötigt, die wir ermöglicht haben :-)

In deinem Fall ist es ähnlich. Der Regelfall wird sein, dass ein String 
komplett auszugeben ist. Und falls das mal nicht so sein sollte, ist es 
für den Aufrufer kein Problem, sich zunächst mal einen Teilstring in der 
gewünschten Länge zu bilden und den dann wieder der allgemeinen Funktion 
zu übergeben.

>
>>Kopiere Strings niemals um..
> Naja, da müsste ich aber die lcd_.._show Funktion komplett im Konzept
> ändern..

Yep. Sollte aber nicht so schwer sein. Normalerweise macht die ja nichts 
anderes als den String in einzelnen Zeichen aufs Display schaufeln.

>>Wieviel Speicher reservierst du denn für die Kopie?
> Eine Zeile, also 20Zeichen+Null.

Was wenn der auszugebende Text länger ist?
Der springende Punkt ist: Du hast dir hier mit diesem Zwischenarray 
Inflexibilität aus keinem guten Grund eingehandelt. Eine allgemeine 
String Ausgabe braucht die Längenangabe normalerweise gar nicht. Die 
wenigsten stringverarbeitenden Funktionen, die einen String als Eingabe 
haben, benötigen die Länge.