Sign in to follow this  
stromchin

Reading someone's code

Recommended Posts

My company made a game before I started working there. And now they are making a new one kind of similar to it.

So I will be mainly in charge of it (it's not that the game type is too complex or anything), I've been told to read and understand the code, but there is barely any comments on it.
Using visual studio 2008, I made class diagrams (which help only a little bit)
and read the code many times, understanding the finer c++ things that I didn't know before. And then trying to understand what different classes are supposed to do.
I basically come from "school knowledge" so for example, I was never taught scripting for games... and this game is heavily scripted, so I'm having trouble with that.
And in general with the flow of the game.

[b]My question: [/b]besides reading and debugging and tracing the code in the debugger... [b]what else can I do?[/b] This is my task until the other guy finishes putting together the base for the new game and hands that to me. I just want to be more prepared (first time with so much responsibility) but I dont know what else I can do.

I wish each class had a comment explain what it's supposed to be doing, or who is supposed to call it.
I can probably end up copying a lot of the stuff for the new game, but I want to understand what I'm doing instead.

I don't think the level is too high for me, but what I think is that without comments, it's really hard to interpret someone else's code from 0 without any kind of comments or documentation.

Share this post


Link to post
Share on other sites
[quote name='stromchin' timestamp='1306737396' post='4817409']
[b]My question: [/b]besides reading and debugging and tracing the code in the debugger... [b]what else can I do?[/b]
[/quote]

Sometimes documentation can be a bad thing i.e. when it is incorrect. If I were you I would look into some kind of automated tool that could parse the code and show you its static structure preferably in a graphical format. Knowing the class hierarchy, the dependencies, and any tangles from a high level would go a long way in helping you determine what the heck is going on.

Share this post


Link to post
Share on other sites
[quote name='loom_weaver' timestamp='1306738979' post='4817413']
[quote name='stromchin' timestamp='1306737396' post='4817409']
[b]My question: [/b]besides reading and debugging and tracing the code in the debugger... [b]what else can I do?[/b]
[/quote]

Sometimes documentation can be a bad thing i.e. when it is incorrect. If I were you I would look into some kind of automated tool that could parse the code and show you its static structure preferably in a graphical format. Knowing the class hierarchy, the dependencies, and any tangles from a high level would go a long way in helping you determine what the heck is going on.
[/quote]

that's a good idea. I don't know much about that, but I've heard about doxygen. Is there a better tool for it?
I think doxygen might not be exactly what I want though...

update: ok, I used doxygen on it, and it helps a bit. At least as a way to see classes in a more organized way.

The problem with this code is that there isn't much inheritance and stuff like that. Just a bunch of classes dispersed everywhere, so the diagrams generated don't seem to help much. But thanks, I'll keep looking at the code with doxygen a bit more and see if I can get a better grasp of this code.
Other than that, I'm pretty bored :unsure:

Share this post


Link to post
Share on other sites
Thanks, I think I saw something that's a bit of a performance problem at some point (the hardware can still take it, but I think I can optimize it)... I guess I'll try to fix that.

[quote]
1) First, always make sure that there doesn't exist some documentation that you don't know about. Ask people who are familiar with the code or former programmer if there exists some documentation that you haven't been given yet. Sometimes you'll get lucky, but don't get your hopes up.
[/quote]
I already did... and nothing. It's like: "the code is there, what else do you need?"

[quote]
2) Use auto-documentation tools. It sounds like you've already done this with Visual Studio's class diagram (I'm unfamiliar with VS though). I typically run the entire source code through doxygen to generate class diagrams, dependency graphs, class/functions/variables/constants lists, etc. It won't do too much to help you understand the code, but it is useful in organizing things and providing a "reference index" of sorts.[/quote]
also just tried it, and it's good... but it can't make miracles happen... at least it gave me a better "view" at some of the classes.

[quote]3) As you go through the code, add comments as you figure pieces of it out. Don't just sit there and read through thousands of lines and try to make sense of it all. its boring and non-engaging, so you'll quickly lose focus or forget what you read through 100 lines ago.[/quote]
I should have done this since I started -_- I'll take it into account from now on. Sounds like a good idea.

