🎉 Celebrating 25 Years of GameDev.net! 🎉

Not many can claim 25 years on the Internet! Join us in celebrating this milestone. Learn more about our history, and thank you for being a part of our community!

Project Review Request

Started by
3 comments, last by M2tM 14 years, 4 months ago
Hello, I'm working on getting a job doing video games and I'd really like a few sets of eyes on the code example I'm considering putting up on my portfolio page (which is in development michaelhamilton.com.) Obviously it's a bit much to impose on a community to read or even skim through a few thousand lines of code for no reason, but I figure just a few of you may take that up anyway for reasons which may not even be clear to yourselves! So this post is to you, you glorious, magnificent person(s). The sample is meant to showcase the basic rendering and audio features of my "2D engine" which I've tinkered with for a while now. I cannot guarantee the entire project will sparkle, but I'm looking for affirmation on what I've done correctly and criticism on what I can improve. I did the art and code, but I got the audio from an open-source audio site. Library Example Download Visual studio 2010 beta 1 or 2 is required to run this project (or if you have a compiler which supports lambda functions that would also work in theory, though I haven't tested it on any other environments.) Do not despair if you do not have VS 2010, I have included the compiled result for your viewing pleasure and you can just take my word that it builds with no warnings, it shouldn't affect how the code reads. [Edited by - M2tM on February 6, 2010 7:46:22 PM]
_______________________"You're using a screwdriver to nail some glue to a ming vase. " -ToohrVyk
Advertisement
Good news so far, this has gotten me to the interview stage at a studio. I would still like feedback if anyone is up for it as this is a project I'll be using in several 2d games in my free time, but -hopefully- I do well in the interview and do not need to use it job-searching again.

Wish me luck!
_______________________"You're using a screwdriver to nail some glue to a ming vase. " -ToohrVyk
I did not notice that there is actually a new forum section called "breaking into the games industry." I believe this thread would get more attention and be more appropriately placed in there.

If a moderator could move this I would be grateful.
_______________________"You're using a screwdriver to nail some glue to a ming vase. " -ToohrVyk
Hey, I really like the design of your page! (except the "fake tabs" up the top which don't seem to do anything - what's with them?)

I assume this is for an entry-level game-programmer job? That page would probably be enough to get an interview.
It would be nice to have the source for your games on there too, not just the binaries, to demonstrate that you do get the difference between C++ and 'C with classes' ;-D
That said, I was actually pretty depressed to find out how widespread the 'C with classes' style is throughout the games industry :/ Just be prepared to write "bad code" if that's the standard where you find employment.

You accidentally put a capital letter in your download link, by the way. Here's a fixed link to library example (downloading it now).

[edit]Your library seems pretty nice.
If you're looking for critique though:
* Sometimes you pass std::strings by const reference, but also sometimes by value.
* Ownership of memory is not documented. You've got destructors which clean up objects, but there's no comments specifying when ownership is transferred (e.g. add takes ownership of it's argument and will delete it).
* No copy constructors or assignment operators? Double deletion will occur if I accidentally copy one of your objects that owns others. Easiest fix would be inheriting boost's noncopyable.
* Find-in-files "assert" turns up 0 results ;p E.g. things like mainScene.getRenderer()->... will obviously cause an access violation if there is no renderer - it would be nice to have assert(mainScene.getRenderer()); right before "dangerous" lines like that.
* Not enough const-correctness. Members like getWindowSize, which don't modify any member-vars should be const.

[Edited by - Hodgman on February 6, 2010 7:32:16 PM]
Thank you for spotting that typo! I've modified the original post to fix the error.

The "fake tabs" were actually set up to become real tabs when I get a little more content together. The site is still in development and I have a few more projects I want to put on, but have not gotten around to doing yet. I was also thinking of making an art section to the thing and that could be its own tab too. I was also thinking of starting up a tech blog on this site, I have a personal blog but it isn't really a professional blog as it is basically an online journal about my life (not that it is embarrassing should someone discover it, but I wouldn't feature it on a portfolio).

In particular I have an interpreted template language written in PHP (which is itself a template language which makes the project a bit silly sounding, but I still find it useful.) I also plan on putting that library example up, and I have a 2d platform game which makes use of a 2d physics engine (box 2d) which needs more work.

Yes, I'm going for an introductory/mid level job, it is difficult to find openings in Canada with all the layoffs dumping a ton of job seekers into the market so I'm not being too picky. I'm absolutely thrilled about getting this interview (on monday) and hope it works out, but if not I'll keep looking!

*Edit:
Thanks for the suggestions, these are all very solid pieces of advice and things which I really should look into solving/documenting!

* Sometimes you pass std::strings by const reference, but also sometimes by value.
-There isn't really a good reason for this except ignorance a couple years ago when I was writing some of the code still present today. I'll do my best to clean this up. *Edit: I could only find one place in the project where I was doing this, I may have missed some but a search for ("std::string" or ", std::string" pops up no results now)

* Ownership of memory is not documented. You've got destructors which clean up objects, but there's no comments specifying when ownership is transferred (e.g. add takes ownership of it's argument and will delete it).
-This is actually a very important point, I am glad you have highlighted for me. Though I put a lot of consideration into who owns what, I never -really- put a lot of consideration into explaining that. This is one of those things that is easy to overlook when working on a solo project.

* No copy constructors or assignment operators? Double deletion will occur if I accidentally copy one of your objects that owns others. Easiest fix would be inheriting boost's noncopyable.
-Copying objects is something which I have not considered closely enough, the safest and easiest option would be to simply make these objects non-copyable as you suggest. This is probably the most fundamental issue you have brought up, and one which could potentially have wide-ranging design changes depending on how I decide to approach it.

* Find-in-files "assert" turns up 0 results ;p E.g. things like mainScene.getRenderer()->... will obviously cause an access violation if there is no renderer - it would be nice to have assert(mainScene.getRenderer()); right before "dangerous" lines like that.
-This is a very good point, one which I'm aware of, but which I did not really sit down and make a solid decision on how to handle. Asserts would really make the most sense here.

* Not enough const-correctness. Members like getWindowSize, which don't modify any member-vars should be const.
-Excellent point, one which I'll definitely employ. I'll admit to being a bit const lazy, but I'm getting better over time.

All of that said, (and I did expect a number of things which could be improved) I'm glad you found it overall "pretty nice"! I'll keep working on it, your critique has been very valuable!

[Edited by - M2tM on February 6, 2010 8:54:33 PM]
_______________________"You're using a screwdriver to nail some glue to a ming vase. " -ToohrVyk

This topic is closed to new replies.

Advertisement