Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I want to create a class that has properties that can be set from one form, and accessed by any other. I have created the class below which works perfectly, but I'm not sure if this is considered bad class design.

This is just an example of how I would use it.

//The class is its own file
class Variables
{
    public static string inspectorID = "";
    private string InspectorID
    {
        get { return inspectorID; }
        set { inspectorID = value; }
    }

    public static string inspector ="";
    private string Inspector
    {
        get { return inspector; }
        set { inspector = value; }
    }
}

Setting properties from Form 1

    private void btnOk_Click(object sender, EventArgs e)
    {
            Variables.inspector = "John" 
            Variables.inspectorID = "100"
    }

Getting properties from form 2

    private void btnOk_Click(object sender, EventArgs e)
    {
            txtInspector.Text = Variables.inspector;
            txtInspectorID.Text = Variables.inspectorID;
    }
share|improve this question

closed as off-topic by ckuhn203, syb0rg, palacsint, Mat's Mug, vnp Jul 9 at 19:00

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – ckuhn203, syb0rg, palacsint, Mat's Mug, vnp
If this question can be reworded to fit the rules in the help center, please edit the question.

1  
Are the 2 forms visible at the same time? Who is controlling what form gets displayed? –  Mat's Mug Feb 18 at 14:49
2  
Is there more than one instance of Form 1 and/or Form 2 in existence at one time? Is Form 2 a child of Form 1, and/or created after or created by Form 1? –  ChrisW Feb 18 at 14:57
    
Form 1 and Form 2 are not up at the same time and there can never be two of any form at one time. –  CodeSlinger Feb 18 at 15:10
    
The code above is just an example of how I could use the properties. In the original code its nothing like this. –  CodeSlinger Feb 18 at 15:19
7  
Please include your actual code, if your code is nothing like what you've posted, it's going to be hard to write a helpful review :) –  Mat's Mug Feb 18 at 15:22
add comment

4 Answers 4

up vote 7 down vote accepted

You have several design issues here.

  • @ChrisW's answer identifies one: your Variables class exposes its fields and defines useless instance properties.
  • You have logic that's not presentation logic, coded inside Click event handlers.

You're not providing lots of context regarding how the forms are displayed, so I'm going to assume the below suggestion can work for you.

A form is a class, like any other: you can define and expose properties - I would encapsulate the form's "model" in its own class, and expose it as a property:

public class MyModel
{
    public string InspectorName { get; set; }
    public string InspectorId { get; set; }
}

public class Form1 : Form
{
    public Form1()
    {
        InitializeComponents();
    }        

    public MyModel Model { get; set; }

    private void btnOk_Click(object sender, EventArgs e)
    {
        UpdateModelValues();
    }

    private void UpdateModelValues()
    {
        Model.InspectorName = "John";
        Model.InspectorId = "100"; // shouldn't this be an int?
    }
}

Then, the code that owns the instance of Form1 can do this:

var form1 = new Form1();
var model = new MyModel { InspectorName = "Jane", InspectorId = "101" };
form1.Model = model;
form1.ShowDialog();

And if you did the same with Form2 you could then do this:

var form2 = new Form2();
form2.Model = form1.Model;
form2.ShowDialog();

Given that Form2 looks something like this:

public class Form2 : Form
{
    public Form2()
    {
        InitializeComponents();
    }        

    public MyModel Model { get; set; }

    private void btnOk_Click(object sender, EventArgs e)
    {
        UpdateValuesFromModel(); // is this code really in a "Ok button" click handler?
    }

    private void UpdateValuesFromModel()
    {
        txtInspector.Text = Model.InspectorName;
        txtInspectorId.Text = Model.InspectorId;
    }
}

...Which essentially boils down to the other two answers: drop the static and start passing instance data between your forms, and the code that controls the form instances also controls the data these forms are playing with ("model").

share|improve this answer
    
No. The code that I showed was just an example of how I could get/set the properties. The problem that I see is that I only create 1 instance of the class. Unlike other classes that you would create an instance of the class every time you need to access it. I guess I'm rusty on design patterns and OOP in general –  CodeSlinger Feb 18 at 15:17
2  
Not sure I understand what you're saying, but "The problem that I see is that I only create 1 instance of the class" - to me that's not the problem, it's the solution! - the two forms need to share an instance of a class that holds the data you need to be shared between them. –  Mat's Mug Feb 18 at 15:27
    
