Gnome:Code Reviews

From Notes

Jump to: navigation, search


Dasher code reviews

29 June 2010

For this code review, I am looking at the master branch.


I am not finding any obvious standalone documentation that explains your work. This is really important, because that is the first thing I (or anybody else) will look for. Without it I am forced to read source code and try to figure out what it is the source code is trying to implement, which can be rather difficult (as you well know). This has to be turned into a top priority. There needs to be some standalone documentation that describes (at a minimum) (1) how you have implemented the game mode, and (2) how you have restructured the event-handling system. Identify relevant classes by name, and include class diagrams if you think they will be helpful. You may also have to spend some time documenting Dasher's architecture as well, so that your description of your work makes sense; if so, that should be clearly delineated.

Oh dear. It looks like at least some of the other Dasher coders are using the /// style of Doxygen comments. We should probably be consistent. I'd recommend poking around the other people's source code to determine whether they are in fact consistently using ///, and if so, changing your style to match theirs. It also looks like they are using Doxygen's grouping mechanism (<a class="reference external" href=""></a>), so maybe consider doing the same if it looks appropriate.


As a generate note on formatting, please make better use of blank lines. E.g.,

namespace Dasher {
class CFileWordGenerator : public CWordGeneratorBase {
    CFileWordGenerator(std::string path, int regen) : CWordGeneratorBase(regen) {

is a lot easier to read as

namespace Dasher {

class CFileWordGenerator : public CWordGeneratorBase {

    CFileWordGenerator(std::string path, int regen) : CWordGeneratorBase(regen) {

Also, notice that other authors have taken a bit of a different approach to defining the namespace; e.g., from DasherInterfaceBase.h we see:

namespace Dasher {
  class CDasherInterfaceBase;

class Dasher::CDasherInterfaceBase:private NoClones

Of course, that's not quite as convenient (you have to declare the class in two places, though in the same file), but does eliminate one level of nesting of braces. Take a look at the other existing code to see if this is done consistently, and if so, please follow that pattern so that your code follows the existing conventions.

GameModule.cpp, GameModule.h

I don't understand the first comment in CGameModule (I don't actually know...). What does this refer to?

The formatting of the declaration of CGameModule in GameModule.h is working against comprehension---it is very hard for me to what role any given identifier is playing, because there is no helpful indentation to tell me, e.g., that I am still reading a sequence of parameters to a constructor.

Try for some consistency in grammar for API documentation. You sometimes end single-line sentences with a period, sometimes not. I recommend the latter.

Please remember that many people set their editors to 72--80 columns, so you should not have lines of any type that are longer than 80 characters. And don't forget about possible tab-size issues!

But in general, the API documentation for CGameModule is very good.

I'm not sure which part of CGameModule::DecorateView() actually draws the arrow---that seems to be a bit of a failure of documentation.

CWordGeneratorBase.h, FileWordGenerator.h, FileWordGenerator.cpp

I'm perplexed by the return value of CWordGeneratorBase::Generate(). Under what conditions would the generation fail? Is failure an ordinary condition? I guess what I'm wondering (just based on the documentation) is why throwing an exception is not a more appropriate way to handle a failure.

It would be good to document the workflow of using a CWordGeneratorBase. I.e., what does a typical client usage look like? In particular, why does a client have to invoke Generate() in order to get the next word? Why not just instantiate the CWordGeneratorBase and then repeatedly invoke GetNextWord? How does the client know when there are no words left? It is possible that there are no words ready right now (so the client should try again later), as compared to the generator is not going to generate any more words (so the client should just go somewhere else)? Finally, it looks like CWordGeneratorBase operates as a kind of iterator/generator (I don't know the C++ terminology for this particular pattern). I assume there are standard method names for getting the next item and checking whether there are more items for such things, in which case it would be good to follow those names. E.g., if this were in Java, you would implement Iterator<String> and implement next and hasNext.

Again, there needs to be some class-level documentation for CFileWordGenerator to explain how the class should be used. To me, it looks like one might instantiate a single CFileWordGenerator and then repeatedly change the source file. That seems an odd pattern. Much more natural to me as a client who might want to change source files would be to just instantiate multiple CFileWordGenerator objects with the file as an argument to the constructor each time I want to use a new file. In fact, that would let me do (probably useless) tricks like generate words from multiple files all at once. Also notice that your implementation of CFileWordGenerator does not quite match the implication of the documentation of CWordGeneratorBase. The latter says that you must invoke Generate() before GetNextWord(), which makes it sound as though the client must invoke Generate(). But in CFileWordGenerator, Generate() is invoked by SetSource(), so the client does not need to explicitly invoke the former.

It looks like CFileWordGenerator reads the entire file into a buffer; is that right? Is that what you really want to do?

In the interests of internationalization, you might look into whether there are more symbolic approaches to defining a word (that may depend on the character encoding) than searching for the space character.

EventHander.h, EventHandler.cpp

I think we need to discuss the logic/design of the event-handling system a bit more. I'm not seeing quite what I expected in the code.

13 July 2010

These comments are based on <a class="reference external" href=""></a>.

General comments

I'm really pleased to see how often you are committing changes. Each commit looks like a single task and is appropriately logged.

I'm also pleased to see that you've updated your timeline, which I hadn't noticed before today.


The documentation in Doc/development/game_mode.pdf is pretty much spot-on. It tells me the relevant classes to look at and how they relate to each other. It also gives a sense of how the classes are to be used. We should see a similar document for the event-handling system.

As I'm thinking more about this, maybe the best place for this kind of documentation right now is the Wiki. Make a section on the Wiki where you give a high-level overview of the major tasks you have been working on: game mode and the event-handling system. Describe your design, including any relevant information about Dasher architecture. You can think of the "Daily updates," "Project plans," and this page as all related: "Project plans" identifies what you intend to do (and probably doesn't get much more updating); this page gives a bird's-eye view of what you have done; and "Daily updates" gives a ant's-eye view of what you have done. Well, maybe "Daily updates" gives a human's-eye view, and the Git logs give the ant's-eye view. Near the end of the internship, we can decide what exactly should go into the Dasher documentation. Ultimately we need to provide that documentation in a modifiable format (not just a PDF); as we've discussed, we will write such documentation in reStructuredText.


Word generators

Documentation for CWordGeneratorBase should not talk about how it is currently used (only by GameMode). It should only talk about how it is to be used. But overall the documentation is much better, as it tells any client how to actually use (and implicitly, how to implement) a CWordGeneratorBase. The only thing I would do is reduce the words and give a code fragment (a code fragment is worth at least 50 words):

After instantiating a CWordGeneratorBase, GetNextWord returns the next
word; if there are no more words, it returns the empty string.  Thus,
typical usage would look something like:
  wg = some CWordGeneratorBase ;
  std::string w ;
  while ((w = wg.GetNextWord()) != "") {
      // Process word w.

The constructor for CFileWordGenerator appears undocumented. What is iRgen?

The documentation for CFileWordGenerator.GetNextWord() I think is misleading, because it doesn't describe the result of an error condition properly. What if there is an error reading the file? That's not quite the same thing as saying that there are no more words. Really, failures of this sort ought to be handled by exceptions, because it is not the expected behavior.

Is there a reason for exposing the methods in CFileWordGenerator that are not specified by CWordGeneratorBase? E.g., do any existing clients actually invoke GetWholeBuffer? If not, then these methods should not be public---you are placing restrictions on implementation details upon yourselves in advance for no reason.

Why is Generate part of CWordGeneratorBase at all, even protected? Why do you insist that an implementor implement it? This would make sense if, e.g., the CWordGeneratorBase constructor invoked Generate in order to generate the words, so that subclass implementors do not have to. But it doesn't, and indeed, I don't think it should. Can you imagine writing a subclass of CWordGeneratorBase without the Generate method? I certainly can. In particular, it seems that FileWordGenerator can be easily re-written without it, and in a way that cleans up the logic. The constructor opens the file handle, throwing an exception if it can't. GetNextWord reads the next word from the current buffer; if at the end of the buffer, it reads another line from the (presumed open) file handle (again, throwing an exception if it can't), closing the file if we are at the end. Done. Indeed, this also eliminates the rather unfortunate use of instance fields for what ought to be local data, like the word to be returned by GetNextWord.


Where is the documentation explaining how the event-handling system works? E.g, I don't see anything that tells me about what happens if I insert an event while the event queue is being flushed (i.e., from a handler).

Documentation for an instance of an overloaded method should only refer to that instance. E.g., documentation for EventHandler.RegisterListener(Dasher::CDasherComponent*) does not need to refer to EventHandler.RegisterListener(Dasher::CDasherComponent*, int).

Why are EvtListenerTable and EvtListenerCollection part of the (public) API of EventHandler?

Question: since the event processing system is single-threaded, would setting m_bIsDispatching at the beginning of ProcessEventQueue() and unsetting it at the end equivalent to the current setup?

Question: does the order of event listeners matter? If not, would a set make more sense than a list (in order to make the search more efficient in RegisterListener()?

I think it makes more sense to test m_bIsDispatching in RegisterListener() before searching, because you're going to do that search again if m_bIsDispatching == true when you add the pending listeners. The else block in RegisterListener() should be deleted, since it is empty.


Looks great!

29 June 2010

For this code review, I am looking at the branch at <a class="reference external" href=""></a>, and I am focusing on the game mode code in DasherCore.


I'm not sure if the developer documentation is complete, but it looks good.



A minor point: the class documentation for CGameModule refers to an arrow that guides the user, but now you are using a crosshair.

In CGameModule::HandleEvent, it looks like there is a fall-through from the EV_NO_GAME_MODE case to EV_EDIT; is this intentional? If so, it is best to document it, because it looks a little odd. Also, it looks like there is a misplaced break statement on line 61. Finally, since it seems that no action is taken when pEvent->m_iEventType == EV_NO_GAME_NODE, why does the case exist at all?

As I understand the logic, CGameModule::DecorateView() is invoked by CDasherInterfaceBase::Redraw(). And this is where you've been forced to add a bit of "ad-hoc" code. Is this right? If so, it would be good to document this in your developer documentation.

As a design note, it seems that CDasherInterfaceBase ought to maintain a collection of "view decorators," and any module(?) ought to be able to register as such. Then Redraw() could just iterate through the view decorators and have each of them decorate the view as desired. Then CGameModule could register as a view decorator and (at least in principle) CDasherInterfaceBase::Redraw() wouldn't have to know about game mode. What is the reason you didn't take this approach?


I'm beginning to see just how difficult it is to separate out the game mode logic. I think it would be good to document this in your developer documentation. As part of that documentation, you should identify which non-Game classes have game mode entanglements.

Personal tools
NSF K-12