Clean Code / Pull Request in Firmen


#1

Hallo,

sagt mir bitte ob ihr in euren Firmen Pull Requests benutzt.
Wie oft stößt man auf Pull Requests in Firmen heutzutage.
Ich meine z.B. Bitbucket configuriert mit Jira.
Ich bin auch sehr gespannt wie oft in euren Firmen die Clean Code Regeln beachtet werden.

Danke im Voraus.


#2

Ich will erfahren, wie oft in eurer Karriere ihr auf pull requests in Projekten gestoßen habt und wie oft clean Code Regeln bei eurer Entwicklung in Teams beachtet wurden.
Ich möchte im allgemeinen erfahren ob Firmen oft oder selten Sie benutzen.
Vor 5 Jahren wurden pullrequests sehr selten benutzt. Wie sieht das heute aus. Hat es sich verbessert? Ich finde pull request wunderbar und clean Code die wichtigste Sache im Projekt. Darum frage ich. Ich hasse in unschönen Code zu wühlen.


#3

Also bei uns werden Pull-Requests dann genommen, wenn neue Mitarbeiter in das Projekt hineinkommen.

Damit kann man dann besser sehen, ob der neue Code zum Rest “passt”.

Selbstverständlich werden hier die Regeln u.a. von Clean Code angewendet. Wir nutzen Sonar Cube für eine statische Quellcode Analyse.

Schöne Grüße

Martin


#4

Wie kommst du denn darauf?

Das letzte mal dass ich in einer Firma gearbeitet habe wo keine Pull Requests vorgeschrieben waren, war 2011 und das war eine Firma die nicht auf SW spezialisiert war.

Seitdem gibt es keine einzige Aenderung ohne Pull Request bei meinen Arbeitgebern.

Clean Code Regeln nach Bob Martin sind ein anderes Thema, jedes Team muss sich selber Regeln festlegen die es einhalten will.

Nachtrag:
Fuer Firmen die an den US Boersen gefuehrt werden sind PRs sozusagen Vorschrift, damit nicht ein einzelner Mitarbeiter in der Lage ist grossen Schaden zu verursachen:


#5

In meinem aktuellen Projekt (seit 1.5 Jahren) werden alle Änderungen über Pull Requests erledigt. Jenkins lässt immer die Tests/Linter/Formatter laufen und ohne “grün” kann auch nichts gemerged werden.

Also bei uns werden Pull-Requests dann genommen, wenn neue Mitarbeiter in das Projekt hineinkommen.

Das ist zwar besser als nichts, aber für mich sind Pull Requests in erster Linie dazu da, um

  1. Fehler zu finden.
  2. Etwas von den anderen zu lernen oder anderen etwas beizubringen.

Beides trifft genauso gut auf die alten Hasen wie die Neulinge zu. Meine Erfahrung macht mich weder fehlerfrei noch “unverbesserungswürdig”.


#6

Bei uns sind Pull Requests auch Pflicht. Wir haben natürlich auch Regeln und “Best Practices” festgelegt, und wir lassen CheckStyle und FindBugs laufen. Vor dem Pushen müssen auch die zugehörigen Testsuiten durchgelaufen sein. Das alles hat schon viel Mist verhindert.


#7

Wir haben auch viele kleine Änderungen… Mich interessiert, wie ihr die Pull-Requests organisatorisch handelt.

Werden diese automatisch freigegeben und durchgeführt, oder führt dies eine ausgezeichnete Person durch?


#8

Unser Kanban-Board hat eine dedizierte CR-Spalte, zwischen “in progress” und “QA testing”. Spätestens wenn man “fertig” ist, öffnet man einen Pull-Request (und startet Tests auf Jenkins) und schiebt den Issue auf “CR”. Die Leute, die Code Review machen sollen, kann man sich selbst aussuchen, wobei bestimmte Code-Teile des Projekts auch “Code-Owner” haben, die automatisch eingeladen werden. Ob man den CR als eingeladene Person auch macht, steht einem prinzipiell frei, auch ob man CR alleine machen will, oder sich kompliziertes Zeug gleich vom Autor erklären lässt. Ebenso kann man CR selbst dann machen, wenn man nicht eingeladen ist, aber denkt, dass sein Input nützlich sein kann. Kann durchaus sein, dass du z.B. an deinem Pull Requests Anmerkungen vom CTO (der irgendwie seine Augen überall hat) findest. Das System finden - soweit ich weiß - alle gut, es ist höchstens jemand genervt, wenn er zu lange auf sein grünes Häkchen warten muss, oder es mit einem Reviewer altbekannte Streitpunkte gibt.


