Autor Beitrag
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: So 22.05.22 09:25 
Hallo Th69,

Vielen Dank für deine Antwort.

Zitat:
Bei deinen String-Arrays fehlt noch die Erzeugung des Arrays (mittels new[...]).

So ich habe jetzt eine neue Variante mit new[...]. Ich werde die Dateien wieder mit anhängen.

Zitat:
Für einfache Datenhaltungsklassen, wie eben UIValues, solltest du die automatischen Eigenschaften ({ get; set; }) benutzen

Ok habe ich angepasst. Vielen Dank für deine Hilfe.

Zitat:
Und dann in der Zuweisungsmethode

Wenn ich das besser mit TryParse machen sollte dann muss ich alle einzelne Zuweisungen die ich in Connect_Click habe in eine If Schleife packen???

ausblenden volle Höhe C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
28:
29:
30:
31:
private void Connect_Click(object sender, EventArgs e)
{
    uivalues.Error = txtError.Text;

    uivalues.IPNr = txtIPNr.Text;
    uivalues.RackNr = int.Parse(txtRackNr.Text);
    uivalues.SlotNr = int.Parse(txtSlotNr.Text);

    uivalues.DBNr = int.Parse(txtDBNr.Text);
    uivalues.DBOffset = int.Parse(txtDBOffsetNr.Text);
    uivalues.DBSize = int.Parse(txtDBSizeNr.Text);

    uivalues.PraegeCodeDBPos = int.Parse(txtPraegeCodeDBPos.Text);
    uivalues.FolderNameDBPos = int.Parse(txtFolderNameDBPos.Text);
    uivalues.PraegeCode2DBPos = int.Parse(txtPraegeCode2DBPos.Text);

    uivalues.TrigSaveFilesByte = int.Parse(txtTrigSaveFilesByte.Text);
    uivalues.TrigSaveFilesBit = int.Parse(txtTrigSaveFilesBit.Text);
    uivalues.TypLinksByte = int.Parse(txtTypLinksByte.Text);
    uivalues.TypLinksBit = int.Parse(txtTypLinksBit.Text);
    uivalues.TyprechtsByte = int.Parse(txtTypRechtsByte.Text);
    uivalues.TyprechtsBit = int.Parse(txtTypRechtsBit.Text);
    uivalues.RoboterDSLeerByte = int.Parse(txtRoboterDSLeerByte.Text);
    uivalues.RoboterDSLeerBit = int.Parse(txtRoboterDSLeerBit.Text);
    uivalues.FMBauteilByte = int.Parse(txtFMBauteilByte.Text);
    uivalues.FMBauteilBit = int.Parse(txtFMBauteilBit.Text);
    uivalues.HeartByte = int.Parse(txtHeartByte.Text);
    uivalues.HeartBit = int.Parse(txtHeartBit.Text);

    for (int i = 0; i < uivalues.SourceFilePathLocalApps.Length; i++)
        uivalues.SourceFilePathLocalApps[i] = tbSourceFilePathLocalApps[i].Text;

Das wird aber den Code in dieser Methode ganz schön vergrößern.
Wäre es nicht besser ich lagere das in eine Methode PassAllUIVariables() aus und rufe die dann in Connect_Click auf? Oder wie sollte ich es deiner Meinung nach machen?
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
if (int.TryParse(txtDBNr.Text, out int dbnr))
{
  uivalues.DBNr = dbnr;
}
else
{
  // Fehlermeldung ausgeben
}

Ich habe den Code getestet und er macht auch was er soll aber mit Nebeneffekten.

Meine Vermutung:
Am Ende von Connect_Click schalte ich den Timer EventLoop ein.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
else
{
    MessageBox.Show("Es müssen alle Felder ausgefüllt werden");
}

EnableTextBoxes(false);

ScanDBOffset.Enabled = true;
EventLoop.Enabled = true;


Im EventLoop_Tick schalte ich den Timer aus. Wenn in die Schleifen reingesprungen wird ist es gut und der Timer bleibt aus.
Wenn nicht wird über den else Zweig der Timer wieder eingeschaltet.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
        private void EventLoop_Tick(object sender, EventArgs e)
        {
            EventLoop.Enabled = false;

            if (BW_Event.IsBusy != true)
            {

                S7VariablenResult s7VariablenResult = LeakResult();

                if (s7VariablenResult.TrigSaveFiles && (s7VariablenResult.TypLinks || s7VariablenResult.TypRechts) && !s7VariablenResult.FMBauteil)
                {
                    BW_Event.RunWorkerAsync();                    
                }
                else
                {
                    if (EventLoop.Enabled == true)
                    {
                        return;
                    }
                    else
                    {
                        EventLoop.Enabled = true;
                    }

                }
            }
        }


Sollte der Timer aus bleiben, weil ein Kopiervorgang gestartet ist, bin ich nicht sicher wo ich den Timer nach dem kopieren wieder einschalten muss.
Der Timer wird in der BW_Event_ProgressChanged wieder eingeschalte. Ist das gut?
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
private void BW_Event_ProgressChanged(object sender, ProgressChangedEventArgs e)
{
    S7VariablenResult s7VariablenResult = LeakResult();

    lblPraegeCode.Text = s7VariablenResult.PraegeCodeNr; // It’s already a string
    lblFolderName.Text = s7VariablenResult.FolderName;
    lblPraegeCode2.Text = s7VariablenResult.PraegeCode2Nr;

    EventLoop.Enabled = true;
}


Noch eine andere Vermutung ist das die beiden Timer daran Schuld sind.
Beide Timer rufen die S7VariablenResult s7VariablenResult = LeakResult() auf wäre das möglich?

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
private void ScanDBOffset_Tick(object sender, EventArgs e)
{
    ScanDBOffset.Enabled = false;
######################################################################
    S7VariablenResult s7VariablenResult = LeakResult();
######################################################################
}


ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
private void EventLoop_Tick(object sender, EventArgs e)
{
    EventLoop.Enabled = false;

    if (BW_Event.IsBusy != true)
    {
#######################################################################
        S7VariablenResult s7VariablenResult = LeakResult();
#######################################################################
    }
}


So und dann zuletzt haben wir noch das sporadische anzeigen von einer Fehlermeldung.
Ich denke das auch der Timer daran Schuld ist.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
for (int i = 0; i < cbActivateLocalApps.Length; i++)
{
    if (cbActivateLocalApps[i].Checked == true)
    {
        string praegeCodeNrFolderName = s7VariablenResult.FolderName;
        string praegeCodeNrFileName = s7VariablenResult.PraegeCodeNr;
        //string praegeCode2NrFileName = s7VariablenResult.PraegeCode2Nr;
        string sourceFilePath = uivalues.SourceFilePathLocalApps[i];
        string targetPath = DEFAULTTARGETPATH;

        MoveFilesfromLocal(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
    }
}


Ich habe mir die Mühe gemacht und habe jede For-Schleife einzeln getestet um den Fehler einzugrenzen.
Da bekomme ich keine Fehlermeldung erst ab 2 For-Schleifen beginnt es und dann auch nicht immer.

Die Fehlermeldung lautet: Eine Datei kann nicht erstellt werden, wenn sie bereits vorhanden ist.
Ich kann das halt nicht nachvollziehen, weil ja alle Dateien kopiert und verschoben werden.

Könntest du dir das bitte mal anschauen? Vielen Dank nochmal für die bisherige Hilfe.

Grüße Tom

Moderiert von user profile iconTh69: C#-Tags hinzugefügt
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: So 22.05.22 09:30 
Hallo,

Ich habe die Dateien vergessen.

Grüße Tom
Einloggen, um Attachments anzusehen!
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: So 22.05.22 11:59 
Hallo,

Kleines Update meinerseits. Ich habe in Google mal nach Timer mit BackgroundWorker gesucht.
Und bin darauf gestoßen das ich den RunWorkerCompleted nutzen sollte um den Timer zum Schluß wieder zu reaktivieren.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
private void BW_Event_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
{
    Ready();

    EventLoop.Enabled = true;
}


Die Ready Methode im ProgressChanged war glaube ich auch falsch und habe sie auch verschoben.
Es sah ganz gut aus und ohne Fehler aber es war auch erst ein Versuch.

Was denkt Ihr ist das gut so?

Vielen Danke

Grüße Tom

Moderiert von user profile iconTh69: C#-Tags hinzugefügt
Einloggen, um Attachments anzusehen!
Ralf Jansen
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starhalf ontopic star
Beiträge: 4700
Erhaltene Danke: 991


VS2010 Pro, VS2012 Pro, VS2013 Pro, VS2015 Pro, Delphi 7 Pro
BeitragVerfasst: So 22.05.22 12:52 
RunWorkerCompleted ist hilfreich. Insbesondere wenn man an Fehlerhandling denkt. In den EventArgs steckt z.b. die Exception drin wenn DoWork durch eine abgebrochen wurde.
Im Moment hast du ja für den Fehlerfall eine MessageBox und die würde alles dauerhaft stoppen bis die einer wegklickt. Üblicherweise nicht das was man bei einem Background Thread will.

Ansonsten solltest du so jetzt den Tick Event vereinfachen können.
Etwa

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
private void EventLoop_Tick(object sender, EventArgs e)
{
    S7VariablenResult s7VariablenResult = LeakResult();

    if (s7VariablenResult.TrigSaveFiles && (s7VariablenResult.TypLinks || s7VariablenResult.TypRechts) && !s7VariablenResult.FMBauteil)
    {
        EventLoop.Enabled = false;
        BW_Event.RunWorkerAsync();
    }
}
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: So 22.05.22 13:25 
UserTom hat folgendes geschrieben:
Wäre es nicht besser ich lagere das in eine Methode PassAllUIVariables() aus und rufe die dann in Connect_Click auf? Oder wie sollte ich es deiner Meinung nach machen?

Ja, das wäre wohl gut.
Und laß ersteinmal die Parse-Aufrufe drin (damit du nicht noch mehr Baustellen aufmachst). Dann baue aber wenigstens einen try...catch-Block um den gesamten Methoden-Aufruf und gebe dem Anwender eine Fehlermeldung (à la "Bitte nur Zahlenwerte eingeben") aus.

Und bgzl. der Timer: du solltest auf jeden Fall sicherstellen, daß jeder Timer nur seine eigenen Aufgaben durchführt und nicht beide dasselbe ausführen.

Auch das mit dem RunWorkerCompleted sieht jetzt sinnvoller aus.

Ansonsten hilft hier nur, viel Testen und debuggen bzw. loggen.
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: So 22.05.22 23:34 
Servus,

Vielen Dank für Eure Antworten.

@ Ralf

Zitat:
Insbesondere wenn man an Fehlerhandling denkt.


Ist die Fehlerbehandlung so richtig?
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
private void BW_Event_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
        {
            if (e.Cancelled == true)
            {
                lblEventResult.Text = "Canceled!";
            }
            else if (e.Error != null)
            {
                lblEventResult.Text = "Error: " + e.Error.Message;
            }
            else
            {
                lblEventResult.Text = "Done";
            }

            Ready();

            EventLoop.Enabled = true;
        }


Zitat:
Im Moment hast du ja für den Fehlerfall eine MessageBox

Die Messagebox habe ich gegen eine Klassenmethode SaveErrorMessage ausgetauscht.
Damit werden Logfiles erstellt. Habe ich aus dem I-Net.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
using System;
using System.IO;
using System.Windows.Forms;

namespace PDE
{
    internal static class ErrorExtensions
    {
        //[DebuggerStepThrough()]
        public static void SaveErrorMessage(this Exception ex)
        {
            var dir = Path.Combine(Application.StartupPath, "ErrorLogging");
            var di = new DirectoryInfo(dir);
            if (!di.Exists)
                di.Create();
            string dateTime = DateTime.Now.ToString("dd.MM.yy");
            FileInfo fi = new FileInfo(Path.Combine(dir, string.Concat("Error-file-from_", dateTime, ".log"))); ;
            //FileInfo fi = new FileInfo(Path.Combine(dir, string.Concat("Error-", DateTime.Now.ToFileTime().ToString(), ".log")));
            using (StreamWriter swLogFile = new StreamWriter(fi.FullName))
            {
                swLogFile.Write(ex.ToString());
            }
        }
    }
}


Zitat:
Ansonsten solltest du so jetzt den Tick Event vereinfachen können.

Du hast IsBusy weggelasen war das absicht? Soll ich die if Abfrage drin lassen?

@ Th69

Zitat:
Dann baue aber wenigstens einen try...catch-Block


Da ich die NumericUpDown Elemente optisch nicht mag habe ich mir überlegt wie ich verhindern kann das man nur Zahlen eingeben kann.
Mit dem KeyPressEvent bin ich mir sicher das man nichts falsches eingeben kann.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
private void OnlyNumber_KeyPress(object sender, KeyPressEventArgs e)
{
    //Prüfung, ob eine Zahl oder BackSpace ausgelöst wurde
    if ("1234567890\b".IndexOf(e.KeyChar.ToString()) < 0)
    {
        //Bei Abweichung Ereignis verwerfen
        e.Handled = true;
    }
}


Zitat:
Und bgzl. der Timer:


Beide Timer rufen die folgende Methode auf
ausblenden C#-Quelltext
1:
S7VariablenResult s7VariablenResult = LeakResult();					


ScanDBOffset_Tick ist für die UI-Elemente der SPS Variablen zuständig.
EventLoop_Tick ist für die IO Operationen zuständig.
Beide müssen Variablen aus dieser Methode abrufen. Ich denke bei 100ms wird das sehr schnell passieren
das die Timer gleichzeitig auf diese Methode zugreifen.

Ich sollte eine 2. s7VariablenResult (s7UIVariablenResult) erstellen nur für die UI-Elemente. Würde das was bringen?
Oder stören die sich gar nicht? Wie ist deine Meinung dazu.

Vielen Dank nochmal für Eure Antworten.

Grüße Tom
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Mo 23.05.22 08:42 
UserTom hat folgendes geschrieben:
Mit dem KeyPressEvent bin ich mir sicher das man nichts falsches eingeben kann.

Doch, nämlich per Copy&Paste (Strg+V bzw. über das Kontextmenü) (wie auch bei meiner NumericTextBox)!

Und bzgl. s7VariablenResult:

Soll bzw. darf der Anwender denn die Werte in den TextBoxen ändern, während die beiden Timer laufen? Oder wäre es nicht besser, sie würden nur beim Start der Timer einmalig ausgelesen?

Denn bisher liest du ja jetzt die TextBox-Werte einmalig beim Connect_Click (bzw. in der ausgelagerten Methode) und in LeakResult (dort solltest du nur das Auslesen der S7-Werte durchführen - die TextBox-Werte stehen ja jetzt in UIValues).

Ist denn S7VariablenResult eine von dir erstellte Struktur? Dann könntest du diese auf die Rückgabewerte beschränken (da die Eingabewerte ja in UIValues jetzt stehen).
Die UIValues-Klasse hatte ich eigentlich nur für die Folder-Namen und CheckBoxen (für die Kopierroutinen gedacht), nicht für die S7-Werte. Vllt. solltest du dann zwei getrennte S7-Klassen (für Eingabe und Ausgabe) erzeugen?
Und die S7-Eingabewerte (IPNr, ... , HeartBit) nur direkt vor dem Starten der Timer aus den TextBoxen auslesen?

Wie du siehst, ist Datenmodellierung hier ersteinmal wichtig, um danach dann an den richtigen Stellen die Daten zu lesen und/oder zu schreiben.

PS: In Zeile 90 ff. kannst du auch einfach jetzt die neue Abfrage-Methode aufrufen: if (!AreAllTextBoxesFilled())
Ralf Jansen
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starhalf ontopic star
Beiträge: 4700
Erhaltene Danke: 991


VS2010 Pro, VS2012 Pro, VS2013 Pro, VS2015 Pro, Delphi 7 Pro
BeitragVerfasst: Mo 23.05.22 12:14 
Zitat:
Ist die Fehlerbehandlung so richtig?

Besser
Zitat:
Die Messagebox habe ich gegen eine Klassenmethode SaveErrorMessage ausgetauscht.
Damit werden Logfiles erstellt. Habe ich aus dem I-Net.

Bisher war im catch Handler nur die MessageBox und sonst nix. Heißt der Code danach würde ausgeführt werden was vermutlich immer falsch wenn etwas DoWork knallt. Vermutlich gehört da noch ein throw; rein.
Zitat:
Du hast IsBusy weggelasen war das absicht? Soll ich die if Abfrage drin lassen?

Absicht. Du hast dir einen so genannten One-Shot Timer gebaut. Da du den Timer abschaltest während der Backgroundworker läuft und erst danach wieder anschaltest kann Timer tick nie feuern während der Background Worker läuft. Der Test ist also überflüssig(auch wenn er nicht schaden würde).
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mo 23.05.22 23:43 
Servus,

@ Th69

Zitat:
Doch, nämlich per Copy&Paste

Ok muss ich schauen was ich da machen werde. Wenn ich den Ablauf mit dem kopieren sicher habe kann ich mich um schöner Wohnen kümmern.

Zitat:
Soll bzw. darf der Anwender denn die Werte in den TextBoxen ändern.

Es wird einmalig von mir eingestellt und nachdem man auf Connect geklickt hat sind alle Eingabefelder gesperrt.
Wenn die Timer laufen kann man nichts verändern. Es sei den jemand logt sich über die Login Form ein und entsperrt die Felder.

Zitat:
Oder wäre es nicht besser, sie würden nur beim Start der Timer einmalig ausgelesen?

Darüber habe ich auch nachgedacht. Vielen Dank für deinen Hinweis. Ich habe jetzt zu Testzwecke einen Timer deaktiviert.
Also mit dem Connect_Click lese ich die Textboxen ein. Das passt für mich da ja einmal lesen reicht. Wenn es mal fertig
wird soll dieses kleine Tool ja vom Prinzip her immer laufen solange wie diese Anlage produziert.

Zitat:
und in LeakResult (dort solltest du nur das Auslesen der S7-Werte durchführen


Meinst du so???
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
private S7VariablenResult LeakResult()
{
    S7VariablenResult s7VariablenResult = new S7VariablenResult();
    // Reads the buffer.            
    S7VariablenResult s7VariablenResult = new S7VariablenResult();
    // Reads the buffer.

    byte[] dbbuffersize = new byte[uivalues.DBSize];

    s7Client.DBRead(uivalues.DBNr, uivalues.DBOffset, uivalues.DBSize, dbbuffersize);

    s7VariablenResult.PraegeCodeNr = S7.GetStringAt(dbbuffersize, uivalues.PraegeCodeDBPos);
    s7VariablenResult.FolderName = S7.GetStringAt(dbbuffersize, uivalues.FolderNameDBPos);
    s7VariablenResult.PraegeCode2Nr = S7.GetStringAt(dbbuffersize, uivalues.PraegeCode2DBPos);
    s7VariablenResult.TrigSaveFiles = S7.GetBitAt(dbbuffersize, uivalues.TrigSaveFilesByte, uivalues.TrigSaveFilesBit);
    s7VariablenResult.TypLinks = S7.GetBitAt(dbbuffersize, uivalues.TypLinksByte, uivalues.TypLinksBit);
    s7VariablenResult.TypRechts = S7.GetBitAt(dbbuffersize, uivalues.TyprechtsByte, uivalues.TyprechtsBit);
    s7VariablenResult.RoboterDSLeer = S7.GetBitAt(dbbuffersize, uivalues.RoboterDSLeerByte, uivalues.RoboterDSLeerBit);
    s7VariablenResult.FMBauteil = S7.GetBitAt(dbbuffersize, uivalues.FMBauteilByte, uivalues.FMBauteilBit);
    s7VariablenResult.HeartBeat = S7.GetBitAt(dbbuffersize, uivalues.HeartByte, uivalues.HeartBit);

    return s7VariablenResult;


Das habe ich probiert. Da bekomme ich dann Probleme mit dem kopieren. Es werden keine Datei kopiert in CopyFilesfromNetwork.
Das verschieben mit Move funktioniert. Wenn ich wieder auf die orginale s7VariablenResult zurück gehe dann wird wieder alles kopiert.

Zitat:
Ist denn S7VariablenResult eine von dir erstellte Struktur?

Nein von einer Vorlage.

Zitat:
Dann könntest du diese auf die Rückgabewerte beschränken

Das verstehe ich nicht. Kannst du das besser erklären.

Zitat:
PS: In Zeile 90 ff. kannst du auch einfach jetzt die neue Abfrage-Methode aufrufen: if (!AreAllTextBoxesFilled())

Ich habe diese Abfrage gelöscht, weil sie meiner Meinung nach überflüssig ist.

@ Ralf

Vermutlich gehört da noch ein throw; rein. Diese throw kannst du mir den genauer erklären?

Vielen Dank nochmal.

Grüße Tom

Moderiert von user profile iconTh69: C#-Tags hinzugefügt
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Di 24.05.22 09:07 
UserTom hat folgendes geschrieben:
Es wird einmalig von mir eingestellt und nachdem man auf Connect geklickt hat sind alle Eingabefelder gesperrt.

Dann ist es ja jetzt gut so, daß nicht jedesmal die (gleichen) Werte wieder und wieder per Parse ausgelesen werden.

UserTom hat folgendes geschrieben:
Meinst du so???

Ja, bzw. fast so (bis auf doppelte Zeilen 5+6).

UserTom hat folgendes geschrieben:
Das habe ich probiert. Da bekomme ich dann Probleme mit dem kopieren. Es werden keine Datei kopiert in CopyFilesfromNetwork.

Das muß dann ein anderer Fehler sein (evtl. hast du vergessen eine bestimmte Variable zu setzen, die du bisher in LeakResult ausgelesen hast). Am besten mal mit dem Debugger (Stichwort: Haltepunkt/breakpoint setzen) dann da durchgehen (s. z.B. [Artikel] Debugger: Wie verwende ich den von Visual Studio?).

UserTom hat folgendes geschrieben:
Nein von einer Vorlage.

Wenn du diese nicht ändern kannst/darfst, dann mußt du eben klar im Code unterscheiden, welche Daten davon Eingabe und welche Ausgabe sind (oder du verzichtest auf diese Klasse und erstellst dir selber 2 getrennte Klassen - ist aber jetzt ersteinmal nicht zwingend).

UserTom hat folgendes geschrieben:
Das verstehe ich nicht. Kannst du das besser erklären.

Die gerade beschriebene Unterteilung bzgl. Eingabe und Ausgabe, so daß LeakResult (bzw. besser wohl ReadResults benannt) nur die Eigenschaften der Ausgabe-Klasse setzt (anstatt nur bestimmte Eigenschaften der Riesenklasse S7VariablenResult).

UserTom hat folgendes geschrieben:
Diese throw kannst du mir den genauer erklären?

throw wirft die gerade ausgelöste Exception im catch-Handler weiter, so daß dieser Fehler-Status dann über RunWorkerCompletedEventArgs in RunWorkerCompleted abgefragt werden kann (dies würde aber DoWork, d.h. den Kopiervorgang abbrechen), s.a. throw (C#-Referenz).


Zuletzt bearbeitet von Th69 am So 29.05.22 09:34, insgesamt 1-mal bearbeitet
Ralf Jansen
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starhalf ontopic star
Beiträge: 4700
Erhaltene Danke: 991


VS2010 Pro, VS2012 Pro, VS2013 Pro, VS2015 Pro, Delphi 7 Pro
BeitragVerfasst: Di 24.05.22 11:55 
Zitat:
Diese throw kannst du mir den genauer erklären?


Hat user profile iconTh69 schon. Dein erster try..catch z.b. ist wenn das anlegen des ZielOrdners schief geht. Im Moment scheint es so zu sein das du den Fehler anzeigst/logst aber dann weiter machst mit dem kopieren der Files. Dein Zielordner konnte aber nicht angelegt werden. Da macht weitermachen keinen Sinn und du solltest da irgendwie abbrechen. Eine Lösung ist da halt dann ein throw; am Ende des Catch Blocks einzufügen damit die Exception weitergeworfen wird um irgendwo anders dann behandelt zu werden(z.b. in WorkerCompleted wo dann im Zweifel nicht mehr viel passieren muß)
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: So 29.05.22 09:04 
Servus,

Vielen Dank für Eure Antworten.

Zitat:
Das muß dann ein anderer Fehler sein.

Es lag an einer Variable. Jetzt funktioniert es.

Zitat:
Wenn du diese nicht ändern kannst/darfst,

Doch, die kann man ändern. Aber das brauche ich nicht mehr, da ich mich dazu entschlossen habe nur noch mit einem Timer zu arbeiten. Ich habe es auch in ReadResult umgetauft.

An Euch beide vielen Dank für die Erklärung über throw.
Zitat:
Dein erster try..catch z.b. ist wenn das anlegen des ZielOrdners schief geht.

Zitat:
(dies würde aber DoWork, d.h. den Kopiervorgang abbrechen)

Eine Frage hier zu. Könnte man über CancellationPending verhindern das wenn der Ordner nicht erstellt wird das man bevor das kopieren beginnt rausspringt? So als hätte man einen Cancel Button.
Wenn ja wie müsste ich das umbauen.
Es ist eines klar, wenn das Ordner erstellen nicht funktionieren sollte bin ich mir fast sicher, das dass Problem von der SPS kommt. Der Pfad "D:\S7_Export\AppData" ist ein Freigegebener Ordner der ohne Adminrechte nicht gelöscht werden kann. Ich hätte an der Stelle, wenn es funktionieren würde, die Möglichkeit an die SPS ein Signal zu schicken damit die Anlage stoppt. Fehler analysieren, Problem beheben usw...


An dieser Stelle würde ich gerne Euch nochmal auf 2 Sachen ansprechen die ihr erwähnt hattet.

@TH69
Zitat:
Eigentlich würde ich dafür sogar empfehlen, den gesamten BackgroundWorker-Code in eine eigene Klasse auszulagern

Würdest du mir das erklären wie das in meinem Fall aussehen würde? Vorausgesetzt mein Aufbau mit dem BackgroundWorker ist in Ordnung.

@ Ralf
Zitat:
Du hast dir einen so genannten One-Shot Timer gebaut.

Ist das falsch oder unnötig? Ist es besser wenn Timer tick feuert? Mir gefällt es gar nicht.
Ich dachte immer ich muss ihn ausschalten während das kopieren läuft, weil ich das aus einem Beispielprojekt von einer anderen Internetseite so übernommen habe.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
 if (s7VariablenResult.HeartBeat)
 {
     lblHeartbeat.BackColor = Color.Green;
 }
 else
 {
     lblHeartbeat.BackColor = Color.LightGray;
 }


Vor allem, weil mein Heartbeat deswegen nicht richtig funktioniert. Es sieht blöd aus wenn es aufhört zu blinken und nach dem kopieren blinkt es weiter. Wie wäre es besser umzusetzen?

Vielen Dank nochmal für Eure tolle Unterstützung.

Grüße Tom

Moderiert von user profile iconTh69: Beitragsformatierung überarbeitet.
Einloggen, um Attachments anzusehen!
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: So 29.05.22 10:25 
Guten Morgen,

ich hatte ja noch etwas mehr geschrieben:
Th69 hat folgendes geschrieben:
Eigentlich würde ich dafür sogar empfehlen, den gesamten BackgroundWorker-Code in eine eigene Klasse auszulagern, weil dann würde man zuerst Fehlermeldungen bei der Verwendung der Form-Controls erhalten (welche man dann sukzessive ersetzen würde), aber das mute ich dir jetzt (noch) nicht zu...

Da ging es mir in erster Linie darum, alle Zugriffe auf die Controls aus dem BackgroundWorker-Code zu eliminieren (und bei Auslagerung wären die Controls in der neuen Klasse ja nicht bekannt und würden ersteinmal Compilerfehler verursachen).

Es stimmt aber, daß man sich über die Auslagerung von Code beim Entwickeln Gedanken machen sollte ("Trennung von Zuständigkeiten").
Bis jetzt hat deine Klasse ja 3 Hauptaufgaben:
  • Login/Connect sowie weitere UI-Aktionen ("Select Folder")
  • Settings lesen/schreiben
  • BackgroundWorker mit Kopiervorgang

Nach der bisherigen Umstellung deines Codes solltest du wenigstens die reinen IO-Operationen (MoveFilesfromLocal, ..., CopyFilesfromNetwork) auslagern (da du dort bisher auch noch Zugriffe auf UI-Elemente machst, müßtest du nur den reinen IO-Code der Methoden auslagern):
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
... = Directory.EnumerateDirectories(...);
foreach (string dir in dirs)
{
  //...
}

Und die jeweilige äußere Schleife in den Form-Methoden lassen:
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
for (...)
  if (cbNetSubFolderUses[j].Checked)
  {
     IOOperations.MoveFilesfromLocal(...);
  }

M.E. kannst du die Klasse IOOperations (oder wie auch immer du sie nennst) ersteinmal static machen.

Du kannst sogar das Exception-Handling so umschreiben, daß der Code nur genau an einer Stelle steht (anstatt immer wieder in jeder Methode), indem du ein Delegate (wie z.B. Action) benutzt:
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
void TryAction(Action action)
{
  try
  {
    action();  
  }
  catch (DirectoryNotFoundException ex)
  {
    ex.SaveErrorMessage();
  }
  catch (UnauthorizedAccessException ex)
  {
    ex.SaveErrorMessage();
  }
}

(zu nahezu jeder Codeverdopplung gibt es [elegante] Wege zur Vermeidung davon.)

Und der Aufruf sieht dann z.B. einfach so aus:
ausblenden C#-Quelltext
1:
TryAction(() => File.Move(filename, $"{Path.Combine(targetPath, praegeCodeNrFolderName, Path.GetFileName(file)}"));					

(hier jetzt [kurz] als Lambda-Ausdruck, aber es gibt auch noch andere Möglichkeiten).

Wenn du bisher noch nicht mit (eigenen) Delegates gearbeitet hast: Delegaten (C#-Programmierhandbuch) - oder du suchst dir weiteres Material dazu im Internet.

PS: Auch Code wie z.B.
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
if (s7VariablenResult.TrigSaveFiles)
{
  lblSaveFiles.BackColor = Color.Green;
}
else
{
  lblSaveFiles.BackColor = Color.LightGray;
}
läßt sich einfach mit einer Hilfsmethode kürzer schreiben (Aufgabe für dich ;-)).

Und gerade auch noch in deinem Code entdeckt:
ausblenden C#-Quelltext
1:
2:
3:
4:
if (!chkActivateLocalApp1.Checked
    && !chkActivateLocalApp2.Checked
    && //...
    && !chkActivateLocalApp8.Checked

(auch das sollte eine Schleife [als eigene Methode] sein, bzw. kurz mit LINQ.)
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: So 29.05.22 19:46 
Guten Abend,

Danke für deine Antwort und die Menge an Hilfe.

Zitat:
Nach der bisherigen Umstellung deines Codes solltest du wenigstens die reinen IO-Operationen (MoveFilesfromLocal, ..., CopyFilesfromNetwork) auslagern

Da hätte ich gedacht das ich das nicht umsetzen kann.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
private IOFileOperations ioFileOperations = new IOFileOperations();

public MainForm()
{
    InitializeComponent();


Die Klasse. Ich habe es aber nicht statisch hinbekommen.
ausblenden volle Höhe C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
41:
42:
43:
44:
45:
46:
47:
48:
49:
50:
51:
52:
53:
54:
55:
56:
57:
58:
59:
60:
61:
62:
63:
64:
65:
66:
67:
68:
69:
70:
71:
72:
73:
74:
75:
76:
77:
78:
79:
80:
81:
82:
83:
84:
85:
86:
87:
88:
89:
namespace PDE
{
    class IOFileOperations
    {
        void TryAction(Action action)
        {
            try
            {
                action();
            }
            catch (DirectoryNotFoundException ex)
            {
                ex.SaveErrorMessage();
            }
            //catch (UnauthorizedAccessException ex)
            //{
            //    ex.SaveErrorMessage();
            //}
        }

        public void MoveDirectoryfromLocal(string praegeCodeNrFolderName, string praegeCodeNrFileName, string sourceFilePath, string targetPath)
        {
            var iOFileOperations = new IOFileOperations();

            IEnumerable<string> dirs = Directory.EnumerateDirectories(sourceFilePath, "*", System.IO.SearchOption.AllDirectories).Where(x => x.Contains(praegeCodeNrFileName));

            foreach (string dir in dirs)
            {
                iOFileOperations.TryAction(() => FileSystem.MoveDirectory(dir, dir.Replace(sourceFilePath, Path.Combine(targetPath, praegeCodeNrFolderName)), true));
            }
        }

        public void MoveFilesfromLocal(string praegeCodeNrFolderName, string praegeCodeNrFileName, string sourceFilePath, string targetPath)
        {
            var iOFileOperations = new IOFileOperations();

            IEnumerable<string> files = Directory.EnumerateFiles(sourceFilePath, "*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(praegeCodeNrFileName));
            foreach (string file in files)
            {
                FileInfo fi = new FileInfo(file);
                string filename = fi.FullName;

                iOFileOperations.TryAction(() => File.Move(filename, $"{Path.Combine(targetPath, praegeCodeNrFolderName + "\\")}{Path.GetFileName(file) }"));
            }
        }

        public void MoveFilesCode2fromLocal(string praegeCodeNrFolderName, string praegeCode2NrFileName, string sourceFilePath, string targetPath)
        {
            var iOFileOperations = new IOFileOperations();

            IEnumerable<string> files = Directory.EnumerateFiles(sourceFilePath, "*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(praegeCode2NrFileName));
            foreach (string file in files)
            {
                FileInfo fi = new FileInfo(file);
                string filename = fi.FullName;

                iOFileOperations.TryAction(() => File.Move(filename, $"{Path.Combine(targetPath, praegeCodeNrFolderName + "\\")}{Path.GetFileName(file) }"));
            }
        }

        public void CopyDirectoryfromNetwork(string praegeCodeNrFolderName, string praegeCodeNrFileName, string sourceFilePath, string targetPath)
        {
            var iOFileOperations = new IOFileOperations();

            IEnumerable<string> dirs = Directory.EnumerateDirectories(sourceFilePath, "*", System.IO.SearchOption.AllDirectories).Where(x => x.Contains(praegeCodeNrFileName));

            foreach (string dir in dirs)
            {
                iOFileOperations.TryAction(() => FileSystem.CopyDirectory(dir, dir.Replace(sourceFilePath, Path.Combine(targetPath, praegeCodeNrFolderName)), true));
                iOFileOperations.TryAction(() => FileSystem.DeleteDirectory(dir, DeleteDirectoryOption.DeleteAllContents));
            }
        }

        public void CopyFilesfromNetwork(string praegeCodeNrFolderName, string praegeCodeNrFileName, string sourceFilePath, string targetPath)
        {
            var iOFileOperations = new IOFileOperations();

            IEnumerable<string> files = Directory.EnumerateFiles(sourceFilePath, "*", System.IO.SearchOption.TopDirectoryOnly).Where(x => x.Contains(praegeCodeNrFileName));
            foreach (string file in files)
            {
                FileInfo fi = new FileInfo(file);
                string filename = fi.FullName;

                iOFileOperations.TryAction(() => File.Copy(file, $"{Path.Combine(targetPath, praegeCodeNrFolderName + "\\")}{Path.GetFileName(file) }"));
                iOFileOperations.TryAction(() => File.Delete(file));
            }
        }
    }
}


Die Methoden
ausblenden volle Höhe C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
        private void MoveFilesfromLocal(string praegeCodeNrFolderName, string praegeCodeNrFileName, string sourceFilePath, string targetPath)
        {
            for (int i = 0; i < cbLocalSubFolderUses.Length; i++)
            {
                if (cbLocalSubFolderUses[i].Checked)
                {
                    ioFileOperations.MoveDirectoryfromLocal(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
                }
                else
                {
                    ioFileOperations.MoveFilesfromLocal(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
                }
            }
        }

        private void MoveFilesCode2fromLocal(string praegeCodeNrFolderName, string praegeCode2NrFileName, string sourceFilePath, string targetPath)
        {
            for (int i = 0; i < cbActivateCode2LocalApps.Length; i++)
            {
                if (cbActivateCode2LocalApps[i].Checked)
                {
                    ioFileOperations.MoveFilesCode2fromLocal(praegeCodeNrFolderName, praegeCode2NrFileName, sourceFilePath, targetPath);
                }
            }
        }

        private void CopyFilesfromNetwork(string praegeCodeNrFolderName, string praegeCodeNrFileName, string sourceFilePath, string targetPath)
        {
            for (int j = 0; j < cbNetSubFolderUses.Length; j++)
            {
                if (cbNetSubFolderUses[j].Checked)
                {
                    ioFileOperations.CopyDirectoryfromNetwork(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
                }
                else
                {
                    ioFileOperations.CopyFilesfromNetwork(praegeCodeNrFolderName, praegeCodeNrFileName, sourceFilePath, targetPath);
                }
            }
        }

Und das kopieren hat sofort funktioniert.

Zitat:
läßt sich einfach mit einer Hilfsmethode kürzer schreiben (Aufgabe für dich ;-)).

Das habe ich bestimmt nicht so umgesetzt wie du das dir gedacht hast.
Ich habe die Methode aber der Code ist nicht kürzer.
Jede Variable hat sein eigenes Label deswegen weiß ich nicht wie ich es einkürzen kann.

ausblenden volle Höhe C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
41:
42:
43:
44:
45:
46:
47:
48:
49:
50:
51:
52:
53:
54:
55:
56:
57:
58:
59:
60:
61:
62:
63:
64:
65:
66:
        private void S7VariablenStatus(S7VariablenResult s7VariablenResult)
        {
            if (s7VariablenResult.TrigSaveFiles)
            {
                lblSaveFiles.BackColor = Color.Green;
            }
            else
            {
                lblSaveFiles.BackColor = Color.LightGray;
            }

            if (s7VariablenResult.TypLinks)
            {
                lblTypLinks.BackColor = Color.Green;
            }
            else
            {
                lblTypLinks.BackColor = Color.LightGray;
            }

            if (s7VariablenResult.TypRechts)
            {
                lblTypRechts.BackColor = Color.Green;
            }
            else
            {
                lblTypRechts.BackColor = Color.LightGray;
            }

            if (s7VariablenResult.RoboterDSLeer)
            {
                lblRobDSLeer.BackColor = Color.Green;
            }
            else
            {
                lblRobDSLeer.BackColor = Color.LightGray;
            }

            if (s7VariablenResult.FMBauteil)
            {
                lblFMBauteil.BackColor = Color.Green;
            }
            else
            {
                lblFMBauteil.BackColor = Color.LightGray;
            }

            if (s7VariablenResult.HeartBeat)
            {
                lblHeartbeat.BackColor = Color.Green;
            }
            else
            {
                lblHeartbeat.BackColor = Color.LightGray;
            }

            if (s7VariablenResult.RoboterDSLeer)
            {
                lblPraegeCode.Text = "";
                lblFolderName.Text = "";
                lblPraegeCode2.Text = "";

                lblCopyAllFilesResult.Text = "";
                progressBar1.Value = 0;
            }
        }


ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
            uivalues.HeartByte = int.Parse(txtHeartByte.Text);
            uivalues.HeartBit = int.Parse(txtHeartBit.Text);            

            if (BW_CopyAllFiles.IsBusy != true)
            {
                S7VariablenResult s7VariablenResult = ReadResult();

#############################################################################
                S7VariablenStatus(s7VariablenResult);
#############################################################################

                if (s7VariablenResult.TrigSaveFiles && (s7VariablenResult.TypLinks || s7VariablenResult.TypRechts) && !s7VariablenResult.FMBauteil)
                {
                    TimerCopyAllFiles.Enabled = false;
                    BW_CopyAllFiles.RunWorkerAsync();
                }
            }


Zitat:
Und gerade auch noch in deinem Code entdeckt:

Ich habe zwar eine Methode erstellt aber ich weiß nicht wie ich sie in die If else Abfrage integrieren soll.
Dadurch kann ich ich die Methode auch nicht testen.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
        private void OneAppAreChecked()
        {
            const int MAXLOCALAPPS = 8;
            for (int n = 0; n < MAXLOCALAPPS; n++)
            {
                cbActivateLocalApps[n].Checked = false;
                
            }
        }

        private void Connect_Click(object sender, EventArgs e)
        {
            // At least one checkbox must be activated.

###########################################################################
            if (  ??????? )
###########################################################################
            {
                MessageBox.Show("No app is activated!");
            }
            else
            {
                uivalues.Error = txtError.Text;


Etwas ärgerlich das ich nicht alles ohne hilfe umsetzen konnte.

Grüße Tom
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mo 30.05.22 08:35 
Guten Morgen,

Zitat:
Und gerade auch noch in deinem Code entdeckt:

Ich bin ja echt blöd gewesen.

Dafür hattest du mir ja schonmal die Hilfe mit enable gegeben, das Prinzip ist das gleiche.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
private bool OneAppIsChecked()
        {
            if (!chkActivateLocalApp1.Checked
                && !chkActivateLocalApp2.Checked
                && !chkActivateLocalApp3.Checked
                && !chkActivateLocalApp4.Checked
                && !chkActivateLocalApp5.Checked
                && !chkActivateLocalApp6.Checked
                && !chkActivateLocalApp7.Checked
                && !chkActivateLocalApp8.Checked)
            {
                return true;
            }

            return false;
        }

        private void Connect_Click(object sender, EventArgs e)
        {
            // At least one checkbox must be activated.
            if (OneAppIsChecked() == true)
            {
                MessageBox.Show("No app is activated!");
            }
            else


Zitat:
(auch das sollte eine Schleife [als eigene Methode] sein, bzw. kurz mit LINQ.)

Methode habe ich aber ohne Schleife. Das mit LINQ zu lösen kann ich noch nicht. Würdest du mir das bitte zeigen?

So, dann schaue ich mal weiter. Vielen Dank.

Grüße Tom
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Mo 30.05.22 08:44 
Ja, so sieht der Code doch schon viel übersichtlicher (und damit wartbarer) aus.

UserTom hat folgendes geschrieben:
Ich habe es aber nicht statisch hinbekommen.

Dafür dann jeweils noch static vor die Klasse sowie die Methoden schreiben:
static class IOOperations sowie z.B. public static void MoveDirectoryfromLocal(...)
Für TryAction solltest du auch eine Compilerwarnung CA1822: Member als statisch markieren. erhalten (falls die Code Analyse in den Projekteinstellungen nicht explizit deaktiviert ist).

Du hast aber bei deiner nicht-statischen Klasse auch noch einen OOP-Fehler drin. Du brauchst nicht jedesmal innerhalb derselben Klasse ein neues Objekt mit new IOFileOperations() erstellen. Die Methode tryAction ist doch in der selben Klasse (wahrscheinlich hattest du vorher einen anderen Compilerfehler und hast es deswegen so gelöst?).

Kleiner Hinweis noch: Vergleiche mal deine TryAction(...) Aufrufe mit meinem Code im letzten Beitrag (ich hatte noch den Aufruf bzgl. Path.Combine verbessert).

Bei den Farbzuweisungen hatte ich es so ähnlich gedacht:
ausblenden C#-Quelltext
1:
SetColor(Control control, bool condition) { ... }					

Und Aufruf dann je Label kurz in einer Zeile, z.B.
ausblenden C#-Quelltext
1:
SetColor(lblSaveFiles, s7VariablenResult.TrigSaveFiles);					

Die Gesamt-Auslagerung in die Methode S7VariablenStatus ist aber auch eine gute Idee (ich würde die Methode dann nur z.B. ShowS7VariablenStatus nennen, damit klarer ist, was sie macht).

Und bei OneAppAreChecked() fehlt noch bool als Rückgabetyp, so daß du dort dann return false oder return true benutzen kannst.
Und dann funktioniert auch die if (OneAppAreChecked())-Abfrage.

Dies hilft dir hoffentlich alles beim Verständnis für mehr OOP (ich will dich damit nicht quälen ; -).

Edit: Jetzt haben sich unsere Antworten überschnitten. Du hattest ja schon den Code von OneAppIsChecked() als Schleife hingeschrieben. Jetzt nur noch ein bißchen logisch anpassen (bes. statt Checked zu setzen eine Abfrage daraus machen).
Du solltest aber true und false vertauschen (bezogen auf den Methodennamen).

Edit2: LINQ ist ein (eigenes) Thema, das du mal in einem kleinen Testprojekt ausprobieren solltest.
Hier nur kurz, wie ich es gedacht habe: if (!cbActivateLocalApps.Any(cb => cb.Checked)) (bzw. innerhalb der Methode OneAppIsChecked).
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mo 30.05.22 10:33 
Servus,

Vielen Dank für deine Antwort.

Zitat:
Dafür dann jeweils noch static vor die Klasse sowie die Methoden schreiben:

Habe ich geändert. Muss ich noch testen.

Zitat:
Kleiner Hinweis noch: Vergleiche mal deine TryAction(...) Aufrufe mit meinem Code im letzten Beitrag


Habe ich probiert, aber da ist noch ein Klammerfehler drin. Siehe Bild.

Zitat:
Bei den Farbzuweisungen hatte ich es so ähnlich gedacht:

Da muss ich mich erstmal mit SetColor beschäftigen.

Zitat:
Die Gesamt-Auslagerung in die Methode S7VariablenStatus ist aber auch eine gute Idee


Und ich konnte sie so einkürzen.

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
 private void ShowS7VariablenStatus(S7VariablenResult s7VariablenResult)
        {
            lblSaveFiles.BackColor = (s7VariablenResult.TrigSaveFiles) ? Color.Green : Color.LightGray;

            lblTypLinks.BackColor = (s7VariablenResult.TypLinks) ? Color.Green : Color.LightGray;

            lblTypRechts.BackColor = (s7VariablenResult.TypRechts) ? Color.Green : Color.LightGray;

            lblRobDSLeer.BackColor = (s7VariablenResult.RoboterDSLeer) ? Color.Green : Color.LightGray;

            lblFMBauteil.BackColor = (s7VariablenResult.FMBauteil) ? Color.Green : Color.LightGray;

            lblHeartbeat.BackColor = (s7VariablenResult.HeartBeat) ? Color.Green : Color.LightGray;

            if (s7VariablenResult.RoboterDSLeer)
            {
                lblPraegeCode.Text = "";
                lblFolderName.Text = "";
                lblPraegeCode2.Text = "";

                lblCopyAllFilesResult.Text = "";
                progressBar1.Value = 0;
            }
        }


Zitat:
Dies hilft dir hoffentlich alles beim Verständnis für mehr OOP (ich will dich damit nicht quälen ; -).

Hoffe ich auch und es ist auch schon etwas besser geworden mit dem Verständnis, aber da fehlt noch sehr viel. Vielen Dank für die ganze Hilfe.
Es gibt so viele mögliche Lösungswege und wenn man nur einen kennt, weil man noch keine Erfahrung hat entsteht Spaghetticode.

So jetzt noch eine Frage über den Code wie er aussieht. Wir haben viele neu Methoden erstellt. Gibt es eine Vorgabe wo an welcher Stelle
die Methoden aufgerufen werden sollten? Gibt es auch einen logischen Hintergrund, wo ich in einer Methode eine andere Methode aufrufen sollte?

Als Beispiel???

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
 void ShowPlcStatus()

  private void TimerCopyAllFiles_Tick(object sender, EventArgs e)
        {
#####################           
 ShowPlcStatus();
#####################  

            uivalues.TrigSaveFilesByte = int.Parse(txtTrigSaveFilesByte.Text);


So vielen Dank nochmal für die tolle Hilfe.

Grüße Tom
Einloggen, um Attachments anzusehen!
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Mo 30.05.22 11:04 
Die Fehlermeldung sagt doch, was fehlt: eine runde Klammer ;-)
Die Codeänderung ist hauptsächlich, damit man nicht selber im Code die Verzeichnistrenner (Slashes bzw. Backslashes) drin hat (hilft auch für eine mögliche Portierung auf andere Systeme) - und wenn dann sollte man Path.PathSeparator verwenden.

UserTom hat folgendes geschrieben:
Da muss ich mich erstmal mit SetColor beschäftigen.

Und ich konnte sie so einkürzen.

Mit dem Tertiäroperator ? : kann man den Code auch gut einkürzen (auch wenn manche Entwickler das unleserlicher als if ... else ... sehen - ich selber benutze es aber auch überwiegend, wenn es nicht zu sehr verschachtelt ist).

Trotzdem wäre es eine gute Übung, wenn du es noch schaffst SetColor zu implementieren (die vermeidet, daß du die Farben jeweils explizit hinschreiben mußt).
Ich selber würde diese Methode sogar noch mit Defaultparametern für die Farben ausstatten, um sie individuell doch noch ändern zu könnnen:
ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
SetColor(Control control, bool condition, Color colorTrue = default, Color colorFalse = default)
{
  if (colorTrue == default)
    colorTrue = Color.Green;

  if (colorFalse == default)
    colorFalse = Color.LightGray;

  // todo: ...
}
(Color-Werte können nicht direkt als Defaultparameter benutzt werden, da sie nicht konstant, sondern nur static readonly sind - ähnlich wie bei String.Empty :-( ).

UserTom hat folgendes geschrieben:
So jetzt noch eine Frage über den Code wie er aussieht. Wir haben viele neu Methoden erstellt. Gibt es eine Vorgabe wo an welcher Stelle
die Methoden aufgerufen werden sollten? Gibt es auch einen logischen Hintergrund, wo ich in einer Methode eine andere Methode aufrufen sollte?

So eine ähnliche Frage gab es schon mal: Aufrufen von Methoden (diese bezieht sich auf die vorherige Frage vom selben User: Rückgabewerte - wofür brauche ich sie?).
UserTom Threadstarter
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Beiträge: 49



BeitragVerfasst: Mo 30.05.22 13:00 
Hallo,

Vielen Dank für deine Antwort.

Ja habe ich noch herausgefunden, ich habe die Klammer vorher an der falschen Stelle ausprobiert und war nicht akribisch genug noch mehr auszuprobieren.
Was mich wieder ärgert. Probieren nicht wissen.

Zitat:
Trotzdem wäre es eine gute Übung

So ich habe eine Variante ich kann sie aber leider nicht testen. Was ist deine Meinung?

ausblenden C#-Quelltext
1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
28:
29:
30:
 private void SetColor(Control control, bool condition, Color colorTrue = default, Color colorFalse = default)
        {
            if (colorTrue == default)
                colorTrue = Color.Green;

            if (colorFalse == default)
                colorFalse = Color.LightGray;

            if (condition)
            {
                control.BackColor = colorTrue;
            }
            else
            {
                control.BackColor = colorFalse;
            }
        }

        private void ShowS7VariablenStatus(S7VariablenResult s7VariablenResult)
        {
            if (s7VariablenResult.RoboterDSLeer)
            {
                SetColor(lblSaveFiles, true);
            }
            else
            {
                SetColor(lblSaveFiles, false);
            }

                //lblSaveFiles.BackColor = (s7VariablenResult.TrigSaveFiles) ? Color.Green : Color.LightGray;


Falls es richtig ist, dann ist es gut für die Wartbarkeit. Habe ich verstanden, ich brauche nur an einer Stelle die Farbe ändern
und kann sogar die Methode für andere Situationen hernehmen. Für die Anzahl der Codezeilen wieder ein Rückschritt, was aber für den
Vorteil der Wartbarkeit zu vernachlässigen ist.

Wenn ich es richtig verstanden habe, ist das hier jetzt ein gutes Beispiel für die OOP gewesen?

Zitat:
So eine ähnliche Frage gab es schon mal:

Habe ich mir durchgelesen. Muss ich erstmal darüber nachdenken und vielleicht nochmal lesen.

Vielen Dank für deine tolle Unterstützung.

Grüße Tom
Th69
ontopic starontopic starontopic starontopic starontopic starontopic starontopic starontopic star
Moderator
Beiträge: 4764
Erhaltene Danke: 1052

Win10
C#, C++ (VS 2017/19/22)
BeitragVerfasst: Mo 30.05.22 13:47 
Fast gut! ;-)

Ich hatte doch schon den (kurzen, einzeiligen) Aufruf angegeben:
ausblenden C#-Quelltext
1:
SetColor(lblSaveFiles, s7VariablenResult.TrigSaveFiles);					

So hast du keine mehrzeilige if ... else ...-Anweisung mehr beim Aufruf (TrigSaveFiles ist ja ein bool-Wert, also true oder false).

(Ist RoboterDSLeer jetzt ein Schreibfehler bei dir, statt TrigSaveFiles?)

Wenn du noch eine konkrete Frage bzgl. der Methodenaufrufe hast, dann stelle sie ruhig (ist besser als nur allgemein darüber zu diskutieren).