Jump to content

  • Log In with Google      Sign In   
  • Create Account

Interested in a FREE copy of HTML5 game maker Construct 2?

We'll be giving away three Personal Edition licences in next Tuesday's GDNet Direct email newsletter!

Sign up from the right-hand sidebar on our homepage and read Tuesday's newsletter for details!


We're also offering banner ads on our site from just $5! 1. Details HERE. 2. GDNet+ Subscriptions HERE. 3. Ad upload HERE.


#ActualKhaiy

Posted 12 November 2013 - 07:44 PM


do you think the definition of the changescreen function will lead to memory leaks?

 

I wouldn't worry about it. C# manages memory on its own with the garbage collector, and while memory leaks can happen I think that you'll be plenty safe. A bigger problem is that when there are no more references to an object it becomes eligible for garbage collection, which may not be what you want for something like a Room in a game (and especially one that might change as the player interacts with it).

 

 

 


Or is there a better way to do it?

 

This is something that's just about always true of any code. Programming is a pragmatic activity in the sense that if your code works as intended it's right enough. But in this case, I suggest the following as broad design approaches (take or leave these, as you like):

 

1. I don't like having each Room class (like Level1) as a direct descendant of DrawableGameComponent. Even though you will need to draw each Room, you don't need to use inheritance to make it happen. It can also force you, as it did in the code you posted above, to do some weird casting and makes for very unintuitive and hard to read/maintain code. You can manually call currentRoom.Draw the Game.Update method. If you really want to keep the inheritance from DrawableGameComponent, you can have an intermediate base class (Room, maybe) from which all individual rooms will inherit.

 

2. The ChangeScreen function seems overly engineered to me. In line with the above, I would prefer it to take a Room parameter rather than a DrawableGameComponent for clarity alone. I'm also not totally sure what you're planning to get by creating instances of Rooms on-demand this way or why you are using Type.GetType to identify the next room to be created. It's the kind of thing that's maybe a little hackish but gets the job done in a small demo-style project but will cause you headaches in the future, unless you have some specific reason for having set things up this way.

 

3. The function calls are well into spaghetti code territory. This too is something that works fine for a small test project but is a nightmare later. Game creates a Level1 object -> Game.Update itself calls Level1.Update (which is fine and normal) -> Level1.Update calls Game.ChangeScreen (this is less fine). Method calls shouldn't be reaching back and forth between distinct objects this way-- you're overly coupled and for no benefit. In general, you want calls to be as one-way as possible in terms of one class containing another as a field.

 

4. You may have a particular reason for on-demand generation of Rooms, but I'll urge you to consider an alternative approach. Because a room's contents, triggers, and other components might change as the game progresses creating previously visited rooms via their constructors can be a problem. The player removes an object from a room, for instance, so you don't want the object to be there again when the player backtracks through the same room. You can avoid this with fancy polymorphic constructors, but you might also have something like a container of Room objects which contains an instance of every room in the game and then access and update them as needed. That way you only have to deal with varying room states when saving and loading a game.

 

I think that the best general advice I can give is to try and think of your program as a set of discrete modules which plug into each other only and exactly as needed. It can be hard to see in a small test project like this one, because Game has so many things that are within its area of responsibility that it's easy to have Game do everything and only use other objects as data containers.


#1Khaiy

Posted 12 November 2013 - 07:41 PM


do you think the definition of the changescreen function will lead to memory leaks?

 

I wouldn't worry about it. C# manages memory on its own with the garbage collector, and while memory leaks can happen I think that you'll be plenty safe. A bigger problem is that when there are no more references to an object it becomes eligible for garbage collection, which may not be what you want for something like a Room in a game (and especially one that might change as the player interacts with it).

 


Or is there a better way to do it?

 

This is something that's just about always true of any code. Programming is a pragmatic activity in the sense that if your code works as intended it's right enough. But in this case, I suggest the following as broad design approaches (take or leave these, as you like):

 

1. I don't like having each Room class (like Level1) as a direct descendant of DrawableGameComponent. Even though you will need to draw each Room, you don't need to use inheritance to make it happen. It can also force you, as it did in the code you posted above, to do some weird casting and makes for very unintuitive and hard to read/maintain code. You can manually call currentRoom.Draw the Game.Update method. If you really want to keep the inheritance from DrawableGameComponent, you can have an intermediate base class (Room, maybe) from which all individual rooms will inherit.

 

2. The ChangeScreen function seems overly engineered to me. In line with the above, I would prefer it to take a Room parameter rather than a DrawableGameComponent for clarity alone. I'm also not totally sure what you're planning to get by creating instances of Rooms on-demand this way or why you are using Type.GetType to identify the next room to be created. It's the kind of thing that's maybe a little hackish but gets the job done in a small demo-style project but will cause you headaches in the future, unless you have some specific reason for having set things up this way.

 

3. The function calls are well into spaghetti code territory. This too is something that works fine for a small test project but is a nightmare later. Game creates a Level1 object -> Game.Update itself calls Level1.Update (which is fine and normal) -> Level1.Update calls Game.ChangeScreen (this is less fine). Method calls shouldn't be reaching back and forth between distinct objects this way-- you're overly coupled and for no benefit. In general, you want calls to be as one-way as possible in terms of one class containing another as a field.

 

4. You may have a particular reason for on-demand generation of Rooms, but I'll urge you to consider an alternative approach. Because the rooms contents, triggers, and other components might change as the game progresses creating previously visited rooms via their constructors can be a problem. The player removes an object from a room, for instance, so you don't want the object to be there again when the player backtracks through the same room. You can avoid this with fancy polymorphic constructors, but you might also have something like a container of Room objects which contains an instance of every room in the game and then access and update them as needed. That way you only have to deal with varying room states when saving and loading a game.

 

I think that the best general advice I can give is to try and think of your program as a set of discrete modules which plug into each other only and exactly as needed. It can be hard to see in a small test project like this one, because Game has so many things that are within its area of responsibility that it's easy to have Game do everything and only use other objects as data containers.


PARTNERS