1. #1

    Registriert seit
    19.11.2011
    Beiträge
    114
    Thanked 42 Times in 31 Posts

    Standard Code-Review: Zahlen Sortieren in C

    Mahlzeit,
    ein Freund von mir studiert Elektrotechnik und hat mir ein Code von sich gegeben und mich geben mal rum zu fragen, ob jemand sein Code korrektur lesen kann und ggf. möglichkeiten zur Verkürzung oder Verbesserung beitragen kann.
    Falls eine Gegenleistung in Form von Geld o.ä. gewünscht ist,wäre er auch bereit dafür zu zahlen.


    #include <omp.h>
    #include <stdio.h>
    #include <time.h>
    #include <stdlib.h>

    int main()
    {
    unsigned short h=0, m=0; //Wertebereich-> 65535
    unsigned int *x, *y, i, q, eingabe; //Wertebereich-> 4294967295
    const int xx= 0,yy=1000; //Wertebereich der Zufallszahlen-> Programm
    srand (time(NULL)); //für Positive Zahlen sonst ändern der Datentypen

    q=1000000; //Größe des Reservierten speichers für das Array für eine Messung

    printf("\nWie viele Zahlen sollen Max. sortiert werden?->x*y\n x->Schrittgröße y-> anzahl der Schritte \nGeben Sie x ein:");
    scanf("%hu",&h);
    printf("Geben Sie y ein:");
    scanf("%hu",&m);
    printf("Wie viele Messungen sollen durchgeführt werden? (Messung1+..+Messungz)/z\n");
    printf("Geben Sie z ein");
    scanf("%d",&eingabe);


    x = (unsigned int*) malloc((q*eingabe) * sizeof(unsigned int));//Speicher Reservieren
    y = (unsigned int*) malloc((q*eingabe) * sizeof(unsigned int));//q*(Anzahl Messungen mit
    if(x==NULL||y==NULL) //unterschiedlichen Zahlen)
    printf("Speicher wurde nicht reserviert"); //

    for(i = 0; i < q; ++i)
    {
    x[i]=(rand()% ((yy+1)-xx))+xx;//Array mit Zufallszahlen füllen
    y[i]=x[i]; //<-Array zum wiederherstellen der Gleichen Zufallszahlen
    }
    // for(i = 0; i < h*m*eingabe; ++i) //Möglicheit zur Ausgabe/Kontrolle der Sortierung
    // printf("Erstellte unsortierte Liste%d\n",x[i]);//
    printf("\nVerfügbare Threads=%d\n",omp_get_num_procs());



    double start, ende, zeit=0, zeitsk;
    unsigned short tauschen, j, k, d, l;
    unsigned int ablage, w, n=0;


    FILE *pFile; // Zur Ausgabe der Ergebnisse in eine Textdatei
    pFile=fopen("liste.txt","w"); // Erzeugt eine Text datei liste.txt
    fprintf(pFile,"Elemente \t=1Thread->\tSkalierung \t\t=2Threads->\tSkalierung \t\t=3Threads->\tSkalierung \t\t=4Threads->\tSkalierung \n");


    for(l=0;l<m;l++) //Schleife um die Sortierung mit Unterschiedlicher länge zu wiederholen
    {

    n=n+h;
    printf("\nZeit um %d Zahlen zu sortieren mit\n",n);

    for(k=1;k<=omp_get_num_procs();k++) //Schleife die die Sortierung mit allen Thread Kombinationen durchführt
    {


    for(d=0;d<eingabe;d++) // Schleife um mehrer Messungen mit Andern Zufallszahlen durchzuführen
    {
    w=d*h*m; //d*maxArraylänge ->um andere Zufallszahlen zu bekommen springt er zum nächsten abschnitt im Array

    tauschen=1;
    start=omp_get_wtime(); //Start Zeitmessung
    while(tauschen)
    {
    tauschen=0;

    for(j=0;j<2;j++)
    {
    #pragma omp parallel for reduction(|:tauschen) private(ablage) num_threads(k)

    for(i=(j+w); i<(n+w-1); i=i+2)
    {
    if(x[i] > x[i+1] )
    {
    ablage = x[i];
    x[i] = x[i+1];
    x[i+1] = ablage;
    tauschen=1;
    }
    }
    }
    }

    ende=omp_get_wtime(); //Ende Zeitmessung
    zeit+=(ende-start);
    if(k==1) // Speichern der ersten Thread Werte-> Durchschnitt berechnen
    zeitsk=zeit; // Mit dem durchschnitt wird die Skalierung bei mehreren Threads berechnet

    printf("%d Threads %d.Messung =%fs Durchschnitt =%fs Skalierung =%f\n",k,d+1,(ende-start), zeit/(d+1),(zeitsk/eingabe)/(ende-start));

    //printf("\n\n"); //Möglicheit zur Ausgabe/Kontrolle der Sortierung
    //for(i = 0; i < (h*m*eingabe); ++i) //
    // printf("Thread=%d Sortiert %d\n",k,x[i]); //
    //printf("\n\n");
    for(i = 0; i < (q*eingabe); ++i) //Array mit der alten unsortierten Liste füllen
    x[i]=y[i]; //
    //for(i = 0; i < h*m*eingabe; ++i) //Möglicheit zur Ausgabe/Kontrolle der Sortierung
    // printf("Thread=%d Unsortiert %d\n",k,x[i]); //
    }

    if(1==k) //Funktion zur Ausgabe der Ergebnisse in eine Textdatei
    { //
    fprintf(pFile,"%d ",n); //
    if(n<1000) //
    fprintf(pFile," "); //
    } //
    //
    fprintf(pFile,"\t\t%f\t%f ",zeit/(eingabe),(zeitsk/eingabe)/(zeit/eingabe));
    //
    if(k==omp_get_num_procs()) //
    fprintf(pFile,"\n"); //


    zeit=0;
    }
    zeitsk=0;
    }

    fclose(pFile);
    return(0);
    }
    //gcc -o sort sort.c -Wall -fopenmp
    //export OMP_NUM_THREADS=1
    //./sort

  2. The Following 2 Users Say Thank You to Daxter For This Useful Post:

    NitroOxX (25.08.2019), Tiger18 (22.05.2020)

  3. #2
    Avatar von DMW007
    Registriert seit
    15.11.2011
    Beiträge
    6.080
    Thanked 9.118 Times in 2.995 Posts
    Blog Entries
    5

    Standard AW: Code-Review: Zahlen Sortieren in C

    Von der Formatierung und Struktur her ist das ganze in meinen Augen ein ziemliches Chaos: Kommentare die lediglich beschreiben was der Code macht, sind fast immer auf schlechten Code zurückzuführen. Sauberer Code erklärt sich selbst, womit sich die Kommentare in den meisten Fällen erübrigen. Und gute Kommentare sollten erklären, WARUM der Code das tut was er tut, warum er X nicht tut obwohl man das vielleicht erwarten würde, auf mögliche Probleme/Fallstricke eingehen usw. Diese Infos bieten einen tatsächlichen Mehrwert. Was der Code macht, sehe ich ihm an.

    pFile=fopen("liste.txt","w") // Erzeugt eine Text datei liste.txt

    Hier ist ein solches Beispiel: Dass die Zeile eine Datei zum schreiben öffnet ist selbst mir klar, obwohl ich mich im C Low-Level Bereich überhaupt nicht auskenne. Wogegen ich als erfahrener Entwickler 3x hinschauen musste, sind Stellen wie diese:

    unsigned short h=0, m=0; //Wertebereich-> 65535
    unsigned int *x, *y, i, q, eingabe; //Wertebereich-> 4294967295
    const int xx= 0,yy=1000; //Wertebereich der Zufallszahlen-> Programm

    Variablen Lustig nach X, Y, Z und am besten dann noch wie in Excel mit XX, YY usw. zu benennen ist ein Paradebeispiel für schlechten Codestil. Die gesamten Kommentare wären unnötig, wenn man sprechende Variablen verwendet: Zum Beispiel randomRangeStart/randomRangeEnd statt xx/yy. Dadurch wird der Code VIEL leichter zu lesen. Wenn ich weiter unten irgendwo y oder XX lese, muss ich ständig nachschauen, um was es da eigentlich geht. Mit sprechenden Namen wäre das nicht nötig. Größere Programme werden mit so was schnell unwartbar. Leider gibt es selbst Berufsschullehrer und Uni-Professoren, die ihren Studenten solch einen Blödsinn vormachen. Bitte nicht machen, wer solch einen Codestil pflegt, hat noch nie richtige Anwendungen entwickelt, die über ein paar Zeilen hinaus gehen...

    printf("\nWie viele Zahlen sollen Max. sortiert werden?->x*y\n x->Schrittgröße y-> anzahl der Schritte \nGeben Sie x ein:");
    scanf("%hu",&h);
    printf("Geben Sie y ein:");
    scanf("%hu",&m);
    printf("Wie viele Messungen sollen durchgeführt werden? (Messung1+..+Messungz)/z\n");
    printf("Geben Sie z ein");
    scanf("%d",&eingabe);

    Das ist (bis auf die schon angesprochene Variablenbezeichnung) aus technischer Sicht nicht zu beanstanden, aber von der Usability her für den Nutzer unnötig umständlich. Der Entwickler hat den Fehler gemacht, die Logik seiner Variablenbezeichnungen auf den Nutzer übertragen. Der Anwender muss sich dadurch mit x und y herumschlagen. Dafür gibt es keinen Grund - warum nicht gleich zur Eingabe von z.B. der Anzahl Schritte auffordern? Wie die Variable gespeichert wird, ist dem Anwender schließlich vollkommen egal, das muss er auch gar nicht wissen. Ist höchstens zum Entwickeln interessant, aber zum Thema sprechende Variablen habe ich ja oben schon was gesagt.


    if(x==NULL||y==NULL)
    printf("Speicher wurde nicht reserviert");

    Das ist zwar Syntaktisch korrekt, aber fehleranfällig: Ohne geschweifte Klammern steht nur die unmittelbar darunter folgende Zeile in der If-Abfrage. Gerade in ohnehin schon unübersichtlichem Code packt man schnell eine zweite Zeile darunter, im Glauben, sie würde ebenfalls in der If stehen. Das hat bei Apple beispielsweise erst vor wenigen Jahren eine Sicherheitslücke verursacht: https://embeddedgurus.com/barr-code/...y-preventable/ Ich halte es daher für Best Practice, bei solchen Abfragen immer geschweifte Klammern einzusetzen, auch wenn das syntaktisch bei einer einzelnen Zeile nicht nötig wäre.


    x = (unsigned int*) malloc((q*eingabe) * sizeof(unsigned int));//Speicher Reservieren
    y = (unsigned int*) malloc((q*eingabe) * sizeof(unsigned int));//q*(Anzahl Messungen mit

    An solchen Stellen würde ich mit Zwischenvariablen arbeiten, schon alleine um die Lesbarkeit zu erhöhen. Eventuell bringt es zusätzlich noch einen theoretischen Performance-Schub. Wobei heutige Compiler das bereits optimieren dürften. Aber Lesbarkeit sollte an erster Stelle stehen - Hardware ist billig, gute Programmierer sind es nicht.

    Ansonsten ist, wie Anfangs schon erwähnt, die Formatierung generell chaotisch und uneinheitlich: Mal die Variablen auf Deutsch, mal Englisch, mal mit Typ-Suffix (pFile). Man sollte sich auf eine Variante einigen. Vor allem in Teams ist das unerlässlich. Da ist Chaos schon vorprogrammiert, wenn einzelne Entwickler verschiedene Konventionen nutzen. Wenn sich dann der einzelne Entwickler noch nicht mal einig ist, kann man nur noch für das Projekt beten Deutsche Variablen kann man zwar nehmen. Ich würde aber zu englischen tendieren, weil meist kürzer, keine Probleme mit Sonderzeichen wie Umlauten, ß und so weiter. Und man hat kein Denglisch im Code. Englisch ist Standard, sowohl was Sprachkonstrukte, Datentypen als auch Bibliotheken angeht. FILE *file wirkt ist daher einheitlicher wie FILE *datei.

    Bei der Struktur dasselbe, oben ist der Code sauber eingerückt, unten nicht, dann mal wieder halb. Erschwert das Lesen, genau wie die Verschachtelungen. Genau wie die fehlenden Leerzeichen in Funktionsaufrufen, Schleifen etc. Langsam aber sicher ist da eine Länge erreicht, wo man nicht mehr alles in die Main klatschen sollte, sondern stattdessen in Funktionen gliedern. Eine IDE (oder teils auch gute Texteditoren) sind da hilfreich, da sie das Meiste automatisch sauber und einheitlich formatieren. Visual Studio beispielsweise formatiert automatisch beim Einfügen, Beenden einer Zeile (Semikolon, Geschweifte Klammer usw). Da kann er dann z.B. eine For-Schleife auch aneinandergeklatscht als for(l=0;l<m;l++) schreiben, spätestens beim Setzen der Klammern macht VS dann for(l = 0; l < m; l++) daraus.

    Teilweise wäre es wohl noch möglich, Deklaration und Zuweisung gleichzeitig durchzuführen:
    unsigned int *x, *y, i, q = 1000000, eingabe; 
    

    Statt

    unsigned int *x, *y, i, q, eingabe;
    q=1000000; //Größe des Reservierten speichers für das Array für eine Messung

    Dadurch wird der Code nicht unnötig aufgebläht.

    Zur C-Spezifischen Codequalität kann ich wenig sagen. Wie schon erwähnt kenne ich mich mit den Low Level C-APIs nicht aus, da ich ausschließlich mit Hochsprachen wie C# arbeite. Es gibt da einige Fallstricke, beispielsweise NullPointer und ähnliches. Bei so klassischen C-Fallstricken fehlt mir aber die Tiefe, weil ich das im Alltag mit den Hochsprachen gar nicht habe. Da sind Zeiger selbst schon unsicherer Code, der gemäß Best Practices nicht verwendet werden soll. Spontan fällt mir nur auf, dass das Errorhandling fehlt. Bei den Eingaben beispielsweise, was passiert da wenn ich Blödsinn (abc) eintippe? Bin mir ohne das zu testen nicht mehr sicher, aber soweit ich mich erinnere, schmiert da bei C alles ab.

    Eventuell kann da jemand mit Erfahrung auf dem Gebiet mehr zu sagen. Ich kann nur dazu raten, vorsichtig zu sein, und sich in die Thematiken einzulesen. Grade in C hat der Entwickler SEHR viel Macht, und damit auch Verantwortung. Man kann vieles falsch machen, was sich in (schwer zu findenden) Bugs und Sicherheitslücken äußert. Solche Low Level Programmierung würde ich daher heutzutage grundsätzlich nur noch einsetzen, wenn es absolut nicht anders geht.


  4. The Following 4 Users Say Thank You to DMW007 For This Useful Post:

    DotNet (16.09.2017), Negok (09.09.2017), Nuebel (08.09.2017), Tiger18 (22.05.2020)

  5. #3
    Avatar von Nuebel
    Registriert seit
    23.11.2013
    Beiträge
    446
    Thanked 361 Times in 236 Posts

    Standard AW: Code-Review: Zahlen Sortieren in C

    DMW007 hat schon viele gute Punkte genannt.


    x = (unsigned int*) malloc((q*eingabe) * sizeof(unsigned int));//Speicher Reservieren
    y = (unsigned int*) malloc((q*eingabe) * sizeof(unsigned int));//q*(Anzahl Messungen mit


    In C muss man das Ergebnis einer Speicherreservierung nicht casten, weil ein void-Pointer automatisch und sicher jeden anderen Typ annehmen kann. Das sizeof(typ) würde ich durch ein sizeof *pointer ersetzen. Wenn man sich später entschließt, den Typ des Pointers zu ändern, muss man es nur in der Deklaration tun, und nicht auch noch in der Argumentenliste von malloc:


    x = malloc(q * eingabe * sizeof *x);
    y = malloc(q * eingabe * sizeof *y);


    In C würde ich außerdem immer zumindest stdint.h inkludieren. Die Wertebereiche von int, unsigned short etc. sind maschinenabhängig. Mit stdint.h bekommt man, was man schreibt.

    unsigned short tauschen, j, k, d, l;
    unsigned int ablage, w, n=0;

    wird zu:

    uint16_t tauschen, j, k, d, l;
    uint32_t ablage, w, n = 0;


    Ab C99-Standard müssen Deklaration und Definition von Variablen nicht getrennt sein. So kann auch die Zählvariable direkt in der for-Schleife definiert werden, was m.M.n. lesbarer ist.
    Geändert von Nuebel (08.09.2017 um 10:11 Uhr)

  6. The Following 3 Users Say Thank You to Nuebel For This Useful Post:

    DMW007 (08.09.2017), Negok (09.09.2017), Tiger18 (22.05.2020)

  7. #4

    Registriert seit
    02.01.2013
    Beiträge
    879
    Thanked 458 Times in 313 Posts

    Standard AW: Code-Review: Zahlen Sortieren in C

    Noch eine Kleinigkeit... Konstanten würde ich nicht mitten im Code vergraben, denn es könnte sein, daß man sie später mal ändern muß.

    Also statt:

    q=1000000;

    im Header ein:

    #define ARRAYGROESSEMESSUNG 1000000

    und im Hauptcode entweder:

    q=ARRAYGROESSEMESSUNG;

    oder überall wo jetzt q steht stattdessen ARRAYGROESSEMESSUNG einsetzen. Die 1000000 werden dann zur Kompilierzeit ersetzt.

    Wichtig ist wie gesagt, diese Konstantendefinitionen an einer Stelle in einer Header-Datei zu sammeln, sodaß man sie leicht findet. Falls das Programm keine Headerdatei hat, die defines oben bei den includes hinschreiben.
    Geändert von freulein (08.09.2017 um 22:31 Uhr)

  8. The Following 4 Users Say Thank You to freulein For This Useful Post:

    DMW007 (09.09.2017), Negok (09.09.2017), Nuebel (09.09.2017), Tiger18 (22.05.2020)

  9. #5
    Avatar von Leuchtturmwärter
    Registriert seit
    04.02.2013
    Beiträge
    61
    Thanked 46 Times in 29 Posts

    Standard AW: Code-Review: Zahlen Sortieren in C

    Hat jetzt mit dem Code an sich nicht ganz so viel zu tun, würde ich beim Pair-Programming aber auch abfragen:

    Hat sich dein Kollege absichtlich für Bubblesort entschieden?
    (Vorausgesetzt, der Code implementiert Bubblesort - die Schleife mit dem J ergibt für mich keinen Sinn, irgendwie werden erst Zahlen mit Geradem, dann mit ungeradem Index untersucht??)

    Gerade für vollständig zufällige Daten kommt bei Bubblesort die Abbruchbedingung erst relativ spät zum tragen, das Sortieren geht dann gegen O(n²).
    Andere Sortieralgorithmen lassen sich auch parallelisieren, auch wenn das evtl. ein bisschen mehr Arbeit ist
    Eine beliebte, noch nicht so besonders komplizierzte Version ist z.B. Quicksort, dass im Durchschnitt schon mit O(n * log(n)) fertig ist.
    Wenn man hier schon Performance-Messungen macht: Divide-and-Conquer -Ansätze wie Quicksort haben bei der Parallelisierung den zusätzlichen Vorteil, dass sich die CPU-Caches nicht so häufig in die Quere kommen:
    In der Regel hat jeder Core mindestens seinen eigenen L1-Daten-Cache. Hat jetzt ein Core in der aktuellen Implementierung ein Zahlenpaar verglichen, liegt die Autorität über die entsprechende Cacheline bei dem Core.
    Soll jetzt ein anderer Core zwei andere Zahlen in der selben Line vergleichen, muss sein L1-Cache erst über den Prozessor-internen Bus gesynct werden, etc. usw.
    Geändert von Leuchtturmwärter (11.09.2017 um 21:15 Uhr)

  10. The Following 3 Users Say Thank You to Leuchtturmwärter For This Useful Post:

    DotNet (16.09.2017), Negok (17.09.2017), Nuebel (16.09.2017)

  11. #6
    Avatar von DMW007
    Registriert seit
    15.11.2011
    Beiträge
    6.080
    Thanked 9.118 Times in 2.995 Posts
    Blog Entries
    5

    Standard AW: Code-Review: Zahlen Sortieren in C

    Zitat Zitat von freulein Beitrag anzeigen
    im Header ein:

    #define ARRAYGROESSEMESSUNG 1000000
    Konstanten zu benutzen ist eine gute Idee, aber ich würde sie zur besseren Lesbarkeit nicht als Fließtext benennen. Sondern mehrere Wörter mit Pascal/CamelCase bzw. Underscores trennen. Im C-Umfeld dürfte Letzteres gebräuchlicher sein, wie man auch an den Standardbibliotheken sieht. M_PI in math.h zum Beispiel. Aus diesem Grund und auch generell scheinen mir englische Bezeichnungen sinnvoll. So gibt es auch kein Denglisch, wenn man z.B. eine englische Standardfunktion aufruft und den Rückgabewert in einer deutschen Variable speichert. Wenn man unbedingt möchte, kann man letztendlich natürlich auch Deutsch nutzen. Dann aber zumindest konsistent, denn das ist noch wichtiger als die Art der Benennung. Keine konsistente Benennung führt immer zu Chaos, egal ob PascalCase, Underscore oder sonst was benutzt wird. In dieser Hinsicht sollte der Code vom TE aber wie schon im anderen Post vorgeschlagen ohnehin noch mal Refactort werden, nachdem er sich für eine Variante entschieden hat.


  12. The Following User Says Thank You to DMW007 For This Useful Post:

    Nuebel (16.09.2017)

Ähnliche Themen

  1. Antworten: 4
    Letzter Beitrag: 28.02.2021, 13:38
  2. [STEAM] Inventory sortieren
    Von watermeloN im Forum Gaming Allgemein
    Antworten: 8
    Letzter Beitrag: 15.06.2017, 12:48
  3. Antworten: 1
    Letzter Beitrag: 08.11.2014, 21:30
  4. Nach was Musik sortieren?
    Von Open Thought im Forum Musik ♫
    Antworten: 10
    Letzter Beitrag: 09.09.2014, 01:08
  5. Antworten: 1
    Letzter Beitrag: 30.04.2014, 01:58
Diese Seite nutzt Cookies, um das Nutzererlebnis zu verbessern. Klicken Sie hier, um das Cookie-Tracking zu deaktivieren.