3. Code-Pruefen

Code-Pruefen

  • V

    Hey Leute,

    Ich bin neu hier (und in C++) und hoffe das ist das richtige Forum.
    Ich habe für meine Schule ein Programm geschrieben, welches alle dateien in einem Ordner ("share"), die älter sind als 7 tage, loescht.
    Des weiteren sollen Dateien und Ordner, welche von Lehrern erstellt wurden, nicht geloescht werden.
    Habe den Quellcode soweit schon fertig. Könntet ihr mal drüber schauen und mir sagen, wo man etwas vlt. noch besser machen könnte?

    //standard input-output library
#include <iostream>
//um verzeichnisse auszulesen
#include <dirent.h>
//um strings zu benutzen
#include <string>
//um anhand uid uname zu bekommen
#include <pwd.h>
//um vektoren zu benutzen
#include <vector>
//z.B. für .c_str()
#include <sys/stat.h>
//um dateien zu lesen
#include <fstream>
//um zeit zu ermitteln
#include <ctime>
//um system-funktionen aufzurufen
#include <cstdlib>

using namespace std;

//wichtigste Variablen
const string SHAREPATH = "/home/willi/Desktop/share/";
const int DAYS = 7;
vector<string>files(0);
vector<string>teachers(0);

//Liefert Dirs
struct dirent* entry;

//Liefert anhand uid uname
struct passwd* owner;

//Liefert Dateiinformationen
struct stat info;

//Funktionsprototypen
void showDir(DIR*);
int besitzerErmitteln(string);
int anzahlDateien(DIR*);
int readTeacherFile();
int checkFileMode(string&);
int loescheFD(string&, string);
string getFullPath(string&);

int main(){
  DIR* share_p;

//tausch oeffnen
  if((share_p = opendir(SHAREPATH.c_str())) == NULL){
    cerr << "Tausch-Ordner konnte nicht geoeffnet werden!" << endl;
    return(1);
  }

//leherdatei lesen
  if(readTeacherFile() != 0){
    cerr << "teachers.list konnte nicht geoeffnet werden" << endl;
    return(1);
  }
//Dateien anzeigen + speichern
  showDir(share_p); 
//Jede Datei durchgehen
  for(int i = 0; i < files.size(); i++){
//Auf Dateien/Files pruefen
    if(checkFileMode(files.at(i)) == 0){
//DIR
      loescheFD(files.at(i), "rm -r ");  
    }else{
//FILE
      loescheFD(files.at(i), "rm ");
    }
  }
  closedir(share_p);
  return(0);
}

void showDir(DIR* share_p){
  files.clear();
//alle verzeichnisse auslesen
  while((entry = readdir(share_p))){
//alle 'falschen' dateien ausschließen
    if(*entry->d_name != '.' && besitzerErmitteln(entry->d_name) == 0){
      files.resize(files.size()+1);
      files.at(files.size() - 1) = entry->d_name;
      cout << files.at(files.size() - 1) << endl; 
    }
  }
} 

int besitzerErmitteln(string fdname){
  time_t datum_aktuell;
  datum_aktuell = time(NULL)/3600/24;
  int pruefe_lehrer = 0;  
//uid und uname ermitteln
  stat(getFullPath(fdname).c_str(), &info);
  owner = getpwuid(info.st_uid);
//ersteller auf teacher pruefen
  for(int i = 0; i < teachers.size(); i++){
    if(teachers.at(i).compare(owner->pw_name) == 0){
      pruefe_lehrer = 1;
      break;
    }
  }
//0, wenn datei nicht von lehrer & zu alt
  if(pruefe_lehrer == 0 && datum_aktuell - (info.st_mtime/3600/24) > DAYS){
    return(0);
  }else{
    return(1); 
  }
  return 0;
}

int checkFileMode(string& fdname){
    stat(getFullPath(fdname).c_str(), &info);
//wenn file = directory -> return 0
    if(info.st_mode & S_IFDIR)
    {
      return(0);
    }else{
      return(1);
    }
}

