nicht angemeldet
Ich vermutete datenintegritäts- und sicherheitskritische Fehler. Diese sind tatsächlich vorhanden. Wenn ein Feldinhalt (z.B. "Nachricht") ein Semikolon enthält, verrutschen die Inhalte und werden nicht mehr den korrekten Variablen zugeordnet, die Ausgabe wird ruiniert. Inhalte werden vor der Ausgabe nicht kodiert, so dass ein Angreifer beliebigen schadhaften Code einschleusen kann. Dies ist ernst und gehört dringend verbessert! Ich spare mir die Details, bei der kompletten Überarbeitung bleibt nichts mehr vom fehlerhaften übrig.
Nun zum Design auf hoher Abstraktion: du benutzt keine bewährten Methoden und Mittel, um deine Ziele umzusetzen, vermutlich aus Unkenntnis, dass es sie gibt (nämlich: Datenbanken und Vorlagen). Dadurch wird dein Code unnötig lang und fehleranfällig, da du besagte Methoden und Mittel selber schlecht umsetzt, statt auf vorhandenen Code zurückzugreifen.
Es folgt Designkritik zu konkreten Punkten:
#!/usr/bin/perl -w
Dieser Schalter ist überholt. Du benutzt bereits den Ersatz, das lexikalische Pragma warnings. Entferne den Schalter!
Du schreibst ein CGI-Programm, es führt also Code im Auftrag anderer Leute (also der Benutzer deines Webangebots) aus. Schalte Taintchecks mittels -T ein!
###############################################################################
Solche Verzierungen taugen als Illuminierung mittelalterlicher Manuskripte, gehören aber nicht in den Quellcode. Entferne sie! Wenn du Unterteilung von logischen Codeeinheiten sichtbar machen möchtest, benutze Subroutinen und rücke sie ein!
guest-reader.pl: Version 1.00
Dieses Metadatum gehört nicht in einen Programmiererkommentar. Benutze die reservierte Packagevariable $VERSION!
our $VERSION = '1.00';
Developed started in: 02.08.2012
Finished in: 03.08.2012
Das gehört nicht in den Code. Dein Versionskontrollsystem gibt bereits Aufschluss über diese Daten. Entferne sie!
Programmed by: XXXXXXXXXXXXXXX
Dieses Metadatum gehört nicht in einen Programmiererkommentar, sondern in die Dokumentation. Benutze den dafür in Pod vorgesehenen Absatz!
Extra-Skalar-Group (ESG) / Skalare
So was gibt es nicht. Hier fehlt Dokumentation zum Verständnis!
my $DB_nick;
my $DB_nachricht;
my $DB_zaehler;
my $DB_gbrgelesen;
my $DB_IPAdresse;
my $DB_Monatstag;
my $DB_Monat;
my $DB_Jahr;
my $DB_Stunden;
my $DB_Minuten;
my $DB_Sekunden;
Du hast mehrere Skalarvariablen, deren Namen mit dem gleichen Präfix anfangen. Das sollte immer ein Warnsignal sein, dass hier besser eine Verbundvariable taugen würde. Aggregiere deine Datenfelder in einem Hash!
Auch hat Deutsch nichts in Programmen verloren. Schreibe den Code, insbesondere die Bezeichner, auf englisch!
Html-Protokoll
So etwas gibt es nicht. CGI implementiert das Protokoll HTTP. Das ist allgemein bekannt, lasse das weg!
print CGI->header('text/html');
Die Zeichenkodierung fehlt. Die empfohlene Kodierung für das Web ist UTF-8. Halte deine Daten UTF-8 vor und gebe sie so kodiert aus!
Read-Out-Engine / Buchausleser
Hier gibt es keine Engine. Das Wort hat eine spezielle Bedeutung, nicht jeder Code ist eine Engine. Ersetze den Kommentar mit einer benannten Subroutine!
open(LESER, "<XXXXXXX/XXXXXXX.csv") or die "ERROR: Unable to open the Message-file during read-access!";
Das ist unidiomatisch. Verwende immer die Drei-Argumente-Variante von open! Verwende lexikalische Dateihandles! Die Fehlernachricht sagt nicht, warum das Öffnen fehlschlägt. Überlasse autodie die Fehlerprüfung und Erzeugung der Fehlernachricht!
use autodie qw(:all);
open my $messages, '<', 'XXXXXXX/XXXXXXX.csv';
while(! eof(LESER)){
my $zeile = <LESER>;
Das ist unidiomatisch. Iteriere einfach bis zur Abbruchbedingung, dass readline mangels Eingabe fehlschlägt!
while (my $line = <$messages>) {
my ($DB_nick, $DB_nachricht, $DB_zaehler, $DB_gbrgelesen, $DB_IPAdresse, $DB_Monatstag, $DB_Monat, $DB_Jahr, $DB_Stunden, $DB_Minuten, $DB_Sekunden) = split(/;/,$zeile);
Dies gibt vor, ein CSV-Parser zu sein. Es funktioniert aber nicht, weil er Escapingregeln missachtet. Benutze einen korrekten Parser, z.B. Text::CSV!
Mit dem späteren Code wird klar, dass die Daten in tabellarischer Form vorliegen. Das bewährte Mittel zur Datenwiedergabe ist eine ACID-konforme Datenbank. Verzichte auf CSV/manuelles Locking und überlasse das dem DBMS!
$counter = ($counter + 1);
Das ist unidiomatisch. Benutze den Inkrementoperator!
$counter++;
unshift (@Collector, $counter, $DB_nick, $DB_nachricht, $DB_zaehler, $DB_gbrgelesen, $DB_IPAdresse, $DB_Monatstag, $DB_Monat, $DB_Jahr, $DB_Stunden, $DB_Minuten, $DB_Sekunden);
Es ist eine enorme Speicherverschwendung, erst alle Daten in eine riesige Datenstruktur einzusammeln und dann darüber zu iterieren, um die Ausgabe zu befüllen. Ab einer gewissen Menge von Daten wird dieses Programm nicht mehr funktionieren, die Experten sprechen davon, dass es nicht skaliert. Stattdessen befülle die Ausgabe stückchenweise mit jeweils nur einem Datensatz auf einmal!
foreach ( my $i=0; $i<@Collector; $i+=12) {
my ($counter, $DB_nick, $DB_nachricht, $DB_zaehler, $DB_gbrgelesen, $DB_IPAdresse, $DB_Monatstag, $DB_Monat, $DB_Jahr, $DB_Stunden, $DB_Minuten, $DB_Sekunden)=@Collector[ $i .. $i+11 ];
Es befindet sich hier magische Zahlen. Benutze Konstanten! Wenn du dein Datenschema änderst, z.B. eine neue Spalte hinzufügst, so braucht nur die Konstante angepasst werden.
exit;
Das ist überflüssig.
━━━━━━━━━━━━
Es folgt nun das verbesserte Programm unter Einhaltung der derzeit als bewährt erachteten Methoden und Mittel. Ein Großteil der vorher genannten Ratschläge ist hiermit hinfällig.
use utf8;
use strictures;
use CGI qw();
use DateTime::Format::RFC3339 qw();
use DBIx::Class::Schema::Loader qw();
use Template qw();
my $c = CGI->new;
print $c->header('text/html; charset=UTF-8');
my $d = DateTime::Format::RFC3339->new;
my $t = Template->new;
my $template = \<<'TEMPLATE';
<div class="buch_eintrag">
<p class="textvariante_c">([% e.counter %]) Name: [% e.nickname|html %] [Datum: [% e.posted.dmy('.') %] / [% e.posted.hms(':') %] Uhr]
</p>
<p class="textvariante_d">[% e.message|html %].
</p>
</div>
TEMPLATE
DBIx::Class::Schema::Loader->naming('current');
my $gb = DBIx::Class::Schema::Loader->connect->resultset('Guestbook')->search({}, {order_by => {-desc => 'counter'}});
while (my $entry = $gb->next) {
$entry->posted($d->parse_datetime($entry->posted));
$t->process($template, { e => $entry });
}
print <<'FOOTER';
</div>
<img id="absatz_bild" border="0" src="reading_book.png" width="500" height="262" alt="Buchleser">
</div>
</body>
</html>
FOOTER
__END__
=encoding UTF-8
=head1 NAME
guest-reader.pl - display guestbook and page footer
=head1 DESCRIPTION
Displays guestbook entries in reverse order. Displays the HTML page footer.
=head2 Database schema
create table guestbook (
counter serial primary key,
nickname text not null,
message text not null,
gbr_read boolean not null default 'true',
ip_address inet not null,
posted timestamp with time zone not null
);
=head1 ENVIRONMENT
Requires the DSN to be set in order to connect to the database. See
L<DBI/DBI ENVIRONMENT VARIABLES>.
=head1 AUTHOR
XXXXXXXXXXXXXXX
━━━━━━━━━━━━
es wäre nicht optimal strukturiert
Du halst dir mit SSI Ärger auf, die gesamte Struktur leidet darunter. Statt HTML seriell zusammenzustückeln, arbeite mit transkludierten Vorlagen! Moderne Webframeworks haben die Technologien SSI und CGI längst abgelöst. Du hast nur einen Teil deines Webangebots hergezeigt, und ich kann nur das verbessern, was ich sehe.
verstehe besonders die "[]"-Eckklammern am ende nicht
Es ist ein Arrayslice über die Indizes $i bis $i plus 11.