Feedback von Sven zu DATEV, mit Stand von 39499f9

Technische Dokumentation
09.11.2017

* Objekt vs Imperativ

Im Moment sieht der Aufruf in SL::DATEV so aus:

  ($datev_ref, $self->{warnings}) = SL::DATEV::CSV->new(datev_lines  => $self->generate_datev_lines,
                                                        from         => $self->from,
                                                        to           => $self->to,
                                                        locked       => $self->locked,
                                                       );

und die Implementation dazu so:

  sub new {
    my $class = shift;
    my %data  = @_;

    my $obj = bless {}, $class;

    # ...

    return _csv_buchungsexport(from        => $data{from},
                               to          => $data{to},
                               datev_lines => $data{datev_lines},
                               locked      => $data{locked},
                              );

    $obj;
  }

Das ist natürlich nix Halbes und nix Ganzes. Ich vermute, Du wolltest Da das Richtige machen und hattest bisher nur keine Zeit dazu. Das ist an der Struktur auch das einzige was ich noch als kritisch sehe.

Kurz nochmal als Überblick die Eingenschaften von Objekten und Imperativ im Gegensatz:

Objekte:
- haben internen Zustand
- werden erzeugt mit new
- danach passieren alle Interaktionüber Methoden
Imperativ:
- Prozeduren haben keinen internen Zustand
- Prozeduren werden direkt aufgerufen und werden nicht erzeugt
- Alle Daten müssen direkt mitübergeben werden, und direkt zurückgegeben werden.

Was hier also falsch aussieht:

- SL::DATEV::CSV->new sollte ein einzelnes Objekt zurückgeben, auf dem dann gearbeitet wird.
- Die letzte Zeile von sub new wird nie erreicht
- Alle SL::DATEV::CSV Methoden werden nie auf dem Objekt aufgerufen
- ...und brauchen Tonnen von Parametern die dem Objekt schon bekannt sind

Du schreibst in den Kommentaren selber schon, dass Du die Parameter in Attributen unterbringen willst, also machen wir das einfach mal flott:

Zuerst, wie soll der Aufruf in SL::DATEV aussehen? Ich sag einfach mal so:


  my $datev_csv = SL::DATEV::CSV->new(
    datev_lines  => $self->generate_datev_lines,
    from         => $self->from,
    to           => $self->to,
    locked       => $self->locked,
  );

Was ist $datev_csv? Interessiert uns an dieser Stelle nicht. Objektparadigma sagt, dass wir darauf Methoden aufrufen um es sinnvolles Sachen tun zu lassen:


  $datev_csv->header   # soll uns den Header als arrayref von Elementen zurückgeben
  $datev_csv->lines    # soll uns die ein Arrayref von Zeilen zurückgeben, die jeweils Arrayrefs sind
  $datev_csv->warnings # soll uns die Warnings geben, wenn vorhanden

Wie es das macht? Uns egal. Ersteres benutzen wir einfach weiter unten in der Zeile die die Zeilen rausschreibt und die warnings tun wir wie hoerher auch in der Slot vom eigenen Objekt:

  $csv->print($csv_file, $datev_csv->header);
  $csv->print($csv_file, $_) for @{ $datev_csv->header };
  ...
  $self->{warnings} = $datev_csv->warnings;

Das sieht sauber aus.

Wie macht man die andere Seite davon? Zuerst lassen wir sub new das machen was sie vorher auch gemacht hat:


  sub new {
    my ($class, %data) = @_;

    croak(t8('We need a valid from date'))      unless (ref $data{from} eq 'DateTime');
    croak(t8('We need a valid to date'))        unless (ref $data{to}   eq 'DateTime');
    croak(t8('We need a array of datev_lines')) unless (ref $data{datev_lines} eq 'ARRAY');

    my $obj = bless {}, $class;
    $obj->$_($data{$_}) for keys %data;
    $obj;
  }

