Being very new to coding, I have just finished a very basic demo of an RPG project I've been working on. First revisions were just hacking away at some code, but when I found Nicks Tile Engine Tutorial series I modified a lot of what I was doing to impliment some of his code, was an amazing way to learn new ways of doing things.

I would love some feedback on what I have written.. Other thought processes on how to solve a problem I ran into.. Maybe something I did that was totally a "Why would you do THAT?!" Basically I'm looking to improve as a programmer.

Now that the basic basic demo is done, I am taking a small break to re-read a few books and learn a few techniques that gave me troubles. Top of my list is the design patterns book a few people recommended, and reading up on reflection and a few other things. if you have any advice, words of wisdom, or questions, PLEASE let me know as it helps me to learn :)

Source could be found at
http://code.google.com/p/firstxnarpg/source/browse

and if you wanted to play around with it, I uploaded a .rar with everything you need. Things are still buggy but thats to be expected :)

Thanks for the help everyone, and also thanks Nick for answering MANY of my questions in your blog hah :D

I just look into your code and test the game for a while. Here there are my first impressions:

• You have strange things in your code such as "(float)Math.Max(value, 0f)", being "value" a float, or the public SpellType enum defined within the public Spells class (why?).
• The Spells class is also an example of a type with a "bad" name: it uses a plural name although represents single objects (BaseObjects is another case).
• You have methods with improvable names, like Inventory.AddItemToInventory (the <MyType> name mustn't appear into methods of the <MyType> type).
• You do explicit string conversions (fps.ToString() in FPSCounter).
• Base shouldn't be used to name base classes (BaseCharacter should be named Character).
• Using a static class for managing input may not be a good idea. Commonly, the Singleton pattern is a best way for doing that things.
• I recomend you to always use the brackets to surround code blocks. Is more readable.
• Do not import a namespace if you're not going to use it.
• Etc, etc, etc.

Don't worry about all these things, I think you've done a VERY GOOD job if you're really "very new to coding". Congratulations! In addition, you have also good things, like the way you manage game screens. You have used something similar to the State pattern :D

Regarding the game graphics, I think rock walls must emulate a 3D appearance. They look flat and they confused me, hehe.

As I said, these are my FIRST IMPRESSIONS. I only tested your game for a while. Take this into account.

The Spells class is also an example of a type with a "bad" name: it uses a plural name although represents single objects (BaseObjects is another case).

Yeah I can see how that could get confusing if I'm working with other people.. mental note, noted :)

You do explicit string conversions (fps.ToString() in FPSCounter).

Actually the FPS counter was a downloaded component from, I forget where I got it from, keep meaning to write my own. How should I be doing the conversion?

Using a static class for managing input may not be a good idea. Commonly, the Singleton pattern is a best way for doing that things.

I don't know what that is, but this week sometime I am going to goto the book store and pick up that design patterns book, so I'll learn about it :)

Don't worry about all these things, I think you've done a VERY GOOD job if you're really "very new to coding". Congratulations! In addition, you have also good things, like the way you manage game screens. You have used something similar to the State pattern :D

Thanks. I got the idea from the beginning C# Game Programming book from Ron Penton and just did some poking around and tried a few things, and thats what I came up with. And yes, I am very new to programming, but I'm going into it full force because I'm trying to change career directions. Picked up my first programming book a few months ago and really have not looked back.

Regarding the game graphics, I think rock walls must emulate a 3D appearance. They look flat and they confused me, hehe.

I have to thank Nick for that one, since I got those textures from watching his tile engine tutorial series :D I'm a good artist, but not computer generated art... give me a scratchboard or a pencil and I can draw you somethin though!

Thanks for the feedback. No feedback is bad feedback... blast away. Actually you can see in the code on how I changed based on my readings and research.. for example the characters stats are hard coded but the enemies are read in from a text file. I couldn't have learned what I know without a community like this, the information is priceless to me :) Thanks!

Cool cool! I must say that the game was way too easy and didn't have a point, geez, lol j/k

Um, here's a few first thoughts on some ideas:

Main menu/Title screen
Why have "the most worthless title screen ever", when you could make it do something absolutely useful.. like, show the controls! Took me a bit to find the buttons used for the game!

Um.. besides that.. I don't have much else. I'm pretty new to this as well, so :D

I really liked what you have done though! Keep it up!

CraigGiles:

You do explicit string conversions (fps.ToString() in FPSCounter).

