Yahtzee Game - Feedback

[skybomb0]

Well, I have been learning C# of and on for a little over a month now and I decided to try a simple game. Yahtzee shouldn't be too bad; just roll the dice and check for your score, right? Actually, the complexity of it turned out to be much deeper than I thought. Because of this, I ended up adding workarounds to my code because my first ideas were accurate. This caused my coding logic to become a little scrambled.

Since I can see a lot of room for improvement, I will probably be rewriting the code, but I wanted to see what everyone thought of it first.

Of course, do you see any bugs?
Is there any coding standards that I am unaware of?
And finally, how would you have designed it? What classes would you have used. I had trouble with this because I was taught classes represent nouns, but soon found out this is not necessarily true.

Thanks for your feedback.

This project was made in C# Express 2010 and I have uploaded the project here.

[ddanbe]

Well, the web page does not open...
Besides, if you are just learning to program, I would probably not start with a game like Yahtzee.

[skybomb0]

The link is working fine here. It is a direct link to a zipped file with the project inside. There shouldn't be any problems.

Here is a mirror of the zipped file uploaded to mediafire.

[Ketsuekiame]

Ok, you've hit one of my pet hates...
Please don't use underscore on the beginning of a method name. =p

You've created a GlobalVariables class...Sure these could go into a more specific class, such as a Player class?

Try and stay away from Globals as much as possible, if you really MUST have globals, look into using a Singleton Pattern.

Other than that I can't see much wrong with it at a quick glance.

[skybomb0]

I figured the global variables were probably not a good idea, but that was the easiest way I saw of storing application settings. Recently, I found out how to use the app.config file which should remove the global variables. The handles to the forms in the global variables were a result of preloading the forms during startup. The underscore at the beginning of the method names is a result of automatically inserting an event in c# express. I will change that too. Thank you for your input.

[Ketsuekiame]

The App.Config should contain only pre-initialisation configuration values. Good things to put in there are values such as your save file location, database provider type etc. Things that can change and changing them won't cause your application to break or be "cracked"

Bad things to put in there are values for username and password, SQL Connection strings (Unless it's integrated security) and options that change often.

You shouldn't exchange and use these as global variables. Using global variables tends to break Object Oriented design. To make it a better design, figure out which class the values most likely apply to and place them in there.

In the case that they really are global, you should consider your original design and if applicable, use a singleton class.

A singleton class should not contain only Properties, but also methods and it should be a class with a defined purpose in your application, not just a dumping ground for variables ;)

As for your naming conventions, I personally use the following (many people will disagree with this but I like it):
Private Variable: _variableName (underscore followed by lower-case. Separate words are initially capitalised)
Public Variable: VariableName (Begins with an upper-case letter. Separate words are capitalised)
Local Variable (Method Scope only): variableName (Begins with lower-case, no symbols used. Separate words are capitalised)
Method Names: MyMethod(Class myVar) (Follows same rules as public methods, properties follow Local Variable rule)
Test Methods: My_Test_Method() (Similar to Method Names rule, however, each word is separated by an underscore)
Form Controls: lblMyLabel tbxTextBox (This is a form of Hungarian Notation. This is the ONLY place I use it, because it was useful in .Net 1.1/2.0 before they had the "is" keyword. For backwards compatibility I still use it)

Obviously you don't *have* to use that naming convention but I find it extremely "pleasing" to read and is a sufficient method of identifying what something is (access levels also) before reading the full name to show what it contains.

[skybomb0]

I don't think these variables are truly global. They are user-defined options that will be used by 2 or 3 classes. I plan to incorporate an options menu allowing the user to switch options off and on during the game. Would user options be something that would go in app.config? If not, what other options are there?

As far as naming standards, I will try to adapt to your preferences. Since I haven't been programming very long (at least not in an object oriented language), I don't have any real naming habits so it won't be difficult to change. I also need to be a bit more consistant in my naming.

[karylm]

My C# instructor mentioned yesterday that he plans to include a Yahtzee project before the end of the quarter. I decided to look around a bit to get some UI inspiration and came across your game. I can't speak to the overall programming (haven't even really looked at it beyond removing the NUnit components so it would run), but the UI is great! Definitely the best of the three C# games I found.

One problem: I thought it odd that in the entire game I played, I never rolled any sixes. Easily fixed: Random's minValue is inclusive, but the maxValue is exclusive. I changed the relevant code to (1, 7) instead of (1, 6) and all is well.