optymalizacja kodu (dużo if-ów)

0

powiadają że początkującego programistę poznaje się po stosowaniu przez niego dużej ilości if-ów
czy poniższy kod da się jakość przekształcić/zoptymalizować?
i czy w ogóle ten kod jest poprawnie napisany? co prawda działa ale nie podoba mi się :-?

kod ma za zadanie porównać zawartości kilku dt i uzupełnić DGV

 public int AktualizujStan()
        {
            int dtObecniRowCount = 0;
            dtObecniRowCount = dtObecni.Rows.Count;
            dtPaneleRCPRowCount = dtPaneleRCP.Rows.Count;
            int dtAlarmyRowCount = dtAlarmy.Rows.Count;

            if (dtAlarmyRowCount > 0 && dtAlarmy.Rows[0]["STAMP"].ToString() != StampPoprzedniegoAlarmu )
                
            {
                                
                    for (int j = 0; j < dtPaneleRCPRowCount; j++)
                    {
                        if (dtAlarmy.Rows[0]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString())
                        {
                            if (dGVGateState.Rows[j].Cells["STAN"].Value.ToString() == "OTWARTY")
                            { }
                            else
                            {
                                dtPaneleRCP.Rows[j]["PRACOWNIK"] = "";
                                dtPaneleRCP.Rows[j]["DATA"] = dtAlarmy.Rows[0]["DATA"];
                                dtPaneleRCP.Rows[j]["GODZINA"] = dtAlarmy.Rows[0]["GODZINA"];
                                dGVGateState.Rows[j].DefaultCellStyle.BackColor = Color.Red;
                                dGVGateState.Rows[j].Cells["STAN"].Value = dtAlarmy.Rows[0]["Event"];
                                                            
                                MediaFile.PlayLooping();
                            }

                        }
                    }
                    StampPoprzedniegoAlarmu =dtAlarmy.Rows[0]["STAMP"].ToString();
                }
            
            else
            {
                if (dtObecniRowCount > 0)
                {
                    for (int i = 0; i < dtObecniRowCount; i++)
                    {
                        for (int j = 0; j < dtPaneleRCPRowCount; j++)
                        {
                            if (dtObecni.Rows[i]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString())
                            {
                                if (dGVGateState.Rows[j].Cells["STAN"].Value.ToString() == "Alarm - Wlamanie")
                                { }
                                else
                                {
                                    dtPaneleRCP.Rows[j]["PRACOWNIK"] = dtObecni.Rows[i]["PRACOWNIK"];
                                    dtPaneleRCP.Rows[j]["DATA"] = dtObecni.Rows[i]["DATA"];
                                    dtPaneleRCP.Rows[j]["GODZINA"] = dtObecni.Rows[i]["GODZINA"];
                                    dGVGateState.Rows[j].DefaultCellStyle.BackColor = Color.DarkSeaGreen;
                                    dGVGateState.Rows[j].Cells["STAN"].Value = "OTWARTY";
                                   // dGVGateState.Refresh();
                                    
                                }

                            }
                            else
                            {
                                if (dGVGateState.Rows[j].Cells["STAN"].Value.ToString() == "Alarm - Wlamanie")
                                { }
                                else
                                {
                                    dtPaneleRCP.Rows[j]["PRACOWNIK"] = "";
                                    
                                    dGVGateState.Rows[j].DefaultCellStyle.BackColor = Color.LightSteelBlue;
                                    if (dGVGateState.Rows[j].Cells["STAN"].Value.ToString() == "OTWARTY")
                                    {
                                        dtPaneleRCP.Rows[j]["DATA"] = DateTime.Now.ToString("yyyy-MM-dd");
                                        dtPaneleRCP.Rows[j]["GODZINA"] = DateTime.Now.ToString("HH:mm:ss");
                                         //dGVGateState.Refresh();
                                    }
                                    else
                                    {
                                        dtPaneleRCP.Rows[j]["DATA"] = "";
                                        dtPaneleRCP.Rows[j]["GODZINA"] = "";
                                    }
                                    dGVGateState.Rows[j].Cells["STAN"].Value = "ZAMKNIĘTY";
                                }

                            }
                        }
                    }

                }
                else
                {
                    for (int k = 0; k < dtPaneleRCPRowCount; k++)
                    {
                        if (dGVGateState.Rows[k].Cells["STAN"].Value.ToString() == "Alarm - Wlamanie")
                        { }
                        else
                        {
                            dtPaneleRCP.Rows[k]["PRACOWNIK"] = "";
                            
                            dGVGateState.Rows[k].DefaultCellStyle.BackColor = Color.LightSteelBlue;
                            if (dGVGateState.Rows[k].Cells["STAN"].Value.ToString() == "OTWARTY")
                            {
                                dtPaneleRCP.Rows[k]["DATA"] = DateTime.Now.ToString("yyyy-MM-dd");
                                dtPaneleRCP.Rows[k]["GODZINA"] = DateTime.Now.ToString("HH:mm:ss");
                                //dGVGateState.Refresh();
                            }
                            else
                            {
                                dtPaneleRCP.Rows[k]["DATA"] = "";
                                dtPaneleRCP.Rows[k]["GODZINA"] = "";
                            }
                            dGVGateState.Rows[k].Cells["STAN"].Value = "ZAMKNIĘTY";
                        }

                    }
                }
            }
            return dtPaneleRCPRowCount;
 	
 }

 
