My current gig is to review code targetted at the Xbox 360 for things like security, performance and code quality. It ranges from Xbox Live Arcade titles, to Dashboard applications, to huge AAA games, both first and third-party.
Everyone I've encountered so far uses the C++ Standard Library throughout their game and core engine. Some have been fast-paced (ie - racing titles) and many of the AAA titles have been on what would be concidered the bleeding edge of graphics and technology.
Not so coincidentally, the ones that make reasoned, extensive and consistent use of C++SL features are the one's which get more audit itterations (because each pass goes more quickly) and invariably produce less feedback from our team to theirs.
Yes, you do not want to allocate memory wontonly and sporadically, but that's why the standard containers take an allocator as a template arguement. It's not uncommon for the titles we review to have completely supplanted new/delete with their own custom memory manager -- which they use with the STL quite nicely.
Portfolio Review: Programmer (all comments welcome!)
Full Disclosure
I am frequently in charge of managing the hiring of new employees in various positions, including those relating to software development, network management, and software architectural positions. Because of this I tend to have to review a lot of profiles, like yours.
Impressions
An initial examination of your site gives me two minor irritations. The first is that the background doesn’t fit with the foreground. The two don’t mesh properly. If you want to go that route, you need to ensure that the foreground meshes with the background properly As an example, a rounded border on the foreground elements would help to “embed” them into the background. Making the transition more seamless.
Secondly, the main page of your website is 200K, that’s a lot of wasted space for many things that I may never view. If you want to do what you’re doing, you might consider using jQuery to pull down the various pages as required (or in the background). This may not seem like a big issue to you, but on a busy corporate network, if I have to wait for your page to load up just to see your initial portfolio… it doesn’t look good. In comparison, the GameDev.net front page (which has significantly more content than you do) is only 109K.
Code
I popped open your sample code for your Cuburxia, and opened up CuburxiaGridAI.cs at random.
Really? That's funny I use the standard library all the time, and in production code too. I notice you had no trouble using the List type in your C# code, that's got the same allocation mechanism (roughly) behind it as the standard library does (there are minor differences, but they are of the kind you wouldn't notice for the most part). I prefer using containers that have been well tested by hundreds of thousands of programmers for years... rather than something that I whipped together to show that I know how to write a dynamically resizable array.
Furthermore, the standard library is no slower than the user. Proper usage of the standard library will produce fast code on most platforms that you'll use. There are exceptions, and for those you will find that eastl (modeled after the standard library) more than suffices. Most of that "debug" slowness is simply the standard library being intelligent and performing extra tracking and debugging of the usage, to prevent stupid things like indexing out of bounds.
I am frequently in charge of managing the hiring of new employees in various positions, including those relating to software development, network management, and software architectural positions. Because of this I tend to have to review a lot of profiles, like yours.
Impressions
An initial examination of your site gives me two minor irritations. The first is that the background doesn’t fit with the foreground. The two don’t mesh properly. If you want to go that route, you need to ensure that the foreground meshes with the background properly As an example, a rounded border on the foreground elements would help to “embed” them into the background. Making the transition more seamless.
Secondly, the main page of your website is 200K, that’s a lot of wasted space for many things that I may never view. If you want to do what you’re doing, you might consider using jQuery to pull down the various pages as required (or in the background). This may not seem like a big issue to you, but on a busy corporate network, if I have to wait for your page to load up just to see your initial portfolio… it doesn’t look good. In comparison, the GameDev.net front page (which has significantly more content than you do) is only 109K.
Code
I popped open your sample code for your Cuburxia, and opened up CuburxiaGridAI.cs at random.
- The first class visible in the file is not the CuburxiaGridAI class, but an abstract class dealing with the matching. This is an abstract class why? It should be an interface, as that most accurately reflects the behavior it exemplifies. Abstract classes contain partial (or full) implementations of functionality, except in that they have the ability to defer some decisions to the inheritor. Interfaces do not have an implementation and specify a contract that must be implemented by the client (the inheritor). Since yours is specifying a contract, it should be an interface, not an abstract class.
- Usage of XML commenting is good. However, your viewpoint is one of the person perspective. You should try and keep your documentation comments non-personal, for instance by using a third person objective writing style. An example: “I take in a standard ai grid state and should look at it to determine if it has any matches.” Becomes: “Takes in a standard CuburxiaGridAIState. Returns null if there are no matches, otherwise a list containing the matches.”
- The usage of null as a return type, instead of an empty list, is not exactly a great idea. Prefer using the empty list instead, as you can simply have a singular code path instead of multiple. That is, you eliminate a check for null in favor of treating both matches and non-matches the same.
- The second class in the file is ALSO not the main class of the file, but is the actual matcher. Your comments in this section get worse. You have unnecessary comments that detail what you’re doing just before you do it. Don’t do that. They are a waste of time, and also a waste of the readers time.
- Prefer using an enumeration for your dimension instead of a random integer. This makes your code much more readable, and eliminates the necessity for the aforementioned comments.
- After all of this, we’re pretty much thinking that this file is in relation to the matcher, and not the grid AI…and then we hit the grid AI class. So, you should really break this out into a separate file, or perhaps two separate files. The matcher interface and the matcher its self can both go in the same file, or you can prefer to have two separate files for each. Alternatively you can keep them all into this file, however… if you do so, make sure that the implementation of the matcher comes AFTER that of the main class.
- At this point things go straight to hell. You’re using threads and you’re aborting and recreating the thread every time you want to refresh your “brain,” instead of reusing the thread. Your thread is using no synchronization at all, and you’re recreating various objects that exist in the parent on the fly. The only form of Synchronization you have is in the “IsWorking” thread that simply tests if the created thread is “non-null” and “IsAlive” both of which are not reliable measures. You should be using some manner of synchronization here.
- Your comments go even further awry here, with some indicating return values on functions that don’t even have a return type (aka, are void). You also have members scattered throughout the class in no particular order, and with dissimilar protections. Some are internal, some are private. Be consistent, and group properly.
- You also have several areas of nearly identical code that could be refactored out to methods which would significantly improve code readability.
- Again you’re using integers instead of enumerations to indicate axis. This decreases code readability. The comments only add a slight improvement to that.
- You also have a random inner class that appears to hold nothing but a reference to an array. Either just pass in the array, or use a value type class (struct). No need for the extra heap allocation.
- Finally we get to the end of the file where we find another TWO classes (albeit tiny ones), one of which could be a value type class.
Quote:Old code, hate it myself. Taking it out.
Whatever you have on your portfolio page should reflect the best of what you have to offer. Nothing less. This is YOUR portfolio, it is there to show prospective employers what you can do. Every sample should be perfect.
Quote:I do use standard libraries quite often, but only when I'm lazy and am just trying to prove a concept. The standard library is slow and chokes to death when compiled under debug. It's also not great when porting to different devices.
Really? That's funny I use the standard library all the time, and in production code too. I notice you had no trouble using the List type in your C# code, that's got the same allocation mechanism (roughly) behind it as the standard library does (there are minor differences, but they are of the kind you wouldn't notice for the most part). I prefer using containers that have been well tested by hundreds of thousands of programmers for years... rather than something that I whipped together to show that I know how to write a dynamically resizable array.
Furthermore, the standard library is no slower than the user. Proper usage of the standard library will produce fast code on most platforms that you'll use. There are exceptions, and for those you will find that eastl (modeled after the standard library) more than suffices. Most of that "debug" slowness is simply the standard library being intelligent and performing extra tracking and debugging of the usage, to prevent stupid things like indexing out of bounds.
Quote:Original post by StephenTC
I do have to say, there is only one AAA game company that I've heard of using the STL.
1. Ogre3D uses the SC++L extensively (there's even talk of including boost in new versions).
2. Ogre3D is used in many successful commercial games.
3. Therefore it follows that AAA game companies use the SC++L (STL as you refer to it as).
Quote:
(Which for a string takes what... 2 hours?).
2 hours to write a self-proclaimed (probably buggy and slow) string class.
vs.
2 seconds to type out std::string which has been written over the course of many years by professionals. For example, does your 2 hour string implementation optimize for short strings?
Author
Washu, thanks for the reply.
Yes I do realize this. However that function fires once every... 4 or 5 seconds. It's not really a penalty to create one or two threads occasionally. Under the implementation it's unnecessary.
You are right, however in this implementation there is no reason to use real Lock objects. The app doesn't do anything with the relevant data while it's searching so it's really equivalent to using Locks. It only uses that time to render.
Yes
Yes, I'd like to take this moment to clarify something.
I use HIGH-LEVEL languages all the time, such as C# and Python and I use all of the data structures involved there. However, I believe that if one is going to go through all the troubles that C++ has, they might as well become pedantic low-level coders. Sometimes I enjoy straight C coding as well (which has NO stl).
I agree that the STL is applicable. If you look at my physics code on my physics section you'll note them there(I'm currently recommenting and reformatting this so spare the reviews just yet). However, I prefer not to use it when I don't have to. Simple things, like strings and arrays do not require that much management. If I scared a recruiter off because I do my own memory management then I'm totally fine with that.
All in all, thank you everyone. I've gotten some good feedback.
Quote: You’re using threads and you’re aborting and recreating the thread every time you want to refresh your “brain,” instead of reusing the thread
Yes I do realize this. However that function fires once every... 4 or 5 seconds. It's not really a penalty to create one or two threads occasionally. Under the implementation it's unnecessary.
Quote:Your thread is using no synchronization at all
You are right, however in this implementation there is no reason to use real Lock objects. The app doesn't do anything with the relevant data while it's searching so it's really equivalent to using Locks. It only uses that time to render.
Quote:Whatever you have on your portfolio page should reflect the best of what you have to offer. Nothing less. This is YOUR portfolio, it is there to show prospective employers what you can do. Every sample should be perfect.
Yes
Quote:notice you had no trouble using the List type in your C# code
Yes, I'd like to take this moment to clarify something.
I use HIGH-LEVEL languages all the time, such as C# and Python and I use all of the data structures involved there. However, I believe that if one is going to go through all the troubles that C++ has, they might as well become pedantic low-level coders. Sometimes I enjoy straight C coding as well (which has NO stl).
I agree that the STL is applicable. If you look at my physics code on my physics section you'll note them there(I'm currently recommenting and reformatting this so spare the reviews just yet). However, I prefer not to use it when I don't have to. Simple things, like strings and arrays do not require that much management. If I scared a recruiter off because I do my own memory management then I'm totally fine with that.
All in all, thank you everyone. I've gotten some good feedback.
Author
Quote:2 hours to write a self-proclaimed (probably buggy and slow) string class.
Just curious, have you ever opened up the stl string and looked at it's construction.
Quote:
(which has NO stl).
Yes it does. It's called the C standard library instead of the C++ standard library. Or do you mean to tell me you never call malloc() in C? printf()? sprintf()? strcmp()?
Quote:
Simple things, like strings and arrays do not require that much management.
Indeed, they require a lot of management for nontrivial uses. If you use a raw-char*-string in a C++ class, you have to start worrying about copy and ownership semantics for that class... exception safety, if you use those, and error cleanup paths if you don't... assignment of the class in question, etc. You have to worry about buffer management (over, under runs, etc). Resizing the array and copying, if needed.
Are these difficult tasks for the competent C++ programmer? No, not really. But they do require finite time and consist largely of rote boilerplate bullshit that one has to be repeating often. Competent C++ programmers usually want to spent their time on more interesting problems. Thus, they use std::string or std::vector, which handle all the above scenarios (and more) optimally for the general case. They will probably still require some management by the programmer in some cases, but far less in general.
Quote:
Just curious, have you ever opened up the stl string and looked at it's construction.
I have. It can be quite hard to follow, it's true. Reading the bulk of the SC++L implementation is a skill that must be acquired (there are reasons that the SC++L header template code is so complex, you know, it isn't just to obscure it). Fortunately it's a skill most programmers don't need to acquire, since the library has been vetted by hundreds -- no, thousands -- of programmers already.
What's your point with this line of argument?
Author
Quote:Yes it does. It's called the C standard library instead of the C++ standard library. Or do you mean to tell me you never call malloc() in C? printf()? sprintf()? strcmp()?
No it doesn't. STL stands for standard template library, not 'ST'andard 'L'ibrary. C has the stdlib which stands for standard library.
The point is there is no language support for vectors and strings.
Quote:I have. It can be quite hard to follow, it's true. Reading the bulk of the SC++L implementation is a skill that must be acquired (there are reasons that the SC++L header template code is so complex, you know, it isn't just to obscure it). Fortunately it's a skill most programmers don't need to acquire, since the library has been vetted by hundreds -- no, thousands -- of programmers already.
What's your point with this line of argument?
Actually more so that for all of that complex stuff, it's doing an absurd amount of nothing. My point is it's not hard to replicate the behaviors in the STL. A lot of the functionality is much more basic than it seems everyone believes.
Quote:Indeed, they require a lot of management for nontrivial uses. If you use a raw-char*-string in a C++ class, you have to start worrying about copy and ownership semantics for that class... exception safety, if you use those, and error cleanup paths if you don't... assignment of the class in question, etc. You have to worry about buffer management (over, under runs, etc). Resizing the array and copying, if needed.
Well it comes down to programming styles and purpose. Yes I agree with you. AGAIN, I use the STL frequently, I use C# and Python (again). This discussion is a road to nowhere.
Quote:
No it doesn't. STL stands for standard template library, not 'ST'andard 'L'ibrary. C has the stdlib which stands for standard library.
As I said before, you mean "SC++L" when you say "STL," and SC++L is the Standard C++ Library. The real STL was a set of libraries called the Standard Template Library (as you note), but wasn't actually part of the C++ standard like the SC++L. It was developed by SGI in the early years of the C++ standardization process. It was called the STL before parts of it were rolled into the standard. The stuff in the standard, the stuff that 99.99% of C++ programmers use, is the SC++L.
Quote:
The point is there is no language support for vectors and strings.
So it's just vectors and strings you have a problem with? So again -- you never call strcmp()? What's the difference? std::string's internal implementation is almost always exactly what you'd expect -- a char* and a count. How is that any different than what you'd write?
Quote:
Actually more so that for all of that complex stuff, it's doing an absurd amount of nothing. My point is it's not hard to replicate the behaviors in the STL. A lot of the functionality is much more basic than it seems everyone believes.
Care to back this up with something resembling a concrete example?
Quote:
Well it comes down to programming styles and purpose. Yes I agree with you. AGAIN, I use the STL frequently, I use C# and Python (again). This discussion is a road to nowhere.
I disagree. This still has much to do with your attitude about the craft, which I think is a serious strike against you as far as hiring purposes go.
Author
Quote:As I said before, you mean "SC++L" when you say "STL," and SC++L is the Standard C++ Library.
A yes, you are right about this. However, most of what I am discussing is in the STL (excluding the string). The point was that there weren't anything similar to these data structures in C (which is almost irrelevant as we all know C is lower level than C++)
Quote:So it's just vectors and strings you have a problem with? So again -- you never call strcmp()? What's the difference? std::string's internal implementation is almost always exactly what you'd expect -- a char* and a count. How is that any different than what you'd write?
1) It's the data structures I have a problem with, not code I didn't write (like strcmp).
2) The difference is at a low level strcmp has a very predictable outcome. When dealing with std::strings, a change could without intention cause a memory realloc. I'm not exactly ANGRY with this functionality, but it's something to think about when working on performance.
3) Exactly! It really doesn't do all that much different than what I'd do, but it somehow contains incomprehensible code(relatively speaking) that spans many files often for the only purpose of just integrating a pointer. When compiled inline (like I earlier mentioned) this is totally fine. However, when compiled in debug it chokes doing all of the jumps. The difference is that they wrote it for dynamism, safety and then speed, whereas mine would be written for speed and specificity (also compile size).
Quote:Care to back this up with something resembling a concrete example?
What I mean to say is this: Yes vectors/strings/etc are nice thick robust efficient data structures. But when developing specialized game structures you can replicate most of the functionality for a specific controlled environment. The idea is that by making something LESS safe and LESS robust you get a performance boost.
Quote:I disagree. This still has much to do with your attitude about the craft, which I think is a serious strike against you as far as hiring purposes go.
I'm not sure what you mean about my attitude to the craft. This has to do with my attitude toward the STL/C++STDLIB. I'd be wary of someone whom jumps to assuming the STL data structures were absolutely necessary to C++ applications(though I think that they SHOULD be known). Again, I use them quite often in much of my newer code. My argument is against the argument against doing manual memory management. My argument is NOT against the STL(just why I don't use it in every application). Cryptic, for example, does all of their coding in straight C. This is entirely a matter of opinion and it really does not matter as long as the job gets done.
[Edited by - StephenTC on January 26, 2010 4:50:22 AM]
Author
One final thing for now, I understand the problems you all have with the code and I'll make sure to go through and have only the code I wanted on the site. I agree that that's how it should be and I'm glad you caught me to grill me on it.
I'd like to hear more about the content on my website (outside of the python code samples, which I would LOVE comments on).
What do you like/dislike about where the website is going? (What do the multiple elements convey).
I'd like to hear more about the content on my website (outside of the python code samples, which I would LOVE comments on).
What do you like/dislike about where the website is going? (What do the multiple elements convey).
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
