Programm "Waren"

[EdAdvokat]

Ein kleines Übungsprogramm "Waren" zur Disskussion. Es soll lediglich eine einfache Berechnung von Preisen, Rabatt und Warenbestand
dargestellt werden.

[EdAdvokat]

Hallo zusammen, gibt es überhaupt nichts zu meckern an dem Programm? So perfekt kann das doch nicht sein.
Zwar ist zu merken, dass es vermutlich Interesse findet, doch keiner meldet sich zu Wort. Bislang konnte
ich aus den konstruktiven Kritiken immer lernen und das soll ja ein Leben lang so weiter gehen ("man lernt
sein Leben lang niemals aus...") Also was ist zu kritisieren?

[HolgerX]

Hmm..

OK, fangen wir an zu zerlegen

Delphi-Quellcode:

procedure TWaren.init;
begin
  FillChar(FWarenbestand,sizeof(FWarenbestand),0);
  FillChar(FWarenverkauf,sizeof(FWarenVerkauf),0);
  FillChar(FaktBestand,sizeof(FaktBestand),0);
  FillChar(FStPreis,sizeof(FStPreis),0);
  FillChar(FVerkPreis,sizeof(FVerkPreis),0);
  FillChar(FNettopreis,sizeof(FNettoPreis),0);
  FillChar(FMWSt,sizeof(FMWSt),0);
  FillChar(FErtrag,sizeof(FErtrag),0);
  FillChar(FAbMWSt,sizeof(FAbMWSt),0);
end;

Wieso benutzt Du FillChar um Integer oder Float Variablen zu initialisieren?
Sollte dies nicht besser nur für arrays ob Byte/Char verwendet werden?

Auch ist es besser lesbar, wenn Du die Variablen mit dem entsprechenden Wert initialisierts...

Delphi-Quellcode:

procedure TWaren.init;
begin
  FWarenbestand := 0;
  FWarenverkauf := 0;
  FaktBestand   := 0;
  FStPreis      := 0;
  FVerkPreis    := 0.0;
  FNettopreis   := 0.0;
  FMWSt         := 0.0;
  FErtrag       := 0.0;
  FAbMWSt       := 0.0;
end;

Delphi-Quellcode:

var
  Form1: TForm1;
  Waren: TWaren;

Und was mich immer wieder den Kopf schütteln läst: Unsinnige GLOBALE Variablen!

'Waren' wird nur 'innerhalb' der FormularClasse verwendet, wieso dort nicht als FWaren : TWaren unter 'Private' anlegen?
Dann würde an jeder Stelle in den Methoden der Form erkennbar sein, wo es herkommt ('FWaren' = Membervar).

Wieso hat die Classe TWaren kein eigenes Init, bei dem z.B. Warenbe); //const Warenbe
gesetzt wird?

und beim Form1.Init sollten die Werte aus der Classe Waren (nach dessen Init) genommen werden und nicht wieder eigene Konstanten.

Delphi-Quellcode:

procedure TForm1.init;
begin
  FWaren.Init;

  Warenbestand.Text := IntToStr(FWaren.Warenbestand);
  WarenVerkauf.Text:='0';
  aktBestand.Text   := IntToStr(FWaren.aktBestand);
  StPreis.Text      := FloatToStr(FWaren.StPreis);  //const StkPreis
  VerkaufsPreis.text:= FloatToStr(FWaren.Verkaufspreis);
  Nettopreis.Text   := FloatToStr(FWaren.Nettopreis);
  MWSt.Text         := FloatToStr(FWaren.MWSt);
  Ertrag.Text       := FloatToStr(FWaren.Ertrag);
  AbMWSt.Text       := FloatToStr(FWaren.AbMWSt);
  BruttoUmsatz.Text := FloatToStr(FWaren.BruttoUmsatz); // Fehlt

  checkbox1.Checked      :=false;
  checkboxRabatt.Checked :=false;
end;

Somit wird nur an einer Stelle Konstanten ver wendet und diese zur Ansicht gebracht.


So, hoffe ein paar Tips geben zu können..

Mal sehen, ob meine Infos jetzt von anderen zerpflückt werden

[jaenicke]

Das größte Problem ist, dass die Mehrwertsteuerberechnung falsch ist. Die 19% Mehrwertsteuer werden auf den Nettowert draufgerechnet. Das sind nicht 19% des Bruttowerts...

[p80286]

Da ich nur D7 habe muß ich mich auf den Sourcecode beschränken:

Delphi-Quellcode:

   begin
          if Waren.Warenverkauf>=10 then
            checkboxRabatt.Checked:=true;
         if checkboxRabatt.Checked then
            Waren.StPreis:=((Waren.StPreis)-1);
            checkboxRabatt.Checked:=false;
        MessageDlg(format(Preismeldung,[(Waren.Verkaufspreis),(Waren.Nettopreis),(Waren.MWSt)]),mtInformation,[mbOk],0);
        end;