[quote]4) If permitted by your superior, fix faulty design aspects of the code. This only applies in certain circumstances such as where you are given the task to fix up a large, broken piece of software. As you start addressing design flaws, things quickly start to make more sense and perhaps you can come to understand the methods behind the madness.[/quote]
well, the game is already finished, so my changes won't have any effect, it's my local copy so I don't see the problem. I will start optimizing what I said before.


About what is good documentation and bad documentation... I've never written a big document (like what doxygen generates) but I have never been in a large project before either, so I guess I just have to get used to the way doxygen wants me to document stuff.
Anyways, about comments. In my opinion, when you create a class or function, there is a minimum of comments that it needs:
1. what is this class supposed to do (and what things isn't)
2. who is supposed to call it, and with what kind of parameters (nothing exhaustive)
3. maybe explain how it does it if it takes some unexpected approach to optimize it or something like that

for example, I'm finding stuff like
class command_text
as a class, and I have to go around figuring out what it does. But I would have done something like this

// this class prints text to the screen without any colors or anything
// it's supposed to be called on the menu screen, not inside the game
class command_text

(if I even use such a generic name for a class)

Share this post


Link to post
Share on other sites
[quote name='stromchin' timestamp='1306741682' post='4817421']
[color="#1C2837"][size="2"]About what is good documentation and bad documentation... I've never written a big document (like what doxygen generates) but I have never been in a large project before either, so I guess I just have to get used to the way doxygen wants me to document stuff.
Anyways, about comments. In my opinion, when you create a class or function, there is a minimum of comments that it needs:
1. what is this class supposed to do (and what things isn't)
2. who is supposed to call it, and with what kind of parameters (nothing exhaustive)
3. maybe explain how it does it if it takes some unexpected approach to optimize it or something like that[/size][/color]
[/quote]

Good documentation (inline code comments) includes [b]why[/b] something was written the way it is. I like seeing comments like:
[code]
// Don't remove this line because otherwise component X will crash (bugzilla-2152)
[/code]

Good comments answer [b]what[/b] when it isn't apparently obvious. E.g. the comment /* Use Newton-Raphson algorithm */ at the top of the function is all that I need to know and is an excellent comment. Another good comment:
[code]
long duration; // in msec
[/code]

Bad comments (apart from obviously incorrect information) is [b]how[/b] something was written and sometimes the [b]what[/b]. For example:
[code]
i++; // increment i [BAD]
myDict[i] = name; // place name into the dictionary [BAD]
[/code]

I can determine that by looking at the code directly thanks.

Finally a good comment says the [b]who[/b].
[code]
// TODO: fixme [BAD]
// TODO-JS: fixme [GOOD]
[/code]

With the second comment I know that my colleague John Smith is the one who I should put my hands around his throat... er go talk to.

Share this post


Link to post
Share on other sites
Be careful of making design changes though. Even if you make only a few simple ones, you'll need to perform tests to make sure they didn't break anything. That's the bad part (and why I only typically do design changes on already-broken software).


When I'm in charge of things, I prefer to document my code the following way:


1) Header files with heavy use of doxygen comments, documenting every class, method, member, function, and set of constants.
Example: http://allacrost.svn.sourceforge.net/viewvc/allacrost/trunk/game/src/modes/map/map_dialogue.h?revision=1888&view=markup

2) Source files with normal comments to quickly explain blocks of code, highlight important points, and insert TODO/TEMP comments appropriately.
Example: http://allacrost.svn.sourceforge.net/viewvc/allacrost/trunk/game/src/modes/map/map_dialogue.cpp?revision=1931&view=markup

3) External documentation for the software that gives a high-level architecture overview and the most common set of operations and interfaces someone would use
Example: http://allacrost.sourceforge.net/wiki/index.php/Quick_Start_Guide and http://allacrost.sourceforge.net/wiki/index.php/Scripting_Engine

4) Doxygen-generated documentation from the code to serve as a reference to the code (ideally automated to re-generate the documentation once a day or so)
Example: http://www.allacrost.org/public/doc/doxygen/index.html (note this is very, very outdated at the moment)


By the way, make sure you know what code standard your company is using as sometimes they include a standard for writing comments as well. And if your company doesn't have a code standard....uhhh, be worried. :unsure:

