Counting Calories

I have this program for counting calories. There isn't a whole lot to it yet. It's a moderately sized project, so I'll do my best to make it as short as possible, posting only the main of the program and my global variables. It's a Windows Forms project.

Before anyone tells me, yes, I should fix the global variables. I'd like to take it one step at a time. I was thinking of implementing them locally and passing them around, but of course that will require some code reconstruction, so I was hoping for advice on the structure and overall cleanliness of the code first so I can move on from there.

As a side note, what is the best way to avoid global variables?

Variables:

```
public struct FoodRelated
{
//Variabled directly linked to food or the creation of the food table.

static Dictionary foodNameList = new Dictionary ();

public static Dictionary FoodNameList {
get {
return foodNameList;
}
set {
foodNameList = value;
}
}

static Dictionary servingSizeList = new Dictionary ();

public static Dictionary ServingSizeList {
get {
return servingSizeList;
}
set {
servingSizeList = value;
}
}

static Dictionary caloriesPerServingList = new Dictionary ();

public static Dictionary CaloriesPerServingList {
get {
return caloriesPerServingList;
}
set {
caloriesPerServingList = value;
}
}

static Dictionary definersList = new Dictionary ();

public static Dictionary DefinersList {
get {
return definersList;
}
set {
definersList = value;
}
}

static readonly float totalCaloriesPerDay = 2140f;

public static float TotalCaloriesPerDay {
get {
return totalCaloriesPerDay;
}
}

public static float Calories {
get;
set;
}

}

public struct GlobalVari

Welcome on Code Review!

By convention, your namespace shouldn't contain underscores, you should split them with . if it is neccessary (which isn't in your case I think)

Weight_Watching_Program -> WeightWatchingProgram

Your members don't have the accessibility modifier written, by default it is private but I like to believe it is easier to read when it is always there so other devs don't have to think about if you forgot it or not. (You might say you're the only one on your project, I don't know, though you should always code with the idea that someone else might review your code like.... Right now!)

All your "classes" are declared as struct at the moment, and you shouldn't do it. Structs are for lightweighted (immutable) objects. If you want more information, read this : http://msdn.microsoft.com/en-us/library/0taef578.aspx

Your properties are declared this way :

static Dictionary definersList = new Dictionary ();

public static Dictionary DefinersList {
    get {
        return definersList;
    }
    set {
        definersList = value;
    }
}

In C#, you can use auto-properties to make it a little shorter. Like this :

public static Dictionary DefinersList { get; set; }

But then you have a problem, you never set your property. Remember that in "your" way, you create a new instance of Dictionary when you declare your variable.

To solve this, you should have a constructor in your class that sets your properties. *Note that this won't work if you keep on using a struct since you can't declare an empty constructor.

public FoodRelated()
{
    DefinersList = new = new Dictionary ();
    //Do this for all your properties
}

In my opinion, this method has the advantage of keeping all your declaration in the same place (the constructo) instead of spreading them all across your code!

Your static readonly string should be a const string it it should never change. This StackOverflow answer has some good information about it : https://stackoverflow.com/questions/755685/c-static-readonly-vs-const

Also, it is easier to read your code if you put all the members at the top of your class followed by the properties instead of mixing them up. This SO answer as it covered so I won't repeat it all : https://stackoverflow.com/questions/603758/whats-the-best-way-to-layout-a-c-sharp-class.

In the C# convention, you should put brackets on a new line. My explanation might not be the best so here's an example :

if (true){ //not cool
}

if(true) //Pretty cool
{
}

You wrote way too much code in here : Equals (deleteSelectedFoodItemButton.Enabled, true) a bool is by default... a bool! Consider that Equals returns true if deleteSelectedFoodItemButton.Enabled equals true, you could shorten it to deleteSelectedFoodItemButton.Enabled == true.

But wait, there's more! Since deleteSelectedFoodItemButton.Enabled itself is a boolean, you could shorten it to : deleteSelectedFoodItemButton.Enabled. (Excuse the poor readability of this paragraph!)

if (Equals (deleteSelectedFoodItemButton.Enabled, true) && foodList.Items.Count <= 0)

then becomes :

if (deleteSelectedFoodItemButton.Enabled && foodList.Items.Count <= 0)

You can do the same thing with Equals (deleteSelectedFoodItemButton.Enabled, true) by using the ! operator.

if (Equals (deleteSelectedFoodItemButton.Enabled, false))

becomes :

if (!deleteSelectedFoodItemButton.Enabled)

In the same idea, if (Equals (labelToChange, null)) could be changed to

if (labelToChange == null)

In your code, you often split the method call from the () with a space, you don't need to do this, it just takes some spaces for nothing. This is what I mean :

Validation.checkCurrentRadioButton /*space here that should be removed*/ (timeRadioButton, calorieRadioButton, caloriesLabel);

I see you use String.IsNullOrEmpty, but have you considered that if I was to enter only white spaces in your code, it would pass this input validation. If it is not the desired behavior, you can use String.IsNullOrWhiteSpace.

Finally, you should not put everything static in a class that isn't static itself. But in this situation, I think that most of your code shouldn't be static. When you put something static, ask yourself if it needs to be used by all instances of your class, in this situation your static code is the whole content of your class, so you shouldn't set this to static. You should declare instances of your class instead.

StackExchange Code Review Q#73820, answer score: 10