das checkboxRabatt.Checked:=false; gehört auf den ersten Blick noch zu if checkboxRabatt.Checked then was nach genauerem hinschauen Blödsinn ist, aber warum unnötig verkomplizieren?
Warum diese "Checkboxrabatt"-Orgie?
Wenn Du mehr als 10Stk verkaufst gibt es Rabatt egal ob die Checkbox gecheckt ist oder nicht?
Wenn ein Rabatt gewährt werden soll, dann sollte dies die Checkbox ändern. Es könnte sein, daß die Rabattgewährung nicht möglich ist. Das weiß aber Waren daher sollte Waren den Status der Checkbox setzen.

Delphi-Quellcode:

        if (Waren.aktBestand)<0 then
         begin
           showMessage(mehrWaren);
           checkbox1.Checked:=true;exit;
         end;

Showmessage soll informieren, daß es nicht mehr genug Waren gibt. Warum überprüft die Oberfläche dies und nicht die Klasse Waren?
Und wer ist checkbox1 Gruß
K-H

[Luckie]

Ich glaube, du begehst die gleichen Fehler wieder, wie bei deinem anderen Programm.

Aber Hut ab für deinen Mut so was hier vor zu stellen. Aber nur so lernt man, aus Fehlern. Weiter so.

[haentschman]

Moin...

Zitat:

Aber Hut ab für deinen Mut so was hier vor zu stellen. Aber nur so lernt man, aus Fehlern. Weiter so.

...so sehe ich das auch. Alle haben mal angefangen...bei den meisten war Eisenhower noch President. Du mußt nur darauf achten, das du alte Fehler nicht wieder einbaust.

Die Tage schaue ich auch mal rein.

[EdAdvokat]

Hallo zusammen, Danke für Eure Anregungen. Da ich zurückliegend durch eure Anregungen mehr gelernt habe als aus den schlauen Delphibüchern wage ich es, hoffentlich ohne aufdringlich zu wirken, immer mal wieder ein Programmchen hier vorzustellen und den "Spezis" aus Eisenhowers Zeiten Futter für Kritiken zu geben. Die alten Fehler(globale Variable, sinnentlehrte Checkboxen)lassen sich erkenntlich nicht so leicht abstellen - doch ich arbeite daran. Leider wird in den Büchern auf die Frage des Setzens von globalen oder lokalen Variablen nicht so der Schwerpunkt gesetzt und mal schnell eine globale gewählt. So habe ich das kritiklos akzeptiert.
Als Truman regierte erblickte ich das Licht der Welt, doch bis zu Turbo Pascal und Delphi vergingen noch ein paar Jahre und die Programmiererei ist nur mein Hobby, denn ich habe in meinem zurückliegenden Berufsleben eher Paragraphen verbogen als Bits und Bytes gestreichelt, so mag man mir diesen oder jenen Fehler verzeihen.
Also P80286 (er lebt vermutlich nach Eisenhowers Regentschaft in der Zeit der 286-iger PC's): Und wer ist checkbox1 ? - das ist ein Opfer der checkbox-Orgie und es gibt ihn so nicht mehr.
Danke Sebastian für den Hinweis auf die MWSt. Das ist mir peinlich, doch ich habe es hoffentlich verbessert, denn nun wird Bruttopreis, Nettopreis und MWSt so angezeigt, wie es die verbindliche
Berechnung verlangt.
HolgerX, auch Dir vielen Dank und ich hoffe Deine Anregungen umgesetzt zu haben. Das Einfügen der globalen Variablen in OOP unter Form1:TForm1 dann Waren:TWaren zu setzen geschah fast automatisch, weil die das alle so gemacht haben in meinen Büchern. Man soll tatsächlich alles hinterfragen und Eure Argumentation ist diesbezüglich überzeugend.
So nun also eine verbesserte Version

[MrSpock]

Kann der Stückpreis nur in ganzen Euro-Beträgen festgelegt werden?

[p80286]

Gut zu wissen wer auf der "anderen" Seite sitzt, willkommen bei den alten Säcken, als Kennedy in Dallas war, habe ich die Grundrechenarten gelernt.

Delphi-Quellcode:

      if (FWaren.aktBestand)>=0 thenif checkboxverfuegbar.Checked then
    begin
      showMessage(Warenbest);
    exit  end
    else if not checkboxverfuegbar.Checked then

        begin
          if FWaren.Warenverkauf>=10 then
            FWaren.StPreis:=((FWaren.StPreis)-1);
            MessageDlg(format(Preismeldung,[(FWaren.Verkaufspreis),(FWaren.Nettopreis),(FWaren.MWSt)]),mtInformation,[mbOk],0);
        end;
        if (FWaren.aktBestand)<0 then
         begin
           checkboxverfuegbar.Checked:=true;exit;
         end;

Dieses Exit; end mag ein gewollter Stolperstein sein, aber

Delphi-Quellcode:

begin
  //irgendwas
  Exit;
end

liest sich doch besser.

Fällt Dir hier etwas auf:

Delphi-Quellcode:

if checkboxverfuegbar.Checked then
    .......
    else if not checkboxverfuegbar.Checked then

(falls wider Erwarten nicht, else ist schon das gleiche wie not checkboxverfuegbar.Checked )

Gruß
K-H