#9

Nur als kleines Kontrastprogramm: Jeder committet direkt in master (oder wo hin auch immer er will). Wenn dabei etwas kaputt geht, muss sich derjenige darum kümmern, dessen Funktionalität dabei kaputt gegangen ist.


#10

Dann kommt hier noch mehr Kontrast.

Wir fangen gerade erst an, unsere Projekte auf Git umzustellen, bisher läuft alles über SVN. In einem Projekt finden sich sogar noch Überbleibsel von CVS, weil irgendwer seine komplette Kopie ins SVN gepackt hatte.

Wir taggen jedes “Release” was an den Kunden geht, komplexe Funktionen werden in einem Branch entwickelt und kleinere Sachen im trunk. Bei BugFixes wird je nach Komplexität, Schwere und Dringlichkeit entschieden.


#11

Ist ja einfach, da reicht ein git revert… :wink:


#12

Also wenn du bei uns den Master kaputt machst, werden erst einmal mehrere Schalen grobkörnigen Spotts über deinem Haupt entleert. Passiert das regelmäßig, wird sich dein Chef freundschaftlich mit dir darüber austauschen. Was häufiger passiert ist, dass jemand Tests bricht (weil viele Tests lokal gar nicht laufen können). Aber eins ist sicher: Wer’s kaputt macht, fixed es auch wieder. Und wenn es der CTO ist.


#13

Das Problem wird dramatisch abgeschwächt, wenn erst vieeel später gemerkt wird, dass etwas “kaputt” ist. Da müßte dann ja erstmal jemand den kausalen Zusammenhang zu einem commit nachweisen, wie soll das gehen?

(Ansonsten halt’ ich mich hier jetzt mal raus. Ich weiß, wie es besser gehen würde. "Keine commits in master" wäre sehr hart, aber je nach Granularität der Tasks sicher nicht das sinnloseste. Darüber hinaus gibt es immer Punkte, die man verbessern könnte, aber … das muss sich dann auch lohnen. Wenn’s schlicht niemanden interessiert, was man macht, kann man seine Prioritäten ja persönlich setzen, wie man will…)


#14

Die meisten haben doch überhaupt keine Ahnung von Clean Code. Man darf doch heute schon froh sein, wenn jemand weiss, dass es JUnit heißt und nicht JUnix. Geschweige denn, wie man einen Test schreibt oder ausführt.

Wie Marco eben anmerkt. Es gibt sogenannte Verdeckte Fehler. Jemand schreibt eine grottige Funktion, die überhaupt nicht aufgerufen wird. Jemand anders ruft diese Funktion dann später korrekt auf und erhält Müll und bricht damit das System. Der darf dann noch zur Belohnung den Mist des anderen ausbügeln?

Die Realität sieht doch teilweise so aus, dass die Leiterin der Softwareentwicklung meint, hey ich committe am besten alles erst abends. Sind dann viele Änderungen aber passt dann schon irgend wie zu den jeweiligen Messages. So grob. Und alle anderen sollten dies ebenso machen, sonst müsste ich mergen. Mag ich garnicht. Ist voll doof.


#15

Ich erstelle einen PR und weise ihn mehreren Kollegen zu (welche sich am besten mit dem Thema auskennen). Diese reviewen das Ganze und entweder gibt es gar nichts auszusetzen (Approve + Merge), oder es sind nur “Nice-To-Haves” (Approve oder Kommentar) oder es gibt schwerwiegende Probleme (Reject).


#16

:astonished: