Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I am currently programming, but i messed up my code, what is the best way to clean my code.

I really need to know this, next week i must be done with the assignment, but the teacher said that the code is not nice.

Any suggestions are welcome.

Here is an example of a method ( buttonUitvoeren_Click_1 ).

private void buttonUitvoeren_Click_1(object sender, EventArgs e)
{
    buttonNoodstop.Enabled = true;
    buttonPauze.Enabled = true;
    buttonUitvoeren.Enabled = false;

    // Orderlijst uitvoeren en meteen nieuwe status wegschrijven wanneer order klaar is
    if (orderLijst.Count > 0)
    {
        // tussen orders kijken of er gespoeld moet worden eerste beker
        for (int t = 0; t < orderLijst.Count; t++)
        {
            // kijk naar laatste beker in huidige en eerste in volgende om te kijken of er gespoeld moet worden
            if (t > 0 && t < orderLijst.Count)
            {
                if (orderLijst[t].BekerList[(orderLijst[t].BekerList.Count - 1)].PercentageKleur !=
                    orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].PercentageKleur)
                {
                    orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].Spoelen = true;
                    orderLijst[t + 1].Spoelingen++;
                    orderLijst[t + 1].Tijd += 40;
                }

                // refresh listview met nieuwe tijd en spoelingen
                refreshList();
            }

            // ordernummer setten zodat deze gebruikt kan worden in pauze methode
            currentorder = orderLijst[t].OrderNummer;

            // zolang order nog niet klaar is
            while (orderLijst[t].OrderStatus != "Klaar")
            {
                // ga alle bekers in de order af
                for (int i = 0; i < orderLijst[t].BekerList.Count; i++)
                {
                    // spoelen van beker
                    if (orderLijst[t].BekerList[i].Spoelen == true && orderLijst[t].BekerList[i].Gespoeld == false)
                    {
                        // nieuwe beker aanmaken met 0 % kleur
                        Beker beker = new Beker(0);
                        thread = new Thread(() => newAanvoerband.Beweeg(new Beker(0)));
                        thread.Start();
                        while (beker.Klaar == false && thread.IsAlive == true)
                        {
                            // Blijf acties uitvoeren in huidige thread(Form)
                            Application.DoEvents();

                            // Abort thread wanneer spoelen klaar is of wanneer noodstop actief wordt
                            if (noodstop == true || beker.Klaar == true)
                            {
                                // afhankelijk van welke waar is de label setten
                                thread.Abort();
                                if (noodstop == true)
                                {
                                    // zorgen dat hij uit de loop springt
                                    t = orderLijst.Count + 1;
                                    i = orderLijst[t].BekerList.Count + 1;
                                    addToLogListView("Noodstop is geactiveerd op order:" + orderLijst[t].OrderNummer);
                                }
                                if (beker.Klaar == true)
                                {
                                    addToLogListView("Er is succesvol gespoeld");
                                }
                            }
                        }
                    }

                    // "normale" uitvoer van beker , met mengverhouding etc
                    while (orderLijst[t].BekerList[i].Klaar == false && noodstop == false)
                    {
                        thread = new Thread(() => newAanvoerband.Beweeg(orderLijst[t].BekerList[i]));
                        thread.Start();
                        while (thread.IsAlive == true && noodstop == false)
                        {
                            // Blijf acties uitvoeren in huidige thread(Form)
                            Application.DoEvents();

                            // Abort thread wanneer beker klaar is of wanneer noodstop actief wordt
                            if (orderLijst[t].BekerList[i].Klaar == true || noodstop == true)
                            {
                                thread.Abort();
                                if (noodstop == true)
                                {
                                    // zorgen dat hij uit de loop springt
                                    t = orderLijst.Count + 1;
                                    i = orderLijst[t].BekerList.Count + 1;
                                    addToLogListView("Order:" + orderLijst[t].OrderNummer + " is beeindigt");
                                }
                                else if (orderLijst[t].BekerList[i].Klaar == true)
                                {
                                    if (orderLijst[t].BekerList[i].Gespoeld == false && orderLijst[t].BekerList[i].Klaar == true)
                                    {
                                        addToLogListView("Beker " + i + " is gevult");
                                    }
                                }
                            }
                            else if (pauzestand == true)
                            {
                                addToLogListView("Order:" + orderLijst[t].OrderNummer + " is gepauzeerd");
                            }
                            else if (pauzestand == false && noodstop == false && orderLijst[t].BekerList[i].Klaar == false)
                            {
                                timer.Tick += new EventHandler(timer_Tick);
                                timer.Start();
                            }

                            // Zolang de uitvoer nog bezig is en noodstop niet geactiveert is
                        }
                    }
                }

                // einde bekerlijst
                addToLogListView("Order:" + orderLijst[t].OrderNummer + " is met succes afgerond");
                orderLijst[t].OrderStatus = "Klaar";

                // Als de status klaar is , schrijf nieuwe status naar textfile
                if (noodstop == false && orderLijst[t].OrderStatus == "Klaar")
                {
                    // Leeg orders.txt
                    try
                    {
                        File.WriteAllText(filesOrdersOrderstxt, String.Empty);
                    }
                    catch (Exception ex)
                    {
                        addToLogListView("Error while emptying:" + ex.Message);
                    }

                    // Schrijf de orders in orderLijst naar Orders.txt
                    try
                    {
                        StreamWriter sw = File.AppendText(filesOrdersOrderstxt);
                        refreshList();
                        foreach (ListViewItem writeorder in orderListView.Items)
                        {
                            sw.WriteLine(writeorder.SubItems[0].Text + seperator + writeorder.SubItems[1].Text);
                        }
                        sw.Close();
                    }
                    catch (Exception ex)
                    {
                        addToLogListView("Error while writing:" + ex.Message);
                    }

                    // Wanneer het programma klaar is met uitvoeren mag er weer uitgevoerd worden
                    buttonUitvoeren.Enabled = true;
                }
            }
        }

        // einde orderlijst
        addToLogListView("Alle " + orderLijst.Count.ToString() + " orders zijn klaar.");
        timer.Stop();
    }

    // Als er geen orders ingevoerd zijn
    else
    {
        buttonNoodstop.Enabled = false;
        buttonPauze.Enabled = false;
        buttonUitvoeren.Enabled = true;
        addToLogListView("Er zijn geen orders ingevoerd");
    }
}