int readTeacherFile(){
//teacherlist lesen und in vektor lesen
  ifstream teacherfile;
  string output;
  teacherfile.open("teachers.list");
  int zaehler = 0;
  if(teacherfile.is_open()){
    while(teacherfile.good()){
      getline(teacherfile, output);
//EOF aus array loeschen
      if(output != ""){
	teachers.resize(teachers.size() + 1);
	teachers.at(zaehler) = output;     
        zaehler++;
      }
    }
    teacherfile.close();
    return(0);
  }else{
    return(1);
  }
}

int loescheFD(string& fdname, string delOperation){
  //File/Dir loeschen
  delOperation.append(getFullPath(fdname));
  if(system(delOperation.c_str())){
    return(0);
  }else{
    return(1);
  }
}

string getFullPath(string& fdname){
  //fullpath ermitteln + return
  string filepath = SHAREPATH;
  filepath.append(fdname);
  return(filepath);
}
    0
  • Mod

    Zunächst einmal Stil:
    - Wichtig: Alle globalen Variablen weg! Die sorgen nur für komplizierte Fehler.
    - Mach unnötige Kommentare weg. Zum Beispiel:

    //um strings zu benutzen
#include <string>

    Ach!
    - Mach falsche Kommentare weg. Zum Beispiel:

    //z.B. für .c_str()
#include <sys/stat.h>

    Bestimmt nicht!
    - Mach irreführende Kommentare weg. Zum Beispiel:

    //Liefert Dirs
struct dirent* entry;

    Das liefert gar nichts, das ist eine Variable, die enthält höchstens etwas.
    - return mit Klammern ist eher eine ungewöhnliche Schreibweise
    - Mach unnötige Anweisungen weg:

    teacherfile.close();
    return(0);

    Und genau beim return würde der Dateistream automatisch zugehen (mal Referenz zum fstream-Destruktor durchlesen - Dateien schließen sich in C++ automatisch wenn sie aus dem Scope gehen). Also völlig unnötig.
    - Ich war ja nie ein Fan von erst Deklarieren, später definieren, wenn man sowieso den gesamten Quellcode in eine einzige Datei schreibt. Aber ist Geschmackssache. Erinnert mich jedenfalls immer an den Stil von Jürgen Wolf, daher eventuell zu unrecht negativ belegt.
    - Ich persönlich mag auch deinen Einrückungsstil nicht (auch wenn er durchaus so üblich ist). Das musst du nicht verbessern, aber ich gucke nicht so intensiv über das Programm, da ich es schlecht lesen kann.

    Technik:
    - Es wäre eine Überlegung wert, die verbreitete und plattformunabhängige Boost-Bibliothek zu nutzen. Die hat auch einen Teil extra zum Arbeiten mit Dateisystemen (Boost Filesystem) und kapselt diesen ganzen C-Stil-Systemkram schön weg.
    - Das gilt ebenso für Datums- und Zeitverarbeitung
    - Lerne die Container richtig zu benutzen!

    teachers.resize(teachers.size() + 1);
    teachers.at(zaehler) = output;    
        zaehler++;

    😮
    Du programmierst die wohl wichtigste Methode des vectors, push_back, extrem umständlich nach.
    - Const correctness! Googeln, verstehen, anwenden! Das hilft dir, sehr viele Fehler gleich zur Compilezeit zu entdecken.
    - Potentiell große Datenpakete (string) sollten per (const-)Referenz an Funktionen übergeben werden, das spart Kopierarbeit.

    Fehler die mir auffallen:
    - Logikfehler:

    while(teacherfile.good()){
      getline(teacherfile, output);
      // Verarbeitung
   }

    Direkt aus einem der vielen schlechten Lehrbücher übernommen. Falls du so eines haben solltest: Wegwerfen!
    Du prüfst auf Erfolg (...good()), dann machst du etwas fehleranfälliges(getline), dann verarbeitest du das Ergebnis. Jetzt denk mal nach, was passiert, wenn der zweite Schritt schief geht. Typischerweise willst du so etwas wie

    while(getline(teacherfile, output))
{
 // Verarbeitung
}

    Das war jetzt schon jede Menge, manches pingelig, manches ernst. Ich denke, du hast erst einmal zu tun. Wenn du alles (oder wenigstens vieles) verbessert hast, kann man noch einmal gucken. Dann wird auch alles viel übersichtlicher sein, dann kann man mehr verstehen und besser helfen.

    0
Anmelden zum Antworten