Projekt

Allgemein

Profil

Feedback von Sven zu DATEV, mit Stand von 39499f9 » feedback_datev_sven.txt

Jan Büren, 09.11.2017 10:00

 
1
h1. * Objekt vs Imperativ
2

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

    
5

    
6
<pre>
7
  ($datev_ref, $self->{warnings}) = SL::DATEV::CSV->new(datev_lines  => $self->generate_datev_lines,
8
                                                        from         => $self->from,
9
                                                        to           => $self->to,
10
                                                        locked       => $self->locked,
11
                                                       );
12
</pre>
13

    
14
und die Implementation dazu so:
15

    
16
<pre>
17
  sub new {
18
    my $class = shift;
19
    my %data  = @_;
20
  
21
    my $obj = bless {}, $class;
22
  
23
    # ...
24
    
25
    return _csv_buchungsexport(from        => $data{from},
26
                               to          => $data{to},
27
                               datev_lines => $data{datev_lines},
28
                               locked      => $data{locked},
29
                              );
30
    
31
    $obj;
32
  }
33
</pre>
34

    
35
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.
36

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

    
39
  Objekte:
40
    - haben internen Zustand
41
    - werden erzeugt mit new
42
    - danach passieren alle Interaktionüber Methoden
43

    
44
  Imperativ:
45
    - Prozeduren haben keinen internen Zustand
46
    - Prozeduren werden direkt aufgerufen und werden nicht erzeugt
47
    - Alle Daten müssen direkt mitübergeben werden, und direkt zurückgegeben werden.
48

    
49

    
50
Was hier also falsch aussieht:
51

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

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

    
59

    
60
Zuerst, wie soll der Aufruf in SL::DATEV aussehen? Ich sag einfach mal so:
61
<pre>
62

    
63
  my $datev_csv = SL::DATEV::CSV->new(
64
    datev_lines  => $self->generate_datev_lines,
65
    from         => $self->from,
66
    to           => $self->to,
67
    locked       => $self->locked,
68
  );
69
</pre>
70

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

    
74
  $datev_csv->header   # soll uns den Header als arrayref von Elementen zurückgeben
75
  $datev_csv->lines    # soll uns die ein Arrayref von Zeilen zurückgeben, die jeweils Arrayrefs sind
76
  $datev_csv->warnings # soll uns die Warnings geben, wenn vorhanden
77
</pre>
78

    
79
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:
80

    
81
<pre>
82
  $csv->print($csv_file, $datev_csv->header);
83
  $csv->print($csv_file, $_) for @{ $datev_csv->header };
84
  ...
85
  $self->{warnings} = $datev_csv->warnings;
86
</pre>
87

    
88
Das sieht sauber aus.
89

    
90
Wie macht man die andere Seite davon? Zuerst lassen wir sub new das machen was sie vorher auch gemacht hat:
91
<pre>
92

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

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

    
100
    my $obj = bless {}, $class;
101
    $obj->$_($data{$_}) for keys %data;
102
    $obj;
103
  }
104
</pre>
105

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

    
109
  my $datev_csv = SL::DATEV::CSV->new(
110
    datev_lines  => $self->generate_datev_lines,
111
    from         => $self->from,
112
    to           => $self->to,
113
    locked       => $self->locked,
114
  );
115
</pre>
116
  
117
ist äquivalent zu:
118
<pre>
119

    
120
  my $datev_csv = SL::DATEV::CSV->new;
121
  $datev_csv->datev_lines($self->generate_datev_lines);
122
  $datev_csv->from($self->from);
123
  $datev_csv->to($self->to);
124
  $datev_csv->locked($self->locked);
125
</pre>
126

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

    
129
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:
130
<pre>
131

    
132
use Rose::Object::MakeMethods::Generic (
133
  scalar => [ qw(datev_lines from to locked warnings) ],
134
);
135
</pre>
136

    
137
...fertig.
138

    
139
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: 
140
<pre>
141

    
142
  # aus /usr/share/perl5/Rose/Object/MakeMethods/Generic.pm:
143
      sub
144
      {
145
        return $_[0]->{$key} = $_[1]  if(@_ > 1);
146
        return $_[0]->{$key};
147
      }
148
</pre>
149

    
150
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.
151

    
152
Okay, das "new" funktioniert nun. Jetzt fehlen noch:
153
<pre>
154

    
155
  $datev_csv->header
156
  $datev_csv->lines
157
  $datev_csv->warnings
158
</pre>
159

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

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

    
164
Also:
165

    
166
 - Lösch den Teil aus _csv_buchungsexport, der Sachen mit header macht
167
 - 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
168
 - Und weil der Parameter "first_day_of_fiscal_year" dabei flöten geht, machen wir uns dafür ne Helfermethode:
169
<pre>
170

    
171
sub first_day_of_fiscal_year {
172
  $_[0]->to->clone->truncate(to => 'year')
173
}
174
</pre>
175

    
176
 - Änder alle $params{x} Stellen auf $self->x
177
 - und weil das alles unformatierte Daten sind, formatierst Du dabei direkt: $self->to->ymd('').
178
 - Gib am Ende \@header statt @header zurück.
179
 - Und benenn die Funktion in 'header' um.
180

    
181
Fertig.
182

    
183
Bleibt als letztes lines.
184

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

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

    
193
Auch Fertig.
194

    
195
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.
196

    
    (1-1/1)