locked
Declaring a C# Class Object in a Form RRS feed

  • Question

  • Is it okay to place "view = new Person" within the button event handler?  If I don't, and I place it within the Form1 constructor, only my last value gets added. Is that the proper way if I want to declare a new instance and then add it to my Arraylist?
    using System;
    using System.Collections.Generic;
    using System.ComponentModel;
    using System.Data;
    using System.Drawing;
    using System.Linq;
    using System.Text;
    using System.Windows.Forms;
    using System.Collections;
    
    
    namespace PersonDisplay
    {
        public partial class Form1 : Form
        {
    
            private Person view;
    
            private ArrayList store;
    
            public Form1()
            {
                InitializeComponent();
                store = new ArrayList();
            }
    
            private void Form1_Load(object sender, EventArgs e)
            {
    
            }
    
            private void button1_Click(object sender, EventArgs e)
            {
    
                //Is it okay to declare a new instance of the Person class with each button push?
                view = new Person();
                view.firstname = txtFirstName.Text;
                view.lastname = txtLastName.Text;
                store.Add(view);
                txtFirstName.Clear();
                txtLastName.Clear();
                
    
            }
    
            private void button2_Click(object sender, EventArgs e)
            {
                foreach (Person display in store)
                {
                    MessageBox.Show(display.ToString());
                }
            }
        }
    }


    • Edited by Oasisfactor Tuesday, August 14, 2012 6:52 PM
    Tuesday, August 14, 2012 6:49 PM

Answers

  • It's technically fine to do this.  If you want each button press to make a new "person", then you'd want to do it this way.

    The main changes I would suggest would be to use List<Person> instead of ArrayList, and declare the Person object locally (since you're not using it outside).  This would look like:

    using System;
    using System.Collections.Generic;
    using System.ComponentModel;
    using System.Data;
    using System.Drawing;
    using System.Linq;
    using System.Text;
    using System.Windows.Forms;
    using System.Collections;
    
    
    namespace PersonDisplay
    {
        public partial class Form1 : Form
        {
            private List<Person> store;
    
            public Form1()
            {
                InitializeComponent();
                store = new List<Person>();
            }
    
            private void Form1_Load(object sender, EventArgs e)
            {
    
            }
    
            private void button1_Click(object sender, EventArgs e)
            {
                // Declare the Person here, since it's only used here
                Person view = new Person();
                view.firstname = txtFirstName.Text;
                view.lastname = txtLastName.Text;
                store.Add(view);
                txtFirstName.Clear();
                txtLastName.Clear();            
    
            }
    
            private void button2_Click(object sender, EventArgs e)
            {
                foreach (Person display in store)
                {
                    MessageBox.Show(display.ToString());
                }
            }
        }
    }


    Reed Copsey, Jr. - http://reedcopsey.com
    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful".

    Tuesday, August 14, 2012 6:55 PM

All replies

  • The scope of the new "Person" object should be within the button click event, yes.  It is incorrect to make it a class variable, as you have done here.

    You shouldn't use "ArrayList".  It is deprecated.  You should use the "List" class, in this case, you should use a "List<Person>".  It will be type safe, so that you won't need to worry about shooting yourself in the foot by adding something that's not a Person, you won't need to worry about what types should go in any given list, and you also don't need to go around casting everything that you pull out of it.  (There are also other benefits, but those are the big ones).

    Tuesday, August 14, 2012 6:53 PM
  • It's technically fine to do this.  If you want each button press to make a new "person", then you'd want to do it this way.

    The main changes I would suggest would be to use List<Person> instead of ArrayList, and declare the Person object locally (since you're not using it outside).  This would look like:

    using System;
    using System.Collections.Generic;
    using System.ComponentModel;
    using System.Data;
    using System.Drawing;
    using System.Linq;
    using System.Text;
    using System.Windows.Forms;
    using System.Collections;
    
    
    namespace PersonDisplay
    {
        public partial class Form1 : Form
        {
            private List<Person> store;
    
            public Form1()
            {
                InitializeComponent();
                store = new List<Person>();
            }
    
            private void Form1_Load(object sender, EventArgs e)
            {
    
            }
    
            private void button1_Click(object sender, EventArgs e)
            {
                // Declare the Person here, since it's only used here
                Person view = new Person();
                view.firstname = txtFirstName.Text;
                view.lastname = txtLastName.Text;
                store.Add(view);
                txtFirstName.Clear();
                txtLastName.Clear();            
    
            }
    
            private void button2_Click(object sender, EventArgs e)
            {
                foreach (Person display in store)
                {
                    MessageBox.Show(display.ToString());
                }
            }
        }
    }


    Reed Copsey, Jr. - http://reedcopsey.com
    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful".

    Tuesday, August 14, 2012 6:55 PM
  • The correct way would be to place Person view = new Person directly in the button event handler like so?

    using System;
    using System.Collections.Generic;
    using System.ComponentModel;
    using System.Data;
    using System.Drawing;
    using System.Linq;
    using System.Text;
    using System.Windows.Forms;
    using System.Collections;
    
    
    namespace PersonDisplay
    {
        public partial class Form1 : Form
        {
    
            
    
            private ArrayList store;
    
            public Form1()
            {
                InitializeComponent();
                store = new ArrayList();
            }
    
            private void Form1_Load(object sender, EventArgs e)
            {
    
            }
    
            private void button1_Click(object sender, EventArgs e)
            {
    
                //Is it okay to declare a new instance of the Person class with each button push?
                
                Person view = new Person();
                view.firstname = txtFirstName.Text;
                view.lastname = txtLastName.Text;
                store.Add(view);
                txtFirstName.Clear();
                txtLastName.Clear();
                
    
            }
    
            private void button2_Click(object sender, EventArgs e)
            {
                foreach (Person display in store)
                {
                    MessageBox.Show(display.ToString());
                }
            }
        }
    }

    Whats wrong with making it a class variable? Security issue?

    Thanks for the tips :D I was aware that ArrayList isn't ideal, I was just playing around with it.



    • Edited by Oasisfactor Tuesday, August 14, 2012 6:57 PM
    • Proposed as answer by Lisa Zhu Thursday, August 16, 2012 7:06 AM
    • Unproposed as answer by Lisa Zhu Thursday, August 16, 2012 7:06 AM
    Tuesday, August 14, 2012 6:57 PM
  • "Whats wrong with making it a class variable? Security issue?"

    The main issue here is that you're going to be holding onto the Person object in between calls, and you don't need to.  This suggests that you're using the "Person" in other methods, and hurts maintainability.  In your specific case, it probably didn't hurt anything, but it served no purpose, which means it's not good to do.


    Reed Copsey, Jr. - http://reedcopsey.com
    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful".

    Tuesday, August 14, 2012 7:00 PM
  • May be this appear to be off-topic but it is good to use MVC 

    http://social.msdn.microsoft.com/Forums/da-DK/csharplanguage/thread/564cc0d4-e115-4e02-afae-5a17eea573c6


    Regards,
    Ahmed Ibrahim
    SQL Server Setup Team
    My Blog
    This posting is provided "AS IS" with no warranties, and confers no rights. Please remember to click "Mark as Answer" and "Vote as Helpful" on posts that help you.
    This can be beneficial to other community members reading the thread.

    Tuesday, August 14, 2012 7:01 PM
  • Thank You!
    • Edited by Oasisfactor Tuesday, August 14, 2012 7:02 PM
    Tuesday, August 14, 2012 7:02 PM