0
  1. Przede wszystkim wywal te puste ify, po co Ci one?
  2. Po drugie powielający się kod (te 5 linii ustawiających coś w dtPaneleRCP i w dGVGateState przenieś do nowej metody.
  3. Jak już będzie można to ogarnąć wzrokiem, to pomyśli się, co jeszcze można zrobić.
0

Hmm...
Faktycznie, trochę straszny ten kod ;)

  1. Notacja węgierska (dtAlarmyRowCount itd) - ogólnie dzisiaj niezalecana.

  2. Ponglish (dtAlarmyRowCount) - bardzo nieprzyjemne w czytaniu.

if (dGVGateState.Rows[j].Cells["STAN"].Value.ToString() == "OTWARTY")
{ }
else .....

Chcesz trafić do perełek? 0_o

if (dGVGateState.Rows[j].Cells["STAN"].Value.ToString() != "OTWARTY") .....

Teraz bardziej subtelne i mniej oczywiste zmiany, ale nie mniej ważne (ważniejsze, ale trudniejsze do sklasyfikowania).

  1. Powtórzenia kodu = zło. Walczy się z nim przez wprowadzania dodatkowych funkcji (patrz 5.)

  2. Rozbicie kodu na funkcje, abstrakcja.
    np. zamiast dtAlarmyRowCount > 0 wyodrębnić funkcję bool AnyAlarmInDatabase(), a zamiast dtAlarmy.Rows[0]["STAMP"].ToString() stworzyć GetStamp(int ndx).
    To może się wydawać dość kontrowersyjne, ale fantastycznie polepsza czytelność kodu.

Cały twój kod (tzn. tą funkcję) możnaby rozbić na

if (AnyAlarmInDatabase() && GetStamp(0) != LastStamp)
{
	DoFirstThing();
}
else if (AnyPresentInDatabase()) // obecni > 0, nie wiem jak przetłumaczyć :)
{
	DoSecondThing();
}
else
{
	DoThirgThing();
}

Zaproponowałbym lepsze nazwy, ale nie wiem sam co robią te funkcje (co świadczy o tym właśnie że jest źle :) )...

Te podfunkcje również można rozbić, tak aby każda wykonywała jedną, dobrze określoną funkcjonalność.

To takie pierwsze wrażenia :)

0

