if abfrage abändern

Hallo miteinander,
ich habe da ein Problem, bei einen meiner Befehle müsste ich einen Fehlercode mit ausgeben wenn der nicht richtig an das Arduino übertragen wurde, das habe ich jetzt auch hin bekommen, doch mich lässt der Gedanke nicht los das ich das nicht Optimal erstellt habe, also die IF abfrage im unteren Bereich.

das ist der Code des Befehls:

void  Befehl_repetition( char* text)
  {
   text = text + strlen_P(PSTR("repetition")) + 1;
   strtok(text, ",");

   repetition = atoi(text);
   W_break    = atoi(strtok(NULL, ","));   

  if (repetition > 1)
    {
      if (W_break > 0)
        { Serial.print(F("OK\n")); }
      else
        { Serial.print(F("NOK, incorrect syntax (pause)\n")); }
    }
  else
   { Serial.print(F("NOK, incorrect syntax (Widerholung)\n")); }
  }

kann mir jemand sagen ob ich das so richtig mache oder ob man das etwas Optimierter schreiben könnte sollte.

Gruß
Mücke

Hallo, ich wüßte nicht was man hier anders machen soll. Ist viel zu kurz um was zu ändern. Macht es denn das was Du möchtest? Das wäre wichtig. :) Wenn repetition kleiner gleich 1 ist, passiert gar nix. Ist repetition größer 1, wird geprüft ob W_break größer 0 ist usw.

Eine Optimierung gibt es, Widerholung mit "ie" bitte. ;)

Du übersprinst den Anfang des Strings, der vermutlich den Text "repetition " enthalten sollte. Das kannst du gleich mit überprüfen (strncmp)

Wenn was nicht stimmt, gibst du einen Text aus. Du könntest der Funktion einen Rückgabewert verpassen, und so da wo sie verwendet wird, leichter im Fehlerfall entscheiden was anderes (nichts) zu tun. Kann ich aber so nicht beurteilen, ob das sinnvoll ist.

Ok das "ie" kann ich einfügen ;-) danke.

sie macht was sie soll.

Wenn repetition - kleiner 1 dann Ausgabe "Fehler" (hmm was ist mit gleich 1, HANDLUNGSBEDARF) - Größer 1 dann Prüfe Wenn W_break -- Größer 0 dann Ausgabe "OK" -- kleiner 0 dann Ausgabe "Fehler" (hmm was ist mit gleich 1, HANDLUNGSBEDARF)

sie macht was sie soll, ich dachte eigentlich das man das IF verschachteln besser gestellten könnte.

die Überprüfung ob der Befehl "repetition" überhaupt existiert erfolgt schon weiter vorne wenn der Befehl nicht existiert wird auch eine Fehlermeldung ausgegeben.

wenn jedoch die IF verschachteln so OK ist dann bin ich zufrieden ;-)

EDIT: bei der Wiederholung bekomme ich auch bei 1 eine Fehlermeldung, das ist richtig so. bei der pause bekomme ich bei 1 keine Fehlermeldung das ist richtig so bei 0 und darunter bekomme ich eine Fehlermeldung

also klappt doch musste nichts nacharbeiten :-)

die Überprüfung ob der Befehl "repetition" überhaupt existiert erfolgt schon weiter vorne wenn der Befehl nicht existiert wird auch eine Fehlermeldung ausgegeben.

Könntest an Befehl_repetition gleich den schon weitergeschalteten Zeiger übergeben.

if (strncmp_P(text, PSTR("repetition "),11) == 0)  Befehl_repetition(text+11);

Aber mehr Geschmackssache, nacharbeiten müssen musst du nichts.

an die Idee hatte ich noch gar nicht gedacht, :-) habe das mal getestet das Sparta mir eine ziele in der Befehls Auswertung

hat es einen voreilt wenn man das so macht? - Speicher einsparen - Ressourcen Sparen

keine Ahnung was man da alles sparen kann oder auch nicht.

Kannst du ausprobieren und dir jeweils die Flash-Größe ansehen, merken und vergleichen -- Viel sparst du nicht, es wird aber hauptsächlich übersichtlicher:

Die Stellen, wo du 11 Buchstaben "repetition " testest und dann auch 11 Zeichen weitergehst, liegen zusammen. (Könntest es einfacher auf "WDH:" und 4 ändern )

In der Unterfunktion, wo du die Parameter auswertest, hast du nur die Parameter im String. (Könntest die gleiche Funktion für "repetition " und für "WDH:" verwenden)

Mehr ein Thema wie "Code übersichtlich und verstehbar halten" als "Wie spare ich 5 Befehle im Flash", obwohl das in diesem Fall zum Glück kein Gegensatz ist.

also das mit dem zusammen legen der "+11" und der abfrage des Befehls leuchtet mir ein.

was mir nicht so ganz klar ist was du mit "WDH:" meinst, zuerst dachte ich du meinst das ich den Befehl ändern sollte das er kürzer ist (möchte ich nicht da die Software die damit arbeitet den Befehl schon kennt, und ich die nicht umschreiben kann).

Doch dann hast du gemeint das der Befehl "WDH" und "repetition" zusammen drauf zugreifen können auf die Funktion das hat mich etwas verwirrt und ich wies nicht was du meinst :-(

Lass dich nicht verwirren! Wenn du was nicht verstehst, taugt es nichts.

Das mit dem "WDH" statt "repetition" hast du schon ziemlich richtig verstanden, du sollst den Befehl nicht ändern, aber du könntest, wenn du wolltest. Und du könntest sogar zwei Befehle haben, die das gleiche machen, wenn du wolltest.

Das wären zwei Sachen, die bei anderen Projekten schonmal vorkommen. Wenns bei dir keinen Sinn macht, vergiss es.

Ging mir eher ums Prinzip, Dinge die zusammengehören, nicht weit verstreut an verschiedenen Stellen im code zu haben ( hier der String "repetition " und seine Länge ) und Funktionen genau das mitgeben, mit dem sie was machen sollen.

Dass man das einfach durch eine Konstante ersetzen kann habe ich auch schon mal vorgeschlagen. Aber ich habe dass dann gelassen, eben gerade er die Zusammenhänge nicht ganz versteht (inzwischen geht es etwas besser) und Probleme damit hat eine Lösung von einer Stelle an andere zu übertragen. Da gab es mal eine Version wo beide Varianten verwendet wurden.

Von daher ist das so etwas aufwendiger, aber vielleicht einfacher zu verstehen. Dass man jetzt eine zusätzliche Funktion hat die Flash braucht oder es etwas mehr Rechenzeit kostet ist egal. Das ist hier völlig unkritisch.

michael_x: Lass dich nicht verwirren!

ich, ne, würde mir nie in den Sinn kommen, äh wo war noch mal oben ;-)

michael_x: Wenn du was nicht verstehst, taugt es nichts.

das würde ich so nicht unterschreiben, ...

Serenifly: Da gab es mal eine Version wo beide Varianten verwendet wurden.

du meinst die in der ich die Wiederholung und pause separat übergeben habe, ja die gab es, jedoch hat mein C# Programmierer sich geweigert das so in die WIN Software zu schreiben, warum habe ich nicht verstanden habe es dann beides in eine Übergabe gepackt dann war er etwas glücklicher und hat es eingebaut :-) ich glaube der wollte das mit dem Befehl run übergeben, jedoch löse ich den Befehl run über einen Taster der am Arduino angeschlossen habe aus.

Danke für die Unterstützung.