EDIT:

Thanks for the feedback , especially you Jeff Vanzella. I have worked a bit on the solution and this is what I have come up with.

One of the points my teacher made is that the code generally doesn't go into the frames. This is why I have moved the code a new class called Controller, this class basically manages the output.

Which has replaced the original code with this:

uitvoer = new Thread(() => newController.VoerAlleOrdersuit(orderLijst));
        uitvoer.Start();
        while (uitvoer.IsAlive == true && newController.Noodstop == false)
        {
            Application.DoEvents();
            if (newController.Noodstop == true)
            {
                uitvoer.Abort();
            }
        }
    SchijfNaarFile();

This piece of code here just calls the method which will handle the output which is displayed beneath this sentence:

public bool VoerAlleOrdersuit(List<Order> orderLijst)
    {
        foreach (Order order in orderLijst)
        {
            foreach (Beker beker in order.BekerList)
            {
                VoerBekerUit(beker);
            }
            return true;
        }
        return false;
    }

And this little code calls this method:

 public void VoerBekerUit(Beker beker)
    {
        timer.Enabled = true;
        timer.Interval = 5000;
        timer.Tick += new System.EventHandler(timer_Tick);
        aanvoerbandUitvoer = new Thread(() => newAanvoerband.Beweeg());
        vulstationUitvoer = new Thread(() => newVulstation.BehandelBeker(beker));
        aanvoerbandUitvoer.Start();
        vulstationUitvoer.Start();
        timer.Start();
        while (beker.ProcesDoorlopen == false && noodstop == false)
        {
            timer.Tick += new EventHandler(timer_Tick);
            if (beker.ProcesDoorlopen == true || noodstop == true)
            {
                aanvoerbandUitvoer.Abort();
                vulstationUitvoer.Abort();
            }
        }
    }

My question is if what I made is an imrovement over the code shown in the original post.

Many thanks !

share|improve this question
1  
You are mixing English with ... Dutch? in comments, variable names and messages. I suppose messages must remain in the original language in the final code, but not knowing what it says would help understanding the logic better too. Could you make those changes? Also, at the end of the day you should not need so many comments. – Leonid Jan 17 at 18:41
I moved your further comments back into the question as they were not an answer, just expanding on the question :) – Jeff Vanzella Jan 18 at 22:57
As for your question, they are an improvement. The standards look good, you have removed almost all of the comments and replaced them with intuitive names for your methods. You seem to understand what methods are used for now, and separating the domain logic from your ui logic. – Jeff Vanzella Jan 18 at 22:58