Share this post


Link to post
Share on other sites
I generally agree with loom_weaver on the descriptions of good/bad comments, though the 'in msec' comment is a bad comment. Well, not such a bad comment as 'duration' is a bad name. In general, I disagree with your assessment of comments:
[quote]
1. what is this class supposed to do (and what things isn't)
[/quote]

This should be fairly obvious by name. And if you are good about your 'one class handles one task' it's even better.

Sure, sometimes things need to push the envelope or can't have a great name due to their... generalness. Then make a comment describing what's going on, but realize that the need to make a comment is a code smell.

[quote]
2. who is supposed to call it, and with what kind of parameters (nothing exhaustive)
[/quote]

Protection level enforces who can call it, and your parameters have descriptive types/names to make the calling convention obvious.

[quote]
3. maybe explain how it does it if it takes some unexpected approach to optimize it or something like that
[/quote]

Certainly. This is the case where comments are most valuable (in addition to loom_weaver's 'don't do this or else' or 'bug 4255 was caused by this being ___'). Cases where you divert from coding norms for a good reason. Comments exist to supply [b]reasons[/b] to code, not to describe code.

Share this post


Link to post
Share on other sites
It sounds like the problem might be no clear module structure. Do the classes interact in a spaghetti fashion? Is there any attempt at separating subsystems into logical units?

Instead of looking at the code, try starting from a high level perspective and move down. So maybe figure out how a mouse click gets turned into an object action. Or try to figure out where from main() you get to some actual rendering. Find out what would be involved in adding a new game object. Do you have a full build environment? Play around with the code, actually change some stuff.

That might give you a better view of the "important" types and functions, the ones you are most likely to interact with when you go to work on extending this. You don't need to understand all the classes, there might be too many.

Share this post


Link to post
Share on other sites
[quote name='rip-off' timestamp='1306769715' post='4817542']
Instead of looking at the code, try starting from a high level perspective and move down. So maybe figure out how a mouse click gets turned into an object action. Or try to figure out where from main() you get to some actual rendering. Find out what would be involved in adding a new game object. Do you have a full build environment? Play around with the code, actually change some stuff.
[/quote]

Yes an excellent idea! I forgot about the entire dynamic angle. Put a whole bunch of println statements in the code and then start executing some common pathways.

Resist the urge to write-off the codebase and disregard it as an a tangled spaghetti-code unholy mess written by incompetents. It may be indeed the case but in the real-world you need to work with legacy systems and that mind-set won't help you any.

Share this post


Link to post
Share on other sites
[quote name='rip-off' timestamp='1306769715' post='4817542']
It sounds like the problem might be no clear module structure. Do the classes interact in a spaghetti fashion? Is there any attempt at separating subsystems into logical units?

Instead of looking at the code, try starting from a high level perspective and move down. So maybe figure out how a mouse click gets turned into an object action. Or try to figure out where from main() you get to some actual rendering. Find out what would be involved in adding a new game object. Do you have a full build environment? Play around with the code, actually change some stuff.

That might give you a better view of the "important" types and functions, the ones you are most likely to interact with when you go to work on extending this. You don't need to understand all the classes, there might be too many.
[/quote]

Yeah, the code isn't too complicated, it's just that it's heavily based on the script, and I had zero experience with that. I was playing with the code, and trying to figure out what certain class did (or didn't do...) I kept investigating, placing breakpoints and playing the game, but nothing happened. It seems it was a completely useless class, I commented it all out and no compiler or linker error.

I mean, I don't want to say the code is bad, because I'm a novice and the game works. But dammit...

Share this post


Link to post
Share on other sites
[quote name='stromchin' timestamp='1306770698' post='4817555']
Yeah, the code isn't too complicated, it's just that it's heavily based on the script, and I had zero experience with that. I was playing with the code, and trying to figure out what certain class did (or didn't do...) I kept investigating, placing breakpoints and playing the game, but nothing happened. It seems it was a completely useless class, I commented it all out and no compiler or linker error.

