rate my first c# app

davetherave2 · 25 Mar 2007 at 15:39

I have just created my first app and just wondering how my code could be improved.

Its a simpel app for when computer equipment is out on loan, when it is due to return, if it is overdue and add in new equipment.

here are some images of the screens.

here is the code

Code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;
using System.IO;

namespace Loan_Kit
{
    public partial class Form1 : Form
    {
        string filename = "eventitems.txt";
        string checkeditem;

        public Form1()
        {
            InitializeComponent();
            LoadAllCombos(filename);
        }

        private void LoadAllCombos(string filename)
        {
            comboBox1.Items.Clear();
            string currentline;
            string itemname;
            int pos;
            StreamWriter writer = new StreamWriter(filename, true);
            writer.Close();

            StreamReader reader = new StreamReader(filename);

            while (reader.EndOfStream != true)
            {
                currentline = reader.ReadLine();
                pos = currentline.IndexOf(',');
                itemname = currentline.Substring(0, pos);
                comboBox1.Items.Add(itemname);
            }
            reader.Close();

        }






        private void button1_Click(object sender, EventArgs e)
        {
            string access;

            StreamWriter textwriter = new StreamWriter(filename, true);
            access = textBox6.Text.Replace( "\r\n", ",");
            //access = textBox6.Text.Replace("\n", ",");
            textwriter.WriteLine(textBox2.Text.ToString() + "," + textBox3.Text.ToString() + "," +
                textBox4.Text.ToString() + "," + textBox5.Text.ToString() + "," + access + ";inn");
            textwriter.Close();
            LoadAllCombos(filename);
           

        }

        private void comboBox1_SelectedIndexChanged(object sender, EventArgs e)
        {
            int index;
            int loop = 0;
            int pos1,pos2,pos3;
            string currentline = "";
            string part;

            index = comboBox1.SelectedIndex;
            StreamReader reader = new StreamReader(filename);
            while (loop <= index)
            {
                currentline = reader.ReadLine();
                loop++;     
            }

            pos1 = currentline.IndexOf(",");
            pos2 = currentline.IndexOf(",", pos1 + 1);
            part = currentline.Substring(pos1 + 1, pos2 - pos1 - 1);
            label8.Text = "Make: " + part;

            pos1 = pos2;
            pos2 = currentline.IndexOf(",", pos1 + 1);
            part = currentline.Substring(pos1 + 1, pos2 - pos1 - 1);
            label9.Text = "Model: " + part;


            pos1 = pos2;
            pos2 = currentline.IndexOf(",", pos1 + 1);
            part = currentline.Substring(pos1 + 1, pos2 - pos1 - 1);
            label10.Text = "Asset Tag: " + part;


            pos3 = currentline.IndexOf(';');
            if (pos3 > 0)
            {
                part = currentline.Substring(pos2 + 1, pos3 - pos2 - 1);
            }
            else
            {
                part = currentline.Substring(pos2 + 1);
            }
            part = part.Replace(",", "\r\n");
            textBox1.Text = part.ToString();
            
            reader.Close();
            
        }

        private void tabPage3_Enter(object sender, EventArgs e)
        {
            checkeditem = null;
            listBox2.Items.Clear();
            string readline;
            StreamReader reader = new StreamReader(filename);
            while (reader.EndOfStream != true)
            {
                readline = reader.ReadLine();
                if (readline.IndexOf(";inn") > 0)
                {
                    listBox2.Items.Add(readline.Substring(0,readline.IndexOf(',')));
                }

            }
            reader.Close();




        }