Die vorletzte Zeile ist ein netter Trick: Die nimmt einfach alle Parameter, und ruft eine Methode gleichen Namens auf. Das heißt:


  my $datev_csv = SL::DATEV::CSV->new(
    datev_lines  => $self->generate_datev_lines,
    from         => $self->from,
    to           => $self->to,
    locked       => $self->locked,
  );

ist äquivalent zu:


  my $datev_csv = SL::DATEV::CSV->new;
  $datev_csv->datev_lines($self->generate_datev_lines);
  $datev_csv->from($self->from);
  $datev_csv->to($self->to);
  $datev_csv->locked($self->locked);

...und das ist gut so, weil das unserem Paradigma von oben entspricht: Alle Interaktionen finden nur über Methoden statt.

Also müssen wir dafür sorgen, dass diese vier Methoden auch existieren. Du hast dafür in SL::DATEV ja schon ein paar Mini-attribut-accessoren für locked, use_pk und warnings geschrieben. Du kannst entweder das gleicher hier wieder machen, oder Du nimmst einfach den Rose Methodmaker, der das für Dich macht:


use Rose::Object::MakeMethods::Generic (
  scalar => [ qw(datev_lines from to locked warnings) ],
);

...fertig.

Falls Du jetzt denkst "Moment, SL::DATEV::CSV ist doch aber gakein Rose Objekt" - stimmt. Macht aber nichts, weil Rose::Object::MethodMaker auch nur generische Funktionen zur runtime erzeugt, die mehr oder weniger genauso aussehen, wie Das was Du manuelll schreiben würdest:


  # aus /usr/share/perl5/Rose/Object/MakeMethods/Generic.pm:
      sub
      {
        return $_[0]->{$key} = $_[1]  if(@_ > 1);
        return $_[0]->{$key};
      }

Und nichts davon braucht die Abhängigkeit von Rose::Object. Um genau zu sein macht das ableiten von Rose::Object auch nur eine Sache: Es gibt Dir eine sub new mit einer etwas schlaueren vorletzten Zeile, die auch mit mehrfachaufrufen klar kommt.

Okay, das "new" funktioniert nun. Jetzt fehlen noch:


  $datev_csv->header
  $datev_csv->lines
  $datev_csv->warnings

warnings funktioniert auch schon, weil es oben mit in der Attributliste drinsteht.

header gibt es schon, heißt im Moment nur anders, und braucht Parameter.

Also:

- Lösch den Teil aus _csv_buchungsexport, der Sachen mit header macht
- Lösch den Teil aus _generate_csv_header, der die Parameter checkt (die haben wir ja schon gecheckt), und nimm stattdessen nur ein einziges Argument: $self
- Und weil der Parameter "first_day_of_fiscal_year" dabei flöten geht, machen wir uns dafür ne Helfermethode:

sub first_day_of_fiscal_year {
  $_[0]->to->clone->truncate(to => 'year')
}
- Änder alle $params{x} Stellen auf $self->x
- und weil das alles unformatierte Daten sind, formatierst Du dabei direkt: $self->to->ymd('').
- Gib am Ende \@header statt @header zurück.
- Und benenn die Funktion in 'header' um.

Fertig.

Bleibt als letztes lines.

Auch hier, die gibts schon, die heißt nur anders, und nimmt komische Argumente. Also wieder:

- Parameterbehandlung raus. my ($self) = @_; ist die magische Formel für alles.
- Wieder $params{x} auf $self->x ändern
- Alles mit header ist schon raus (siehe oben)
- Die warnings geben wir am Ende nicht zurück, sondern stecken sie in den attributslot dafür: $self->warnings(\@warnings);
- Benenn die Funktion in 'lines' um.

Auch Fertig.

An dieser Stelle noch als Erinnerung: Methoden mit "_" Präfix ist kein Sprachfeature sondern eine Konvention, dass sie für den internen Gebrauch sind. Weil header und lines jetzt von extern aufgerufen werden, haben sie jetzt keins mehr.

Dateien

feedback_datev_sven.txt Magnifier (7,028 KB) Jan Büren, 09.11.2017 10:00