Will my solution cause problems? –  CodeSlinger Feb 18 at 15:30
2  
Immediate ones, not necessarily. But what you're describing is pretty much a singleton (public, static, single-instance), which is an anti-pattern. –  Mat's Mug Feb 18 at 15:34
    
wow great info! I guess I need to brush up on design pattern best practices. What is your solutions pattern? –  CodeSlinger Feb 18 at 15:40
show 1 more comment

The approach you're taking here creates a static coupling between your forms. This has a few negative consequences:

  • You can't test the forms in isolation from each other
  • You can't open multiple copies of the forms and have them communicating independently from each other
  • When you make a change in any form it must be verified that it does not have a negative impact on any other form.

Also your use of variables as a drop-off point for data has the disadvantage that your other forms need to check to see if the data they need is there, and will not update dynamically if it changes.

I would take a much more dynamic approach. If one form produces data that other forms need to be able to access, I would use the Observer Pattern. This means that the observers need to be able to get a reference to the form that produces the data. In order to do this, I would consider using Dependency Injection. By making these changes you would achieve the following improvements:

  • Ability to handle dynamically updating data
  • Removing statically coded dependencies allowing you to more easily redefine the system if you need a different pattern of communication (E.g. to open up a second channel so you can have two groups of forms showing different data).
share|improve this answer
add comment

The way I would think about designing this is to approach it from the standpoint of which form is controlling the data you are trying to share. From your sample code, it looks like an "Inspector" should likely be made into its own class and passed between forms as an object.

There are a couple ways you can go about doing this, but I think @ChrisW hit the nail on the head when he suggested that you just pass instance data between the forms. Remember, a form is a class like anything else in C#, so you can just add a custom property to the Form2 (etc.) and just pass the parameters you need when you instantiate the form.

If the objects you need to pass around are more complex or if you need to track the state of different forms, I would consider creating a forms controller. Instead of creating sub-forms from Form1, you can raise a custom "open form" event that passes the object in the EventArgs. The controller can then open the correct subform and pass any parameters that it needs.

share|improve this answer
    
There are many forms that need to access this information. Not just form 2. Form 1 is just a place in the program for the user to tell the program who they are. then that information is used in almost every query to the databse. Including inserts. –  CodeSlinger Feb 18 at 15:13
    
I would then have to pass the object to every form that I initiate. I was trying to avoid doing this because there are many other properties that need to be passed as well. I figured I could use a class similar to a Module in Visual Basic –  CodeSlinger Feb 18 at 15:27
1  
Well, then look at your design from the standpoint of where to data is consumed. If it is being consumed by the database, it likely belongs in a data access type class. –  Comintern Feb 18 at 15:42
add comment

You're not using the private properties at all (and cannot use them because they are private).

You might as well redesign the class as:

// static class contains no instance data, only static data
static class Variables
{
    public static string inspectorID = "";
    public static string inspector ="";
}

Because the inspector data are static there is only one of them. If you have more than one instance of form 1 (unlikely in Windows Forms, more likely in ASP.NET), then the two forms would be writing to the same (static) data variables.

An alternative way to code what you've done is using one of the many types of singleton ...

class Variables
{
    public string inspectorID = { get; set; }
    public string inspector { get; set; }
    public static Variables singleton = new Variables();
}

... whose properties you can access like this ...

Variables.singleton.inspector = "John"

It's not clear to me that a singleton like this is better than multiple static data in a static class (unless you want to do something complicated, such as give the singleton virtual methods and instantiate a subclass of it).

share|improve this answer
2  
Although this is possible, I wouldn't use static data just because it's easy. I think that using non-static variables would be preferred here. –  Simon André Forsberg Feb 18 at 14:36
    
@SimonAndréForsberg Can you explain how to share instance data (e.g. a Variables object with non-static members) between forms? –  ChrisW Feb 18 at 14:38
1  
Use dependency injection. If you really have to you could use a singleton pattern, but I believe injecting an instance of the variables class is prefferred –  Simon Feb 18 at 15:00
    
This is just an example. I have many more properties that I use in this manner. Some get set more then once based on what the user is doing. Its a way for me to access important pieces of information from any form that needs it. –  CodeSlinger Feb 18 at 15:21
add comment

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