Rätsel mittels XOR lösen (Feedback)



  • Hey, es wurde bereits hier über die Lösung des Rätsels diskutiert: https://www.c-plusplus.net/forum/342732

    Ich habe das Programm jetzt in C programmiert und möchte mir bitte ein paar Tips holen. Was denkt ihr zur der Art und Weise wie es programmiert wurde und wie würdet ihr es verbessern, anpassen?

    #include <stdio.h>
    #include <math.h>
    #include <stdbool.h>
    
    void FindAndPrint(int states[]);
    int testStatues(int states[]);
    bool genPoss(int * begin, int * end);
    int BinTodec(int bin[]);
    
    int main(void)
    {
      int states[7] = { 0 };
    
      printf("You have to activate(1) following statues in any order to reveal the exit:\n");
    
      FindAndPrint(states);
    
      return 0;
    }
    
    //finds and prints solutions
    void FindAndPrint(int states[])
    {
      int i = 0;
      int dec = 0;
    
      do
      {
        dec=testStatues(states);
       // printf("%d\n", dec); // just to see all possible final states
    
        if (dec == 127) //if all statues are activiated (1111111)...
        {
          for (i=0; i < 7; i++)
          {
            printf("Statue %d: %d\n",i, states[i]); //...print the solution
          }
        }
    
      } while (genPoss(states + 0, states + 7));
    }
    
    /*
    * function for computing the final state of n statues after activating them
    * returns the final state in decimal representation
    */
    
    int testStatues(int states[])
    {
      int StateBuff[7] = { 0 };
      int position[7] = { 0 };
      int cnt = 0;
      int dec = 0;
      int buffer = 0;
    
      int i = 0;
      int j = 0;
      int n = 0;
    
      for (i = 0; i < 7; i++)
      {
        StateBuff[i] = states[i]; //saving current states
      }
    
      for (i = 0; i < 7; i++)
      {
        if (StateBuff[i] == 1) //find activated statues
        {
          position[n] = i; //and save them for later to compute the final state
          n++;
        }
      }
      cnt = n;
      n = 0; 
    
      for (n = 0; n < cnt; n++)
      {
        for (j=0; j < 7; j++)
        {
          StateBuff[j] = 0;               // writing array with zeros. statues are not activated. 
        }
    
        StateBuff[position[n]] = 1;          // this block creates
                                             // the state of the statues
        StateBuff[(position[n]+3)%7]=1;      // after activating one single statue i=position[n].
        StateBuff[(position[n]+4)%7]=1;      // there are n separate state of the 7 statues
    
        dec = BinTodec(StateBuff);           //gets decimal number for easier XOR-calculation
    
        buffer ^= dec; // the final state will be computed by using XOR with each and every of n states
      }
      return buffer;
    }
    
    /*
    * Generates all possible variations of a 7 bit value: 0000000 to 1111111
    * 'begin' points at the [0]-position and 'end' at [7] from the array states[].
    * 'end' moves so long until reaches 'begin' 
    * returns 0 at the end of the list which is 1111111
    * returns 1 after creating a new possible value.
    */
    bool genPoss(int * begin, int * end)
    {
      if (begin == end)      // when begin meets end, we are done
      {                      // so we are back to zero
        return false;      // that was the last number
      }
      --end;
      if (*end == 0)   // zero was found
      {
        ++*end;            // increase to one
        return true;       // still more numbers to come
      }
      else                   // one was found
      {
        --*end;            // decrease to zero
        return genPoss(begin, end);   // call again for creating next number
      }
    }
    
    /*
    * converts a binary number, which is stored in an array, to a decimal number
    * returns deciaml value
    */
    int BinTodec(int bin[])
    {
      int i = 0;
      int dec = 0;
      int buffer = 0;
      int exp = 0;
    
      for (i=6; i >= 0; i--)
      {
        buffer = bin[i] * (int)pow(2.0, (double)exp);
        dec += buffer;
        exp++;
      }
    
      return dec;
    }
    


  • Bin mal kurz den Code durchgegangen:

    - du verwendest immer 7. Wie wäre es, den Wert in eine Konstante zu packen und diese zu verwenden?

    • type parameter[] ist effektiv type*parameter , verführt dich aber zusätzlich zu glauben, dass du hier mit Arrays statt mit Zeigern arbeitest. Dass das nicht der Fall ist, wird ersichtlich, wenn du folgenden Code mit einem vernünftigen Compiler kompilierst:
    void foo(int bar[])
    {
        printf("%zu\n",sizeof(bar));
    }
    

    Egal, wie groß das Array ist, es wird immer sizeof(int*) rauskommen, und das sagt dir der Compiler auch:

    »sizeof« on array function parameter »bar« will return size of »int *« [-Wsizeof-array-argument]
    

    - mach für BinTodec doch einfach Bitshifting.
    - wenn du deinen Arrays später garantiert einen Wert zuweist, brauchst du Nullinitialisierung ( StateBuff = {0} ) nicht.
    - wieso musst du FindAndPrint ein Array übergeben? Das Ding wird doch eh nie mehr danach von main angefasst. Entweder separierst du Finden und Ausgabe (damit man mit dem berechneten Status auch etwas macht), oder die Funktion baut sich ihr Array selbst.

    Weiter habe ich jetzt auch nicht mehr geschaut.



  • - einheitliche Namenskonventionen für Typen/Variablen/Funktionen fehlt, also entweder je Kategorie alle lowerCamelCase oder alle UpperCamelCase
    - einheitliche Deklaration von Funktionsparameter-Zeigern [] oder *, aber nicht beides im Wechsel
    - printf("...\n") ohne variable Parameter immer durch puts("") ersetzen
    - const überall wo möglich verwenden
    - bei for-Schleifen eine temp. Laufvariable immer for-lokal in der initializer clause definieren, da du ja C99 verwendest
    - funktionsübergreifend verwendete Konstanten immer modulglobal bzw. (mit geeignetem signifikantem Namen) auch global definieren, bei int immer als enum, bei dir also die 7
    - Zeiger auf undefinierten Speicher immer vermeiden (FindAndPrint,Z40,states +7) auch wenn in deinem Fall durch die aktuelle Implementierung keine Dereferenzierung stattfindet (weil zuvor dekrementiert wird); also die Implementierung so ändern, dass sie mit ausschließlich definierten Speicheradressen klarkommt

    Du siehst also, schon bei einem solchen Miniprogramm gibt es viel zu verbessern, deshalb ist deine Nachfrage prinzipiell lobenswert denn fast alle Anfänger und sich selbst als fortgeschritten oder Profi Bezeichnende geben sich mit "läuft durch","funktioniert" zufrieden und glauben, ihr Programm liefe korrekt.



  • Verstehe, habe alles geändert.

    Ich habe erfahren, dass rekursive Programmierung nicht gerade für Anfänge geeignet ist. Man kann dadurch(bei zu wenig Erfahrung) den Stack unbewusst vollschreiben. Genau das sagt auch der 1. Punkt in der 1. Antwort hier: http://stackoverflow.com/questions/3021/what-is-recursion-and-when-should-i-use-it

    Wie ist euere Meinung dazu? Bei genPoss() wird Rekursion genutzt.



  • du kannst bei rekursiven Funktionen einen Zähler als zusätzliches Argument übergeben, der die Verschachtelungstiefe mitzählt, prüft und ggf Alarm schlägt, bevor der Stack überläuft.


Log in to reply