ANSI C Stil ok so? Ginge ich als C Dev durch?

Ich bemühe mich total, mir einen klassischen C Stil anzueigenen.

Daher mal ganz nebenbei und ungezwungen/unverbindlich eine kurze Umfrage,
ob es an diesem "Stil" was auszusetzen gibt, aus der Sicht von C Codern.

1. Was sind die Punkte, die mal garnicht gehen?

2. Was hab ich explizit gut gemacht (und C-typisch)?
(Weil, hab auch viel vom Java-Stil aus unserer Vorlesung einfliessen lassen,
wo ich es für besser hielt (subjectiv).)

3. Sonstige Anmerkungen?

#include <stdlib.h>

/*
 *
 *  function:
 *
 *      - my_find():
 *
 *        returns lowest index of string where substring is
 *        contained, else returns index, where index is length
 *        of string (:= end of string, index pointing to '\0').
 *
 *  return value:
 *
 *      - unsigned int.
 *
 *        Legal Range:
 *           from 0 to max(unsigned int).
 *           max(unsigned int) := 2**32-1 (32 bit) or
 *                                2**64-1 (64 bit).
 *
 *        Logical Range:
 *           from 0 to length(string).
 *           length(string) also counts last byte '\0'.
 *
 *       +----------------------------+---------------------------+
 *       | Return Value               | Meaning                   |
 *       +----------------------------+---------------------------+
 *       | 0 .. length(string) - 1    | Lowest index of substring |
 *       |                            | match within string.      |
 *       +----------------------------+---------------------------+
 *       | length(string)             | No match. Index points to |
 *       |                            | end of string '\0'.       |
 *       +----------------------------+---------------------------+
 *
 *  parameters:
 *
 *      1st: char* __substring. (required)
 *           The substring you are looking for within string.
 *
 *      2nd: char*    __string. (required)
 *           The string we are searching substring in it.
 *
 *  variables:
 *
 *      - char* substring;
 *        The substring you are looking for within string.
 *      - char*    string;
 *        The string we are searching substring in it.
 *
 *      - unsigned int    ssp;
 *        Substringpointer: the current index of substring.
 *        substring[ssp] is the current char to be compared.
 *      - unsigned int     sp;
 *        Stringpointer: the current index of string.
 *        string[sp] is the current char to be compared.
 *      - unsigned int      i;
 *        Index: the index of the matching position in string.
 *        string[i] is identical to substring[0] in case of a match.
 *      - unsigned char match;
 *        Selfmade Boolean type. If match == 0x00 := false.
 *                               If match == 0xFF :=  true.
 *        Indicates a match if true.
 *        
 */
unsigned int
my_find
(char* __substring, char* __string)
{
    char* substring;
    char*    string;

    unsigned int    ssp;
    unsigned int     sp;
    unsigned int      i;
    unsigned char match;

    substring = __substring;
    string    =    __string;

    ssp   =    0;
    sp    =    0;
    i     =    0;
    match = 0x00;

    while ( string[sp] != '\0' && substring[ssp] != '\0' ) {
        if ( substring[ssp] == string[sp] ) {
            if ( ssp == 0 )
                i =      sp;
            sp    =  sp + 1;
            ssp   = ssp + 1;
            match =    0xFF;
        }
        else {
            if ( ssp == 0 )
                sp = sp + 1;
            ssp    =      0;
            match  =   0x00;
        }
    }

    while ( string[sp] != '\0' )
        sp = sp + 1;

    if ( match == 0xFF )
        return i;
    else
        return sp;
}

int
main
(void)
{
    char* substring;
    char*    string;    

    unsigned int i;

    substring =          "abc123";
    string    = "jasllfaoaabc123jsfk";
//  RULER RULER  0123456789012345678
//  MATCH MATCH  =========^=========

    i = my_find(substring, string);

//  expected output:
//      index: 9
    if ( string[i] != '\0' )
        printf("index: %u\n",i);
    else
        printf("no match.\n");

    return 0;
}

Danke für Feedback.

simon.gysi

ja, das ist klassisch, richtig kryptisch...

__Stefan__

Schlecht ist, dass du nicht einfach strstr() benutzt...

Ausserdem kanst du die Variablen direkt initialiseren:

int x = 0;

Und Bezeichner die einen doppelten Underscore enthalten sind verboten.

Bashar

Zusätzlich:

