views:

251

answers:

7

I have GUI that allows an user to create and modify a point object. I need to store a list of these points to insert at a specific drawing.

Here's how I came up with it:

In the form code, I opened a private property List<Points> and I manipulate it directly inside form code. Is this the correct way to handle?

Something like:

public partial class TesteInterface_AdicionarVertice : Form {

   public List<VerticeDNPM> listaVertices;

   public TesteInterface_AdicionarVertice()
   {
      InitializeComponent();
      listaVertices = new List<VerticeDNPM>();
   }

}

So, what do you think about this design? Is there a better way to do it?


Thanks for all thoughs.

I will make the list read-only. thanks for that idea.

The real thing here is this: I have a button which creates points, and another that creates polygons from points.

I need to have a way to get the List of points at the time the user chooses to create that certain polygon. That is basically what i am asking :P

I though to use a class property (in this case List) to store temp points, until the user creates the polygon. Is this a valid approach?

A: 

Sorry for that. The correct code is:

public partial class TesteInterface_AdicionarVertice : Form {

    public List<VerticeDNPM> listaVertices;

    public TesteInterface_AdicionarVertice()
    {
        InitializeComponent();
        listaVertices = new List<VerticeDNPM>();
    }

}

George
edit your question instead of creating an answer
Francis B.
Edited it for you, but please remove this. It's customary to edit your answer and leave a comment to say what you've done.
C. Ross
A: 

I'm not really sure what you are asking. Aside from moving your non-UI code out of the UI, I would change the list creation to a readonly field like so:

public partial class TesteInterface_AdicionarVertice : Form {
    private readonly List<VerticeDNPM> listaVertices = new List<VerticeDNPM>();
    public List<VerticeDNPM> Vertices {get; set;}; 

    public TesteInterface_AdicionarVertice()
    {
        InitializeComponent();       
    }
}
John Kraft
The generic List<> you are referring to is defined in the System.Collections.Generic namespace so you might be missing an "using" clause.
Darin Dimitrov
@darin - the problem was in the answer markdown; not the actual code. I was using the <pre><code> tags. For some reason they cause the generics to not be visible in the post. I took them out and they display now.
John Kraft
A: 

Assuming that you include the appropriate namespace inclusions and class definitions, then what you have posted is valid and does not clearly violate any best practices (unless you count naming conventions, in which case VerticeDNPM should be VerticeDnpm according to Microsoft's naming guidelines). However, in order critique your approach from a design standpoint, you'd really need to provide more information.

Nimrand
+1  A: 

I wouldn't make your list public. Then you never know who is modifying it. Make your list private, then expose it as read only.

You can read more about it here. You can also run your code through FxCop. I'm sure it would pick this up.

Aaron Daniels
A: 

Thanks for all thoughs.

I will make the list read-only. thanks for that idea.

The real thing here is this: I have a button which creates points, and another that creates polygons from points.

I need to have a way to get the List of points at the time the user chooses to create that certain polygon. That is basically what i am asking :P

I though to use a class property (in this case List) to store temp points, until the user creates the polygon. Is this a valid approach?

George
I have copied the content of this follow-up "answer" to the question. Please delete this.
Wim Coenen
+1  A: 

I agree that making your list public is a bad idea as then a consumer of the class can modify the actual list object itself, which is not what you want. Instead you want to expose it as a read-only property thereby allowing consumers to access the list contents.

public partial class TestInterface_ADicionaryVertice : Form 
{
    private List<VerticeDNPM> listVertices = new List<VerticeDNPM>();
    public List<VerticeDNPM> { get { return listVertices; } }

    public TestInterface_ADiciontaryVertice()
    {
        InitializeComponent();
        ...manipulate list of points here...   
    }
}

In this way you are modifying a private list of points in your code while still allowing a consumer (presumably something that receives the form as a parameter?) to access the list of points and read through it.

Two additional thoughts: 1) If the only consumer is deriving from this form instead of operating on it (receiving it as a parameter somewhere) then consider making the list protected instead. 2) If you do not want the list to be modified by any consumer (i.e. the list of points can't change once you're done manipulating them) then consider exposing an enumerator for the list instead of the list itself. This way someone can enumerate the points but can't change them.

Example: public IEnumerator GetPoints { get { return listVertices.GetEnumerator(); } }

Stuart Thompson
I would expose an IList<T>, IEnumerable<T>, ReadOnlyCollection, etc. instead of a List<T>
Ed Swangren
A: 

If you give a user access to a read-only property and return the original list, then the a consumer could still modify that list. The List is still a reference type, so the property is returning a pointer to the list. A true read-only property will create a copy of the list inside the 'get' and return that instead.

alexD