Actually the FPS counter was a downloaded component from, I forget where I got it from, keep meaning to write my own. How should I be doing the conversion?

Let the compiler do the work for you: concatenating an string with anything results on a new string. Change "FPS: " + fps.ToString() by "FPS: " + fps. The result is the same as you did, but more readable.

Warren:

Main menu/Title screen
Why have "the most worthless title screen ever", when you could make it do something absolutely useful.. like, show the controls! Took me a bit to find the buttons used for the game!

hehe whoops :) For future reference if anyone is going to download to test or stroll through the code, the wiki page has the information on controlls;
http://code.google.com/p/firstxnarpg/wiki/ShackRPGDemo

But yes, good call... i'll put that on the title screen portion. My first thought was to do an introduction animation on that game screen, but I just never got around to doing that.

Abel García Plaza:

Using a static class for managing input may not be a good idea. Commonly, the Singleton pattern is a best way for doing that things.

Can you explain this for me? In my mind the only difference between a static class and the singleton pattern is that the singleton instantiates a single instance which can be retrieved through a static method or property. Then all data is stored as instance data instead of static data. To me, then, the only advantage of the singleton pattern is that you can null out the instance to clear memory. But with something like input where it's a rather small memory foot print and likely to be used throughout the game's lifetime, I don't see any advantages in the singleton pattern.

 In fact it's worse in my opinion because instead of:

InputHelper.DoSomething()

I now have to do

InputHelper.Instance.DoSomething()

Both yielding the same result.

Nick Gravelyn:

Abel García Plaza:

Using a static class for managing input may not be a good idea. Commonly, the Singleton pattern is a best way for doing that things.

Can you explain this for me? In my mind the only difference between a static class and the singleton pattern is that the singleton instantiates a single instance which can be retrieved through a static method or property. Then all data is stored as instance data instead of static data.

Here's what the Micorsoft C# docs have to say on the matter.
http://msdn2.microsoft.com/en-us/library/ms998558.aspx

dteviot:

Nick Gravelyn:

Abel García Plaza:

Using a static class for managing input may not be a good idea. Commonly, the Singleton pattern is a best way for doing that things.

Can you explain this for me? In my mind the only difference between a static class and the singleton pattern is that the singleton instantiates a single instance which can be retrieved through a static method or property. Then all data is stored as instance data instead of static data.

Here's what the Micorsoft C# docs have to say on the matter.
http://msdn2.microsoft.com/en-us/library/ms998558.aspx

That gives a great description on how to implement a singleton, but what I want to know is why should the OP use a singleton. I already know how to create a singleton class, but what I'm after is what are the benefits of using a singleton overjust having static data? Like I said above, I can only think of the memory benefits of being able to kill your instance, but perhaps I'm missing something else.

Some reasons I can think why a singleton might be interesting:

• It separates the way you use the object instance from the fact that this instance was originally created as a singleton. This could give you more future proofing if you someday find a reason to support more than one of these objects.
• Opens up the possibility (assuming you have suitable extension points) of providing a mock implementation of that singleton while unit testing.
• Access to instance fields is (slightly) faster than static fields in .NET

That said, I don't agree that using statics is bad practice, and in fact I would probably use statics rather than a singleton myself in this case.

It seems to me that a lot of time when people use singletons, they really just want a global or static, but they've bought into the OO religion that says "globals are evil", so they use singletons as a kind of trick to make what looks like an OO class instance behave more like a global. In which case why not just admit it and use a real global?

There are certainly cases where the singleton pattern makes sense, but I personally think they're a lot rarer than many people assume (at least when you're using a language that supports true globals/statics: remember the singleton pattern was originally invented for Smalltalk, which has no such construct).

Shawn Hargreaves:

I don't agree that using statics is bad practice

I think that using statics is bad practice if you're not an experienced guy, and he said to be "very new to coding". Unexperienced people don't know that when using statics you're paying the price of losting some OOP features that you might need in the future. So I emphasize that statics aren't the good way for a beginner.

Shawn Hargreaves:

I would probably use statics rather than a singleton myself in this case.

But you're an experienced guy :D