Warum kopierst du die Funktionsparameter in lokale Variablen?

Das Layout gefällt mir nicht, solche "hübschen" Einrückungen würd ich nicht verwenden. Code-Layout sollte sich IMHO auf das beschränken, was ein guter Editor (z.B. Emacs, nicht Notepad) von Haus aus mitbringt. Alles andere sind unnötige Schnörkel.

Der Kommentar über der Funktion sollte nicht die Innereien beschreiben. Verwende lieber aussagekräftige Variablennamen als beschreibende Kommentare.

Arbeite mit Pointern statt Indizes.

Verwende Increment- und Decrement-Operatoren statt x=x+1

Die Parameterliste einer Funktion würde ich nicht auf die nächste Zeile schreiben, sowas hab ich auch noch nie gesehen.

Zum Algorithmus kann ich nicht soviel sagen, nur soviel:
match auf 0xFF für "true" zu setzen halte ich für kryptisch. Für boolesche Werte verwendet man klassisch int, true ist 1 und 0 ist false. Ab C99 gibt es bool, true und false.

Bashar schrieb:

Arbeite mit Pointern statt Indizes.

wieso sollte man statt string[sp] besser *(string+sp) schreiben?

supertux

Versuch in den Beeichnernamen auf __ zu verzichten (zumindest als Anfang des Bezeichnernamens).

Statt char *__string nimm lieber char *string

Im Code selber, verstehe ich das nicht:

unsigned int
my_find
(char* __substring, char* __string)
{
    char* substring;
    char*    string;

    ...

    substring = __substring;
    string    =    __string;

wozu diese 2 Zuweisungen? Welchen Sinn/Vorteil hat es?

Was den Kommentaren anbetrifft: ich würde eher doxygen Synatx benutzen, damit man auch doxygen verwenden kann. Und jede einzelene lokale Variable brauchst du gar nicht zu dokumentieren, das ist viel zu viel (meiner Meinung nach).

Bashar

fricky schrieb:

Bashar schrieb:

Arbeite mit Pointern statt Indizes.

wieso sollte man statt string[sp] besser *(string+sp) schreiben?

Zu Funktionsparameter in lokale Variablen kopieren:

- Damit die ursprünglichen Parameter über die gesamte Laufzeit der Funktion
  unverändert erhalten bleiben. (Kann sinnvoll sein, wenn man debuging Mechanismen
  einbauen möchte. Gut, war hier nicht der Fall, aber ich sehe es nicht als
  grundsätzlich falsch - im Gegenteil.)
- In der Java Vorlesung haben wir Parameter immer mit "p" als Prefix definiert:
  public static int foobar (int pBreite) {
      int Breite;
      Breite = pBreite;
      // Arbeite weiter mit Breite.
      return Breite;
  }
  Die "__" habe ich dagegen aus Python geklaut und anstelle des Prefix "p" genommen.

Zu den Kommentar der Funktion und aussagekräftigen Variablen:

- Ich finde es aber sehr nützlich, so einen Kommentar zu haben,
  weil Variablenamen, egal wie gut benannt, sagen nix über den
  Context der Verwendung aus oder Begrenzungen oder Ausnahmebehandlung usw.
  Also falsch fand ich das jetzt nicht... wenn bloss jeder so gut dokumentieren würde.

Zu den Pointern:

- Ich habs mir kurz angeschaut, aber schnell eingesehen, dass das nicht die
  Menschen-verständliche Methodik ist, Software zu schreiben. Dann lieber Brainfuck.
- Damit spart man sich nur ein paar Zeilen Code C, aber portabler wird die
  Software dadurch nicht. Mit Portabel mein ich nicht "ist doch C und C ist schon
  portabel...", sondern "schreib mal das C Programm um in python, java, perl, ada95,
  lisp, ...". Wichtiger ist, zu sehen, __was__ genau passiert, statt wie man es durch
  Pointerarithmetik im Source Code kleiner macht (die LOC).

Zu der separaten Deklaration und Initialisierung der Variablen:

- Sieht einfach sauberer aus IMHO. Trennung "Was hab ich?", "Was ist drin?".

Zum selbstgemachten Bool:

- Meine persönliche Marotte. Aber ich finde false := b'00000000', true := b'11111111'
  sinnvoller als 32/64bit ints, wo false := b'00000000...00000000' und
  true := b'00000000...00000001' (32/64 bit lang). Keine Ahnung, ich lasse
  mir halt das Hintertürchen offen binäre Operationen damit anzuwenden.

Danke für die ernst gemeinten Anmerkungen.

LordJaxom

- Damit die ursprünglichen Parameter über die gesamte Laufzeit der Funktion
  unverändert erhalten bleiben. (Kann sinnvoll sein, wenn man debuging Mechanismen
  einbauen möchte. Gut, war hier nicht der Fall, aber ich sehe es nicht als
  grundsätzlich falsch - im Gegenteil.)

Wenn Du möchtest, dass die Parameter über die gesamte Funktionslaufzeit gleich bleiben, verwende const. Ich sehe das als grundsätzlich unsinnig.

- In der Java Vorlesung haben wir Parameter immer mit "p" als Prefix definiert:
  public static int foobar (int pBreite) {
      int Breite;
      Breite = pBreite;
      // Arbeite weiter mit Breite.
      return Breite;
  }
  Die "__" habe ich dagegen aus Python geklaut und anstelle des Prefix "p" genommen.

Das ist ja grausam. Nicht einmal das Prefixing an sich (wobei da hier im Forum auch schon genug drüber lamentiert wurde), aber die nicht vorhandene Initialisierung und die anschließende Zuweisung. Gerade in Java, in C seit C99 übrigens auch, kann man Variablen definieren, wenn man sie braucht.

- Ich finde es aber sehr nützlich, so einen Kommentar zu haben,
  weil Variablenamen, egal wie gut benannt, sagen nix über den
  Context der Verwendung aus oder Begrenzungen oder Ausnahmebehandlung usw.
  Also falsch fand ich das jetzt nicht... wenn bloss jeder so gut dokumentieren würde.

An sich richtig, ich halte es aber gern mit dem Grundsatz: Kommentare sollen den Quelltext ergänzen (warum mache ich etwas), nicht erklären (was wird da gemacht). Mal rein interessehalber: Wie lange hast Du für die Funktion gebraucht, wie lange für den Kommentar? (Verhältnis reicht)

- Sieht einfach sauberer aus IMHO. Trennung "Was hab ich?", "Was ist drin?".

Wenn Definition und erste Zuweisung weit auseinander liegen, vergisst man letztere auch schonmal und halst sich (unnötige!) Probleme mit uninitialisierten Variablen auf.

- Meine persönliche Marotte. Aber ich finde false := b'00000000', true := b'11111111'
  sinnvoller als 32/64bit ints, wo false := b'00000000...00000000' und
  true := b'00000000...00000001' (32/64 bit lang). Keine Ahnung, ich lasse
  mir halt das Hintertürchen offen binäre Operationen damit anzuwenden.

Damit stellst Du Dich aber ins Abseits allen gegenüber, die Deinen Code verwenden oder von Dir gelieferte "bools" auswerten möchten. Zumindest wenn einmal echte bools in Deine Werte überführt werden müssen.

DerGrosseC schrieb:

Zum selbstgemachten Bool:

- Meine persönliche Marotte. Aber ich finde false := b'00000000', true := b'11111111'
  sinnvoller als 32/64bit ints, wo false := b'00000000...00000000' und
  true := b'00000000...00000001' (32/64 bit lang). Keine Ahnung, ich lasse
  mir halt das Hintertürchen offen binäre Operationen damit anzuwenden.

aber wenn du's so machst, dann funzt es z.b. nicht mit dem 'not'(!) operator.
also !FALSE == TRUE und !TRUE == FALSE geht nicht (das erste davon), wenn du FALSE als 0xff definierst.
in C ist zwar jeder wert !=0 TRUE, aber vergleiche z.b. ergeben 1 oder 0 als ergebnis. daher ist es besser, du nimmst die 1 als TRUE.
wenn schon eigene bools, dann mach es doch so:

typedef enum bool
{
 false,
 true
} bool_t;

^^sieht dann auch beim debuggen gut aus, es wird 'false' bzw. 'true' angezeigt und nicht 0 oder 1.
dann geht z.b. auch sowas:

bool_t smaller = (a<b);
if (smaller == TRUE)
{
  // a ist kleiner als b
}

Bashar

Hattest du gehofft, deine persönlichen Marotten würden rein zufällig mit dem "klassischen" Stil übereinstimmen? Ich bin irgendwie verwundert über die Motivation dieses Threads.

DerKuchen

Bezeichner mit zwei Unterstrichen am Anfang sind explizit für den Compiler reserviert. Auch wenn es normalerweise geht sollte man die nicht benutzen.

Die Parameter nochmal in lokale Variablen zu kopieren ist meiner Meinung nach völlig unnütz. Wenn sie nicht verändert werden sollen, nimm const, ansonsten ist es egal. Selbst dein Debugging Argument greift nicht, da du nur den Pointer kopierst, die dann aber beide auf den gleichen Speicherbereich zeigen. Wenn du das wirklich haben willst, musst du den String schon kopieren.
Abgesehen davon macht es den Code nicht gerade leserlicher, wenn die Parameter in der Funktion nicht mehr verwendet werden. Beim schnellen drüberlesen verwirrt das eher.

Deine Einrückungen find ich eher übertrieben. Da entstehen größere "Löcher" im Text, was den Lesefluss stört. Vor allem folgende Stelle fand ich sehr irritierend:

if ( ssp == 0 )
    i =      sp;
sp    =  sp + 1;
ssp   = ssp + 1;
match =    0xFF;

Durch das gleich ausgerichtete = wirkt das, als ob alle 4 Anweisungen zusammengehören. Das if geht da eher unter.

Meiner Meinung nach sollte die Signatur einer Funktion (also typ name(parameter)) in eine Zeile geschrieben werden, da lässt sich aber drüber streiten. Immerhin gehören diese Informationen direkt zusammen, deshalb sollte man sie nicht extra trennen.

Zu deinem bool wurde ja schon einiges gesagt, aber wenn es dir um "Ansi C Stil" geht, dann nimm 0 und 1. Alle Standard C Funktionen benutzen 0 und 1, daher sollte man sich da nicht unbedingt was eigenes ausdenken.

Variablen sollten eigentlich immer direkt bei der Deklaration initialisiert werden. Das ist allgemein "guter Stil", und dient unter Anderem dazu, dass keine uninitialisierten Variablen rumfliegen. Außerdem solltest du statt

x = x + 1

lieber

++x;

benutzen. Das ist für C Programmierer wesentlich einfacher zu lesen.

Den Kommentarblock finde ich völlig übertrieben, der ist länger als die eigentliche Funktion. Guter Code sollte selbstdokumentierend sein. Statt spp hättest du z.B. einfach substrPos nehmen können, und hättest einen Kommentar gespart.
Außerdem solltest du die Kommentare zu den betreffenden Code Zeilen schreiben, nichts ist nerviger als ständig scrollen zu müssen, um etwas nachzusehen. Das hilft auch dabei die Kommentare aktuell zu halten.

Eine Anmerkung hätte ich noch, auch wenn es weniger stilistischer Natur ist: Statt der Länge des Strings als Rückgabewert für einen Fehler würde ich lieber -1 nehmen, das lässt sich wesentlich leichter von einer guten Rückgabe unterscheiden. Außerdem ist der Test auf Fehler dann einfacher.

_matze

DerGrosseC schrieb:

- In der Java Vorlesung haben wir Parameter immer mit "p" als Prefix definiert:
public static int foobar (int pBreite) {
int Breite;
Breite = pBreite;
// Arbeite weiter mit Breite.
return Breite;
}

Der Präfix "p" steht in C/C++ eigentlich für einen Pointer. Kennzeichnet man damit in Java wirklich jedes Funktionsargument?

Wenn du damit jeden Parameter kennzeichnest, führt das jeden C/C++-Programmierer in die Irre. Das wird spätestens wichtig, wenn du beruflich programmierst und dich mit Kollegen arrangieren musst.

Wenn dir die Kennzeichnung von Parametern aber so wichtig ist, könntest du dir ja was ausdenken und z.B. "p_" o.ä. verwenden. Ein als Parameter der Funktion übergebener unsigned int Pointer könnte dann "p_puiVarName" heißen. Man kann es mit der Ungarischen Notation aber auch übertreiben. Das merkt man spätestens, wenn die Präfixe die eigentlichen Variablennamen in der Länge übertreffen.

Gerade im Hinblick darauf, dass Funktionen grundsätzlich nicht zu groß werden sollten, ist eine Kennzeichnung als Parameter imho für die Übersicht unnötig.

supertux

DerGrosseC schrieb:

Z
Zu den Kommentar der Funktion und aussagekräftigen Variablen:

- Ich finde es aber sehr nützlich, so einen Kommentar zu haben,
  weil Variablenamen, egal wie gut benannt, sagen nix über den
  Context der Verwendung aus oder Begrenzungen oder Ausnahmebehandlung usw.
  Also falsch fand ich das jetzt nicht... wenn bloss jeder so gut dokumentieren würde.

Kommentare zu den Knackpunkten bzw. verwendete Algorithemen sind wichtig, ein Kommentar für jede Zeile hinderlich und auf Dauer nicht zu halten.

Außerdem ist es wichtig zu trennen, was man dokumentiert, sprich, was die Funktion macht und wie die Funktion etwas macht.

Wenn du mein Tipp mit doxygen wahrnimmst, dann musst du nur dokumentieren, was die Funktion macht. Dem Benutzer der Funktion interessiert in erster Linie nicht im Detail, wie genau die Funktion arbeitet. Wenn zum Verständnis ein bisschen über die Arbeitsweise der Funktion geschrieben wird, ist ok, aber nicht den gesamten Quellcode erklären (das ist eher kontraproduktiv; veränderst du den Algorithmus, musst du die Kommentare neu schreiben, also unnötige unproduktive Arbeit).

Ich bin nicht der einzige der das folgende dir sagt: verwende lieber aussagerkräftigere Variablennamen. Das hilft mehr als Kommentare.

DerKuchen schrieb:

Außerdem solltest du statt
x = x + 1
lieber
++x;
benutzen. Das ist für C Programmierer wesentlich einfacher zu lesen.

ist zwar egal, wenns so allein da steht, aber ich empfinde x++ noch ein kleines bisschen leserlicher.

DerKuchen

Stimmt auch wieder...
Das Preincrement-++ hab ich mir durch C++ angewöhnt, da es hier durchaus einen Unterschied macht.

Badestrand

DerKuchen schrieb:

Stimmt auch wieder...
Das Preincrement-++ hab ich mir durch C++ angewöhnt, da es hier durchaus einen Unterschied macht.

Ne, wenn eine native Zahl-Variable alleinstehend post-inkrementiert wird, wirst du höchstwahrscheinlich nicht mal nen Unterschied im Binary feststellen

PS: Das "nativ" soll "int", "short" oder Konsorten bedeuten

LordJaxom

_matze schrieb:

Der Präfix "p" steht in C/C++ eigentlich für einen Pointer. Kennzeichnet man damit in Java wirklich jedes Funktionsargument?

Ich bezweifle dass man das tut. Ich jedenfalls mache es nicht. Ich habe zwar schon Styleguides gesehen, die ein Prefix (allerdings häufiger "a" oder "an" als "p") vorsehen, aber glücklicherweise muss ich mich nicht an solch einen halten.

LordJaxom schrieb:

_matze schrieb:

Der Präfix "p" steht in C/C++ eigentlich für einen Pointer. Kennzeichnet man damit in Java wirklich jedes Funktionsargument?

Ich bezweifle dass man das tut. Ich jedenfalls mache es nicht. Ich habe zwar schon Styleguides gesehen, die ein Prefix (allerdings häufiger "a" oder "an" als "p") vorsehen, aber glücklicherweise muss ich mich nicht an solch einen halten.

ich halte in java gar nix von prefixes. in java kann man keine überraschungen erleben, wenn man mit einer variable hantiert. da es keine pointerarithmetik gibt, kann im grunde auch gar nichts schiefgehen.

manche empfehlen allerdings prefixes für member, um zuweisungsfehler auszuschließen. aber ich verlass mich da lieber auf compilerwarnings

_matze

Ich halte sprechende Variablennamen für sehr sinnvoll. Das schließt für mich auch entsprechende Präfixe ein. Member mit "m_" und Globale mit "g_" zu kennzeichnen, ist bei uns Pflicht und entspricht auch meiner persönlichen Meinung. "l_" für Lokale halte ich dagegen für übertrieben und unnötig, da eine nicht so gekennzeichnete Variable nach dem Ausschlussprinzip nichts anderes als eine lokale Variable sein kann. Funktionsparameter auch noch irgendwie zu kennzeichnen ist, wie gesagt, bei Funktionen, die man komplett im Blickfeld hat, ebenfalls unnötig. Aus demselben Grund sind Typen-Präfixe ("i" für int) gut, aber nicht für jede kleine Zählvariable nötig.