dziękuję za ustosunkowanie się do kodu i wskazanie odpowiedniej drogi
poniżej załączam przerobiony kod
proszę o porcję krytycznych/konstruktywnych uwag :-)

    public int AktualizujStan()
    {
        int dtObecniRowCount = 0;
        dtObecniRowCount = dtObecni.Rows.Count;
        dtPaneleRCPRowCount = dtPaneleRCP.Rows.Count;
           

        if (AnyAlarmInDatabase() && GetAlarmStamp != LastAlarmStamp)
        {          
                for (int j = 0; j < dtPaneleRCPRowCount; j++)
                {
                    if (dtAlarmy.Rows[0]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString())
                    {
                        if (GetdGVGateStateStan(j) != "OTWARTY")
                        {
                       
      SetdGVGateState(j, "", dtAlarmy.Rows[0]["DATA"].ToString(), dtAlarmy.Rows[0]["GODZINA"].ToString(), Color.Red, dtAlarmy.Rows[0]["Event"].ToString());
                   
                           MediaFile.PlayLooping();
                        
                        }

                    }

                 }
                LastAlarmStamp = GetAlarmStamp;
        }
        
        else
        {
            if (dtObecniRowCount > 0)
            {
                for (int i = 0; i < dtObecniRowCount; i++)
                {
                    for (int j = 0; j < dtPaneleRCPRowCount; j++)
                    {
                        if (dtObecni.Rows[i]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString())
                        {
                            if (GetdGVGateStateStan(j) != "Alarm - Wlamanie")

                            {
  SetdGVGateState(j, dtObecni.Rows[i]["PRACOWNIK"].ToString(), dtObecni.Rows[i]["DATA"].ToString(),
dtObecni.Rows[i]["GODZINA"].ToString(), Color.DarkSeaGreen, "OTWARTY");
                                                              
                            }

                        }
                        else
                        {
                            SetdGVGateStateStan(j);
                            SetdGVGateStateStan(j);

                        }
                    }
                }

            }
            else
            {
                for (int k = 0; k < dtPaneleRCPRowCount; k++)
                {
                    SetdGVGateStateStan(k);

                }
            }
        }

        return dtPaneleRCPRowCount;

}

    private bool AnyAlarmInDatabase()
    {
        int dtAlarmyRowCount = dtAlarmy.Rows.Count;

        if (dtAlarmyRowCount > 0)
        {
            return true;
        }
        else
        {
            return false;
        }
    }

    private void SetdGVGateState(int j, string pracownik, string data, string godzina, Color kolor, string stan)
    {
        dtPaneleRCP.Rows[j]["PRACOWNIK"] = pracownik;
        dtPaneleRCP.Rows[j]["DATA"] = data;
        dtPaneleRCP.Rows[j]["GODZINA"] = godzina;
        dGVGateState.Rows[j].DefaultCellStyle.BackColor = kolor;
        dGVGateState.Rows[j].Cells["STAN"].Value = stan;
        dGVGateState.Refresh();
    }

    private string GetAlarmStamp
    {
        get { return dtAlarmy.Rows[0]["STAMP"].ToString(); }
    }

    private string GetdGVGateStateStan(int j)
    {
        return dGVGateState.Rows[j].Cells["STAN"].Value.ToString();
    }


    private void SetdGVGateStateStan(int j)
    {
        switch (GetdGVGateStateStan(j))
        {
            case "OTWARTY":
                {
                    dtPaneleRCP.Rows[j]["DATA"] = DateTime.Now.ToString("yyyy-MM-dd");
                    dtPaneleRCP.Rows[j]["GODZINA"] = DateTime.Now.ToString("HH:mm:ss");
                    break;
                }
            case "":
                {
                    dtPaneleRCP.Rows[j]["DATA"] = "";
                    dtPaneleRCP.Rows[j]["GODZINA"] = "00:00:00";
                    break;
                }
        }

         dGVGateState.Rows[j].DefaultCellStyle.BackColor = Color.LightSteelBlue;
         dtPaneleRCP.Rows[j]["PRACOWNIK"] = "";
         dGVGateState.Rows[j].Cells["STAN"].Value = "ZAMKNIĘTY";
    }
0

sformatowany kod kolegi, bo tego powyzej nie da sie czytac:

public int AktualizujStan()
{
  int dtObecniRowCount = 0;
  dtObecniRowCount = dtObecni.Rows.Count;
  dtPaneleRCPRowCount = dtPaneleRCP.Rows.Count;


  if (AnyAlarmInDatabase() && GetAlarmStamp != LastAlarmStamp)
    {
      for (int j = 0; j < dtPaneleRCPRowCount; j++)
        {
          if (dtAlarmy.Rows[0]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString())
            {
              if (GetdGVGateStateStan(j) != "OTWARTY")
                {

                  SetdGVGateState(j, "", dtAlarmy.Rows[0]["DATA"].ToString(), dtAlarmy.Rows[0]["GODZINA"].ToString(), Color.Red, dtAlarmy.Rows[0]["Event"].ToString());

                  MediaFile.PlayLooping();

                }

            }

        }
      LastAlarmStamp = GetAlarmStamp;
    }

  else
    {
      if (dtObecniRowCount > 0)
        {
          for (int i = 0; i < dtObecniRowCount; i++)
            {
              for (int j = 0; j < dtPaneleRCPRowCount; j++)
                {
                  if (dtObecni.Rows[i]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString())
                    {
                      if (GetdGVGateStateStan(j) != "Alarm - Wlamanie")

                        {
                          SetdGVGateState(j, dtObecni.Rows[i]["PRACOWNIK"].ToString(), dtObecni.Rows[i]["DATA"].ToString(),
                          dtObecni.Rows[i]["GODZINA"].ToString(), Color.DarkSeaGreen, "OTWARTY");

                        }

                    }
                  else
                    {
                      SetdGVGateStateStan(j);
                      SetdGVGateStateStan(j);

                    }
                }
            }

        }
      else
        {
          for (int k = 0; k < dtPaneleRCPRowCount; k++)
            {
              SetdGVGateStateStan(k);

            }
        }
    }

  return dtPaneleRCPRowCount;

}

private bool AnyAlarmInDatabase()
{
  int dtAlarmyRowCount = dtAlarmy.Rows.Count;

  if (dtAlarmyRowCount > 0)
    {
      return true;
    }
  else
    {
      return false;
    }
}

private void SetdGVGateState(int j, string pracownik, string data, string godzina, Color kolor, string stan)
{
  dtPaneleRCP.Rows[j]["PRACOWNIK"] = pracownik;
  dtPaneleRCP.Rows[j]["DATA"] = data;
  dtPaneleRCP.Rows[j]["GODZINA"] = godzina;
  dGVGateState.Rows[j].DefaultCellStyle.BackColor = kolor;
  dGVGateState.Rows[j].Cells["STAN"].Value = stan;
  dGVGateState.Refresh();
}

private string GetAlarmStamp
{
  get { return dtAlarmy.Rows[0]["STAMP"].ToString();
  }
}

private string GetdGVGateStateStan(int j)
{
  return dGVGateState.Rows[j].Cells["STAN"].Value.ToString();
}


private void SetdGVGateStateStan(int j)
{
  switch (GetdGVGateStateStan(j))
    {
    case "OTWARTY":
    {
      dtPaneleRCP.Rows[j]["DATA"] = DateTime.Now.ToString("yyyy-MM-dd");
      dtPaneleRCP.Rows[j]["GODZINA"] = DateTime.Now.ToString("HH:mm:ss");
      break;
    }
    case "":
    {
      dtPaneleRCP.Rows[j]["DATA"] = "";
      dtPaneleRCP.Rows[j]["GODZINA"] = "00:00:00";
      break;
    }
    }

  dGVGateState.Rows[j].DefaultCellStyle.BackColor = Color.LightSteelBlue;
  dtPaneleRCP.Rows[j]["PRACOWNIK"] = "";
  dGVGateState.Rows[j].Cells["STAN"].Value = "ZAMKNIĘTY";
}

jak na mnie dalej jest nieczytelny

0

Małe poprawki. Jeśli każdy, kto tu zajrzy coś poprawi, to w końcu będzie to czytelne. :)