I mean, I don't want to say the code is bad, because I'm a novice and the game works. But dammit...
[/quote]
On legacy code bases useless code is everywhere. It's just something you have to get used to. It will piss you off, but eventually you'll realize that it's just the way legacy codebases work. You'll probably even end up doing some less than desirable practices because it's easier than rewriting a significant portion of a legacy codebase.

It is a hazard of the job. When you have a 3000 file code base that's had pieces of it used by 20 projects you aren't going to have the manpower to have a perfectly tailored codebase to the last game in the line.

Share this post


Link to post
Share on other sites
[quote name='stromchin' timestamp='1306770698' post='4817555']
I kept investigating, placing breakpoints and playing the game, but nothing happened. It seems it was a completely useless class, I commented it all out and no compiler or linker error.

I mean, I don't want to say the code is bad, because I'm a novice and the game works. But dammit...
[/quote]
Actually you made good progress. You figured out that the class wasn't needed. If I was in your shoes at that 'aha' moment I would feel some (perverse) satisfaction. Determining what parts of the code aren't used or are not relevant is equally important to your goal.

Share this post


Link to post
Share on other sites
[quote name='stromchin' timestamp='1306770698' post='4817555']
I kept investigating, placing breakpoints and playing the game, but nothing happened. It seems it was a completely useless class, I commented it all out and no compiler or linker error.

I mean, I don't want to say the code is bad, because I'm a novice and the game works. But dammit...
[/quote]
That's a good start.

Be careful because sometimes code that seems safe to comment out may actually be used by some less-common code cases and code routes. Be sure to look for edge cases and other ways in to the code.

Now use your logic skills to guess at why they might have written it, why it might be unneeded.

Getting started in an unfamiliar code base is a bit like an evil scientist doing a vivisection. You discover that poking here causes a response over there. Pinching this causes that to twitch. Closing off something causes the game to crash.

One good way to learn is to just jump in. Figure out a small change that you want to make, perhaps adjust an existing UI screen, or add a small piece of functionality, or fix a known bug. The first time in you will end up dropping lots of breakpoints to see where to stop the program at various times and places, you will need to search for all instances of a data type or member variable or for everywhere a function is used.

When you discover something that you didn't understand add a comment so you'll immediately recognize it next time. When you have an awkwardly named variable or function then use your tools to help rename it into something more meaningful to you.

Gradually it will grow into something you can work with comfortably.

Share this post


Link to post
Share on other sites
Beware things that *seem* to be dead code - some scripting systems I've seen use reflection (or some other kind of runtime lookup) when a script is interacting with the engine code. Removal of such engine code will compile, but then mysteriously crash at runtime when the particular script tries to call the associated engine code.

Share this post


Link to post
Share on other sites
[quote name='Nypyren' timestamp='1306809582' post='4817732']
Beware things that *seem* to be dead code - some scripting systems I've seen use reflection (or some other kind of runtime lookup) when a script is interacting with the engine code. Removal of such engine code will compile, but then mysteriously crash at runtime when the particular script tries to call the associated engine code.
[/quote]

Yeah, a better idea (depending on language and compiler) would be to compile the app, lib, whatever it is, and then actually view the debug map files generated at the end to make certain that the classes/functions weren't left in the lib because of some command line switch to the compiler to disable stripping of unused code and classes. Then you can be absolutely sure that you can get rid of them, because if they're not in the final product, then they're obviously not used at all!

Share this post


Link to post
Share on other sites
Script can be both a blessing and a curse, depending on how is it implemented and how is it used.
The purpose of a good script is to invoke very well defined code functions in a dynamic way to react to events and such. It often gets overused, for instance to calculate projectile trajectory, thats not what script is for, thats game code responsibility. If you see a lot of disconnected classes and methods that seem to never get called in code, they are probably being called from script. In some cases even the flow of the game is implemented almost entirely from script, not usually the best idea, but it does work and it is used.

Unless you are using an in-house engine (one created by your company) you might do well in telling us what you are using in order for those who know it to provide you with additional tips.

Share this post


Link to post
Share on other sites
Also learn the scripting language as most of the game will probably be done in it and you need to know it to follow the flow of the game. Thats also one of the reasons why the classes in the project seem so unconnected, as the connection isn't made in the code but in the data of the game.

If there is something like a main in the script thats most likely where the game starts running and from there you can follow what is going on.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this