        private void button2_Click(object sender, EventArgs e)
        {

            if (checkeditem == null)
            {
                MessageBox.Show("Please select an item to check out", "No Item Selected", MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
            else
            {
                string[] items;
                items = new string[100];
                int counter = 0;
                int counter2 = 0;

                string line;
                StreamReader reader = new StreamReader(filename);

                while (reader.EndOfStream != true)
                {
                    items[counter] = reader.ReadLine();
                    counter++;

                }

                reader.Close();

                StreamWriter writer = new StreamWriter(filename);

                while (counter2 < counter)
                {

                    if (items[counter2].IndexOf(checkeditem.ToString()) > -1)
                    {
                        line = items[counter2] = items[counter2].Substring(0, items[counter2].Length - 4) + ";out;" + monthCalendar1.SelectionStart.ToString() + ";" + monthCalendar2.SelectionStart.ToString();
                        writer.WriteLine(line.ToString());

                    }
                    else
                    {
                        writer.WriteLine(items[counter2].ToString());
                    }
                    counter2++;


                }
                writer.Close();
                tabPage3_Enter(sender, e);
                checkeditem = null;
            }
            
        }

       

        private void tabPage4_Enter(object sender, EventArgs e)
        {
            checkeditem = null;
            listBox1.Items.Clear();
            string readline;
            StreamReader reader = new StreamReader(filename);
            while (reader.EndOfStream != true)
            {
                readline = reader.ReadLine();
                if (readline.IndexOf(";out") > 0)
                {
                    listBox1.Items.Add(readline.Substring(0, readline.IndexOf(',')));
                }

            }
            reader.Close();

        }

     

        private void listBox1_SelectedIndexChanged(object sender, EventArgs e)
        {
            checkeditem = listBox1.Text.ToString();
            string currentline, part;
            int pos;
            StreamReader reader = new StreamReader(filename);

            while (reader.EndOfStream != true)
            {
                currentline = reader.ReadLine();
                if (currentline.IndexOf(checkeditem) > -1)
                {
                    pos = currentline.LastIndexOf(";");
                    part = currentline.Substring(pos);
                    label22.Text = "Check in Date: " + part.Substring(1, 10);
                    currentline = currentline.Substring(0, currentline.Length - (currentline.Length - pos));
                    pos = currentline.LastIndexOf(";");
                    part = currentline.Substring(pos);
                    label21.Text = "Checked Out Date: " + part.Substring(1, 10);
                }

            }

            reader.Close();

        }

        private void listBox2_SelectedIndexChanged(object sender, EventArgs e)
        {
            checkeditem = listBox2.Text.ToString();
        }

        private void button3_Click(object sender, EventArgs e)
        {
            if (checkeditem == null)
            {
                MessageBox.Show("Please Select an Item to check in", "No Item Selected", MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
            else
            {
                string[] items;
                items = new string[100];
                int counter = 0;
                int counter2 = 0;

                string line;
                StreamReader reader = new StreamReader(filename);

                while (reader.EndOfStream != true)
                {
                    items[counter] = reader.ReadLine();
                    counter++;

                }

                reader.Close();
                StreamWriter writer = new StreamWriter(filename);
                while (counter2 < counter)
                {

                    if (items[counter2].IndexOf(checkeditem.ToString()) > -1)
                    {
                        line = items[counter2] = items[counter2].Substring(0, items[counter2].IndexOf(";out") - 1) + ";inn";
                        writer.WriteLine(line.ToString());

                    }
                    else
                    {
                        writer.WriteLine(items[counter2].ToString());
                    }
                    counter2++;


                }
                writer.Close();
                tabPage4_Enter(sender, e);
                checkeditem = null;
            }

        }

        private void Form1_Load(object sender, EventArgs e)
        {
            StreamReader reader = new StreamReader(filename);
            string[,] eventitems;
            eventitems = new string[100, 2];
            int counter = 0;
            int tempcounter = 0;
            int tempcounter2 = 0;
            string itemsoverdue;

            while (reader.EndOfStream != true)
            {
                eventitems[counter, 0] = reader.ReadLine();
                eventitems[counter, 1] = Convert.ToString(eventitems[counter, 0].IndexOf(";out"));
                counter++;
            }
            reader.Close();

            label1.Text = "Number of Items Listed: " + Convert.ToString(counter - 1);

            for (int a = 0; a < counter; a++)
            {
                if (Convert.ToInt32( eventitems[a, 1]) > 0)
                {
                    tempcounter++;
                    itemsoverdue = eventitems[a, 0].Substring(eventitems[a, 0].LastIndexOf(";") + 1, 10);
                    if (Convert.ToDateTime(itemsoverdue) < DateTime.Today)
                    {
                        tempcounter2 ++;
                    }



                }
            }
            label2.Text = "Number of Items In: " + Convert.ToString(counter - tempcounter);
            label3.Text = "Number of Items Out: " + Convert.ToString(tempcounter);


            
            label6.Text = "Number of Items Over Due: " + Convert.ToString(tempcounter2) ;


        }

        private void tabControl1_Selected(object sender, TabControlEventArgs e)
        {
            Form1_Load(sender, e);
        }
    }
}

so just wondering how I could optimise the code or if it is fine as it is.

It probably is fairly simple compared to others but I am still learning C# and it really is helping me to come to grips with the language.

With every project I seem to be learning something new and this is helping me a lot with my career in programming.

So guys please don't hold back if there is a better way of doing what I have done I would like to know.

If you require the project files they can be downloaded from here:

http://jellybeans.bulldoghome.com/ps/infoboxs/docstore/docedit.aspx?doc=15367&what=download

davetherave2 · 25 Mar 2007 at 16:06

Well I noticed a small error in the code for the combo box so have updated it slightly to reflect the change

Code:

private void button1_Click(object sender, EventArgs e)
        {
            string access;

            StreamWriter textwriter = new StreamWriter(filename, true);
            if (textBox6.Text == "")
            {
                access = "none";
            }
            else
            {
                access = textBox6.Text.Replace("\r\n", ",");
            }
                //access = textBox6.Text.Replace("\n", ",");
                textwriter.WriteLine(textBox2.Text.ToString() + "," + textBox3.Text.ToString() + "," +
                    textBox4.Text.ToString() + "," + textBox5.Text.ToString() + "," + access + ";inn");
                textwriter.Close();
            
            LoadAllCombos(filename);
           

        }

Now thinking about it I may have to add in a check to make sure all boxes are filled in otherwise there will be errors.

edit have updated this section again

Code:

 private void button1_Click(object sender, EventArgs e)
        {
            string access;
            if (textBox2.Text == "" || textBox3.Text == "" || textBox4.Text == "" || textBox5.Text == "")
            {
                MessageBox.Show("Not all values are entered", "No value Entered", MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
            else
            {
                StreamWriter textwriter = new StreamWriter(filename, true);
                if (textBox6.Text == "")
                {
                    access = "none";
                }
                else
                {
                    access = textBox6.Text.Replace("\r\n", ",");
                }
                //access = textBox6.Text.Replace("\n", ",");
                textwriter.WriteLine(textBox2.Text.ToString() + "," + textBox3.Text.ToString() + "," +
                    textBox4.Text.ToString() + "," + textBox5.Text.ToString() + "," + access + ";inn");
                textwriter.Close();

                LoadAllCombos(filename);
            }

        }

Eriedor · 25 Mar 2007 at 16:18

I haven't looked at the code but as long as the code is robust there is no reason to "optimise" it.

"Premature Optimisation is the Root of All Evil" - C.A.R. Hoare

davetherave2 · 25 Mar 2007 at 16:25

i guess what I am after is techniques that may help me with future projects that will aid better coding practises. I am still early on in my development of c# and now that I have a job as a junior developer I am trying to cram as much info that I understand in as quick a time as I can.

Mr^B · 25 Mar 2007 at 21:59

Hi,

First place to start is looking at variable/property/field names. counter/counter2/tempcounter/filename etc.

Make them meaningful, what is the counter counting? Why is a temp counter temporary? What is the file in filename, etc.

Second thing is to think in terms of objects - each item of equipment is an object, it has properties (name, weight, etc) it has methods (you can hire it, you can return it, etc).

Then think about re-engineering the code to make use of the objects.

Inquisitor · 25 Mar 2007 at 22:22

Just a minor aesthetic point, but the controls on your form seem to have inconsistent spacing between them. Use the snap feature in VS to align them for you

Una · 25 Mar 2007 at 22:24

eriedor said:
I haven't looked at the code but as long as the code is robust there is no reason to "optimise" it.

"Premature Optimisation is the Root of All Evil" - C.A.R. Hoare

Premature Optimisation is optimising it as you go along. He has written it already so he should be looking at ways to improve it. What Hoare was refering to was 9 times out of 10 the bottleneck in your code is no where you expect it.

toString · 25 Mar 2007 at 23:07

Not many comments.... you'll be a pro!

SiriusB · 26 Mar 2007 at 01:02

Comments and meaningful variable names are absolutely essential in any program no matter how small. If you plan to do this professionally then you need to learn to do it. You could be writing a small part of a huge project and if you don't comment and make things obvious, anyone else working on your code will have to waste time trying to work out what is going on.

A computing teacher once said to me that if a non-programmer can follow what is going on in your code, you have done well with your commenting!

Another missing thing is exception handling. In a small project it is probably not necessary but if it grows into something larger then it something you should include. Basically any piece of code should be put inside a try/catch/finally block.

This tells the compiler to try a piece of code, if it fails catch the exception and do something with it, then regardless of an error or not finally do something else.

eg:

try
{
/** some code here **/
}
catch(ExceptionType ex)
{
/** do something with exception **/
}
finally
{
/** do something regardless if there is an error or not **/
}

There is quite a lot to explain so it would be easier if you googled it by yourself

davetherave2 · 26 Mar 2007 at 08:21

well thanks for the comments I am going to recode some of the sections and see what the users think of it and then add in some of the ideas tht you have suggested.

cheers for the tips.

Wiggis · 26 Mar 2007 at 20:15

There are 2 basic types of optimization,
Performance optimization and code structure optimization.

Performance optimization is things like memory management (e.g. making sure there are no FileWriter or FleReader objects left open), where as code structure means tidying up the code by adding plenty of comments and exctracting areas of "common" code into separate methods so that it can be reused easily. A good example of this in your code is the FileWriter stuff, you could use a simple method that takes a filename and an array of String to be written out.
e.g.

Code:

public void writeMessages(String fileName, ArrayList messages)
{
  try 
  {
    StreamWriter writer = new StreamWriter(fileName);
    for (String msg : messages)
    {
      writer.writeLine(msg);
    }
  }
  catch (Exception e)
  {
     // do something here
  }
  finally
  {
    // makes sure the writer is always closed even if there is an error
    writer.close();
  }
}

Then you can call this method instead of having to use the same lump of code throughout your program.

Also, I noticed a line in your comboBox1_SelectedIndexChanged method that does not seem right although you may mean to do it.

Code:

            StreamReader reader = new StreamReader(filename);
            while (loop <= index)
            {
                currentline = reader.ReadLine();
                loop++;     
            }

Basically all this is going to assign the last line of the file to currentline. Did you mean this or was it supposed to process the line inside the loop or even just append the contents of ReadLine() onto the end of currentline, in which case you need

Code:

currentline += reader.ReadLine();