public int AktualizujStan()
{
    int dtObecniRowCount = dtObecni.Rows.Count;
    int dtPaneleRCPRowCount = dtPaneleRCP.Rows.Count;

    if (AnyAlarmInDatabase() && GetAlarmStamp != LastAlarmStamp)
    {
        this.ZjecObiat(dtPaneleRCPRowCount);
    }
    else
    {
        if (dtObecniRowCount > 0)
        {
            this.WydymajChomika(dtObecniRowCount, dtPaneleRCPRowCount);
        }
        else
        {
            this.ZróbDurząKupe(dtPaneleRCPRowCount);
        }
    }

    return dtPaneleRCPRowCount;
}

private void ZjecObiat(int dtPaneleRCPRowCount)
{
    for (int j = 0; j < dtPaneleRCPRowCount; j++)
    {
        if (dtAlarmy.Rows[0]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString() && GetdGVGateStateStan(j) != "OTWARTY")
        {
            SetdGVGateState(j, "", dtAlarmy.Rows[0]["DATA"].ToString(), dtAlarmy.Rows[0]["GODZINA"].ToString(), Color.Red, dtAlarmy.Rows[0]["Event"].ToString());
            MediaFile.PlayLooping();
        }
    }
    LastAlarmStamp = GetAlarmStamp;
}

private void WydymajChomika(int dtObecniRowCount, int dtPaneleRCPRowCount)
{
    for (int i = 0; i < dtObecniRowCount; i++)
    {
        for (int j = 0; j < dtPaneleRCPRowCount; j++)
        {
            if (dtObecni.Rows[i]["KONTROLER"].ToString() == dtPaneleRCP.Rows[j]["NAZWA"].ToString() && GetdGVGateStateStan(j) != "Alarm - Wlamanie")
            {
                SetdGVGateState(j, dtObecni.Rows[i]["PRACOWNIK"].ToString(), dtObecni.Rows[i]["DATA"].ToString(),
                dtObecni.Rows[i]["GODZINA"].ToString(), Color.DarkSeaGreen, "OTWARTY");
            }
            else
            {
                SetdGVGateStateStan(j);
                SetdGVGateStateStan(j);
            }
        }
    }
}

private void ZróbDurząKupe(int dtPaneleRCPRowCount)
{
    for (int k = 0; k < dtPaneleRCPRowCount; k++)
    {
        SetdGVGateStateStan(k);
    }
}

private bool AnyAlarmInDatabase()
{
    return dtAlarmy.Rows.Count > 0;
}

private void SetdGVGateState(int j, string pracownik, string data, string godzina, Color kolor, string stan)
{
    dtPaneleRCP.Rows[j]["PRACOWNIK"] = pracownik;
    dtPaneleRCP.Rows[j]["DATA"] = data;
    dtPaneleRCP.Rows[j]["GODZINA"] = godzina;
    dGVGateState.Rows[j].DefaultCellStyle.BackColor = kolor;
    dGVGateState.Rows[j].Cells["STAN"].Value = stan;
    dGVGateState.Refresh();
}

private string GetAlarmStamp
{
    get
    {
        return dtAlarmy.Rows[0]["STAMP"].ToString();
    }
}

private string GetdGVGateStateStan(int j)
{
    return dGVGateState.Rows[j].Cells["STAN"].Value.ToString();
}

private void SetdGVGateStateStan(int j)
{
    switch (GetdGVGateStateStan(j))
    {
        case "OTWARTY":
            {
                dtPaneleRCP.Rows[j]["DATA"] = DateTime.Now.ToString("yyyy-MM-dd");
                dtPaneleRCP.Rows[j]["GODZINA"] = DateTime.Now.ToString("HH:mm:ss");
                break;
            }
        case "":
            {
                dtPaneleRCP.Rows[j]["DATA"] = "";
                dtPaneleRCP.Rows[j]["GODZINA"] = "00:00:00";
                break;
            }
    }

    dGVGateState.Rows[j].DefaultCellStyle.BackColor = Color.LightSteelBlue;
    dtPaneleRCP.Rows[j]["PRACOWNIK"] = "";
    dGVGateState.Rows[j].Cells["STAN"].Value = "ZAMKNIĘTY";
} 

1 użytkowników online, w tym zalogowanych: 0, gości: 1