Code-Review für Produktivcode

ADV

Hallo liebe Mitprogrammierer,
auf der Arbeit versuche ich gerade, möglichst guten Quellcode zu produzieren. Hierbei würde ich ein (gerne äußerst kritisches) Review der Profis erfragen, einerseits um zu lernen und andererseits natürlich um die Qualität des Codes zu verbessern.

Meine Frage: gibt es hier etwas zu verbessern? Ein Beispiel Ich habe bereits boost/thread/mutex durch durch std::mutex ersetzt, da mir dies einfacher vorkommt. Außerdem fehlt eine Abfrage, damit keine Bücher ohne Author oder Titel eingefügt werden. Diese werde ich morgen einbauen.
Falls also einer der schlauen Leute hier etwas zum guten Stil/Sicherheit/Performance des Codes beitragen kann wäre ich äußerst dankbar!

Mit freundlichen Grüßen
ADV

auf der Arbeit versuche ich gerade, möglichst guten Quellcode zu produzieren.

Dann sollte sich da auch jemand den Code ansehen.
Das geht natürlich nicht, weil das ganz sicher kein „Produktivcode" ist. Hausaufgabe trifft da wohl eher zu.

Warum verwendest du einen Mutex? Warum so?

_<Großbuchstabe>... ist reserviert.

Warum new? Ohne smart_pointer? Mit Memoryleak?

ADV

Erstmal danke für die schnelle Antwort!
Es handelt sich nicht um eine Hausaufgabe, aber durchaus um ein Praktikum. Da ich frisch von der Uni komme, wird davon ausgegangen, dass ich mit C++11 besser klarkomme als alt eingesessene, die eh hauptsächlich mit VB.NET arbeiten.

Da verschiedene Elemente zeitgleich auf die Funktionen zugreifen können, sollen die Mutexe z.B. das löschen von Elementen während einer Zugriffsoperation verhindern.

smart_pointer sind ein großartiger Tipp! Bei den _Großbuchstaben muss ich mal die Menschen fragen, warum das überhaupt eingeführt wurde.

Gleichzeitig geht nur mit Threads. Davon solltest du erstmal die Finger lassen.

Deine Autoren schreiben nur jeweils ein Buch?

Schlangenmensch

Wenn es tatsächlich um eine Bücher Datenbank geht, würde ich es mit einem relationalem Datanbank System versuchen

Ansonsten, wenn es um produktive Code geht (gilt aber eigentlich immer) nimm dir
a) die Zeit um dein System ordentlich zu planen/designen und
b) versuche möglichst viel mit Unit Tests abzudecken.

Ansonsten hätte ich noch "Const Correctness" und "Call by Reference" im Angebot.

SeppJ

Schlangenmensch schrieb:

a) die Zeit um dein System ordentlich zu planen/designen

Das hier. Neben den schon angesprochenen Schwächen bei der technischen Umsetzung kommt mir das Gesamtkonzept derzeit unnötig starr und weltfremd vor. Eine Büchdatenbank, die annimmt, dass jeder Autor nur genau ein Buch geschrieben hat? Dass alle Bücher unterschiedliche Titel haben? Und der Threadersteller kündigt schon an, dass er Autor und Titel zu Zwangsfeldern deklarieren möchte. Titel kann ich ja noch verstehen, aber jede Menge Bücher haben keinen Autor.

Falls die Hausaufgabe war, eine Buchdatenbank anzulegen, dann wäre dies im professionellen Bereich direkt durchgefallen. Design und Umsetzung entsprechen einem zwar begabten, aber bei weitem zu unerfahrenen Programmierer.

wob

SeppJ schrieb:

aber jede Menge Bücher haben keinen Autor.

Und jede Menge Bücher haben mehr als einen Autor.

Häufig haben sie auch noch einen Herausgeber.

Bevor du jetzt auf ISBN gehst: nicht alle Bücher haben eine ISBN (ältere Bücher nicht) und zumindest bei der ISBN10 sind auch Fälle bekannt, wo es mehr als ein Buch zu einer ISBN gibt. Es gibt angeblich sogar Bücher mit fehlerhafter ISBN... Toll, oder?

was passiert, wenn zwischen dem if (_LIBRARY.count(author)) ... und dem
_MUTEX.lock(); b = *_LIBRARY[author]; ... ein Threadwechsel stattfindet und der unterbrechende Thread das Buch vom author löscht?

da gibt es vieles:
-warum Mutext direkt mal hart in die Klasse rein - wird die IMMER multi-threaded genutzt?
-_LIBRARY[author] wenn author nicht exisitiert dann wird hier ein Eintrag erzeugt
-gross/klein-Schreibung
-findBookByAuthor: wie schon bemerkt: dein lock ist unzureichend
-RAII fehlt (lock/unlock schön von Hand)
-warum eine map und nicht ein vektor? nur damit man "schneller" nach Autoren suchen kann?
-in addItem Parameter Book b dann new Book(b)?
-addItem, warum nicht add - was soll sonst in einer BookDB als Books sein?
-viel viel lock/unlock in der Klasse - lieber auslagern