2 Answers

up vote 7 down vote

I would start by trying to extract methods from your code. My general rule of thumb is that a method should be small enough to fit on the screen without having to scroll up or down.

Here is an example:

if (t > 0 && t < orderLijst.Count)
{
    if (orderLijst[t].BekerList[(orderLijst[t].BekerList.Count - 1)].PercentageKleur !=
        orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].PercentageKleur)
    {
        orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].Spoelen = true;
        orderLijst[t + 1].Spoelingen++;
        orderLijst[t + 1].Tijd += 40;
    }

    // refresh listview met nieuwe tijd en spoelingen
    refreshList();
}

Could become:

private void ProcessOrderIfInRange(int t)
{
    if (t <= 0 || t >= orderLijst.Count)
    {
        return;
    }

    if (orderLijst[t].BekerList[(orderLijst[t].BekerList.Count - 1)].PercentageKleur !=
        orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].PercentageKleur)
    {
        orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].Spoelen = true;
        orderLijst[t + 1].Spoelingen++;
        orderLijst[t + 1].Tijd += 40;
    }

    refreshList();
}

and in your main method:

for (int t = 0; t < orderLijst.Count; t++)
{
    ProcessOrderIfInRange(t);

    // ...

I also find you are using too many comments (from what I can read of them). For example, when I see a method refreshList(), I know that is probably going to refresh a list. a comment like // refresh listview met nieuwe tijd en spoelingen is unnecessary. One thing though, you might want to rename refreshList() to something which explains it a bit better, i.e. RefreshListViewWithUpdatedData()

I also noticed you are not following C# standards. In C# method names start with a capital: RefreshList() (Pascal Case), local variable names start with a low case letter (Camel). I'd suggest you read Microsoft's Page on casing.

I also find you have WAY to many nested if statements. Things get really confusing when you start trying to figure out the code path. Using the same method extraction I mentioned above, you should be able to totally eliminate the nested ifs by pulling them out.

if (a)
{
     if (b)
     {
          // Do something
     }
     else
     {
          // Do something
     }
}
else
{
     if (c)
     {
          // Do something
     }
     else
     {
          // Do something
     }
}

Should be changed into

if (a)
{
     ProcessB();
}
else
{
     ProcessC();
}

where

private void ProcessB()
{
     if (b)
     {
          // Do something
     }
     else
     {
          // Do something
     }
}

private void ProcessC()
{
     if (c)
     {
          // Do something
     }
     else
     {
          // Do something
     }
}

Of course ProcessB() and ProcessC() could be changed to return something if required.

These suggestions are a start, try them out, see what you come up with and repost, your code.

share|improve this answer
1  
A good rule of thumb to add is that your event handler should do nothing more than verifying the event args and passing off to another method, preferably in another class (a controller or presenter, were you using the MVC/MVP pattern to structure your code). It also looks like you could be using LINQ to do sorting and processing your data for display in a more readable fashion. – jessehouwing Jan 17 at 22:08
Fantastic answer; in summary, more modular and less 'arrow-headed'. – Richard Jan 18 at 9:51
up vote 0 down vote

I see a problem in here:

for (int t = 0; t < orderLijst.Count; t++)
    {
        // kijk naar laatste beker in huidige en eerste in volgende om te kijken of er gespoeld moet worden
        if (t > 0 && t < orderLijst.Count)
        {
            if (orderLijst[t].BekerList[(orderLijst[t].BekerList.Count - 1)].PercentageKleur !=
                orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].PercentageKleur)
            {
                orderLijst[t + 1].BekerList[(orderLijst[t + 1].BekerList.Count - 1)].Spoelen = true;
                orderLijst[t + 1].Spoelingen++;
                orderLijst[t + 1].Tijd += 40;
            }

            // refresh listview met nieuwe tijd en spoelingen
            refreshList();
        }

In this for loop you already declare that t will be less than orderLijst.Count so there is no need for checking it in if statement. Also, in this if statement you are using t>0, shich means that you will skip first element of orderLijst, and I suppose that it shouldn't be that way. To sum it up, I would avoid this line:

if (t > 0 && t < orderLijst.Count)

And if you really need t to have value larger than 0 in this part of code, you can leave only

if(t > 0)

Hope this helps.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.