In my case, I would prefer input abstraction (let's say, an IInput interface). And that's a thing which can't be made through a static class. There are reasons for using both your and my solution.

Shawn Hargreaves:

It seems to me that a lot of time when people use singletons, they really just want a global or static, but they've bought into the OO religion that says "globals are evil", so they use singletons as a kind of trick to make what looks like an OO class instance behave more like a global. In which case why not just admit it and use a real global?

Because a static class is not an object. The Singleton pattern is used to globally expose an object, not a set of variables. And we all know that an object is not merely a set of variables.

Nick Gravelyn:

In my mind the only difference between a static class and the singleton pattern is that the singleton instantiates a single instance which can be retrieved through a static method or property.

Yes, but that difference is very important. A singleton is an object. A static class isn't. I think your oppinion is based on the simpliest (and, of course, the most common) way to use a singleton. But, as I said, a singleton is a reference to a global object, not to global variables. You can have, for example, a method which takes an IMyType interface as a parameter. Then, you can pass to it a MyTypeImplementation object or a MySingletonTypeImplementation object. The method don't worry about the origin of the IMyType parameter. The origin is abstract, but we can pass to it an object globally visible which can be modified in many places of our application, not only on that method, not only via the IMyType interface. Other way to use a singleton is by returning an instance of a derived class of the Singleton one, not the Singleton class itself, depending on some environment conditions. You can't do that things when using statics.

Ah, ok. I had not thought about using an interface for the class. Not sure I'd personally design a system around that ideology myself, but I can see where it would be useful. In my mind if I'm creating a static class or a singleton, it's usually because I don't expect there to be any competitor for its functionality.

For instance if I was making a system with an IInput interface so the user could plug in their own input handler, I likely wouldn't use the singleton approach. I would probably utilize a static class to store the chosen object which would function much like the singleton structure (a single static property that would be get-set), but of course this would allow multiple instances to float around so it isn't actually a singleton.

Of course it's all a matter of personal preference so to each their own.

I think that using statics is bad practice if you're not an experienced guy, and he said to be "very new to coding". Unexperienced people don't know that when using statics you're paying the price of losting some OOP features that you might need in the future. So I emphasize that statics aren't the good way for a beginner.

"very new to programing" in this case translates to less then a half a year ever looking at code.. and like I said, any feedback is good feedback, especially when it gives an insight on why some people go with one solution over another. IMO i'm all about options at this point, trying to learn whatever I can, so I very much appreciate the feedback thats been given :)

Shawn Hargreaves:

It seems to me that a lot of time when people use singletons, they really just want a global or static, but they've bought into the OO religion that says "globals are evil", so they use singletons as a kind of trick to make what looks like an OO class instance behave more like a global. In which case why not just admit it and use a real global?

Amen! Preach it brother! :)

Creating globals easily are one of the things I love about VB.NET, although newbies usually abuse it and then get frustrated when they move to something like C# where it works a bit differently.

Back on topic:

* BaseCharacter class - your derived Player and Enemy classes both have 4 members that are the same - camera, inventory, battle, and _InBattle. Why not move these to the BaseCharacter class?

* I recommend renaming some of your screen classes - Saved, Loaded, Battle. You have the inventory screen named InventoryScreen, so try to be consistent in your naming.

* ScriptEngineV1 and TileEngineV2 - I don't recommend putting version numbers in your class names.

* You have the same situation with your screen as you do with your BaseCharacter derived classes. I would create a Screen class and put your members there that you use in each screen class. You can implement the interfaces in your Screen class and override them in your classes derived from it if you want.

* Maybe it's just me but I wouldn't use a Stack for your screens. You may never need to render multiple screens, but you never know. What if you decided to make your InventoryScreen class only take up part of the screen?

Good effort so far. Congrats on getting this far.

What are some other methods for doing the screens? I'll take a look at the screen management demo for some other ideas.

For this game if i wanted to make the inventory screen only take up a part of the screen I probably would have had a _ShowInventory bool somewhere in there, and just draw that when the inventory was to be shown. First thought that came to mind, not sure if it would be the best, but its how I would have done it to start out.

The usual way to do it is the way it's done in the Game State Management sample. You have a management class that handles all the object in it and the objects know whether or not they should be rendered or they let the manager know. All your game code needs to do is tell the manager to update and it takes care of everything.

CraigGiles:

For this game if i wanted to make the inventory screen only take up a part of the screen I probably would have had a _ShowInventory bool somewhere in there, and just draw that when the inventory was to be shown. First thought that came to mind, not sure if it would be the best, but its how I would have done it to start out.

So what happens if you then have a half dozen screens that you want to display over top of the game, like every MMORPG out there? Things will quickly get out of control. Getting into an object-oriented state of mind is a good thing to do, IMO. :)