Documentation and regression testing
Moderator: Forum Moderators
-
jacob
Documentation and regression testing
In a private mail to David, I suggested improving the developer documentation of Wesnoth. He suggested that I should post to this forum.
In this post, I wish to argue what I believe the benefits of improved developer documentation is, and what problems I see with the current version in CVS in this regard.
I welcome all suggestions, and would like to know what you think of it.
I realize there are many other things which need to be done, such as bug fixing, but I believe some of these suggestions would make it easier to maintain the code, which I hope will result in a more proactive and systematic approach to bugfixing. Aditionally, I think it will be easier for new developers to understand the source.
If you wish, I could work on implementing the suggestions for a single source file, so you can get an idea of what I mean and whether it is worth it.
---------------------------------------------------------------
Table Of Contents
1. Why shorter functions?
2. Examples of poor comments.
3. Suggestions for improving the documentation.
4. Generated documentation.
5. Regression testing.
---------------------------------------------------------------
1. Why shorter functions?
Some of the functions in the source code are very long. Examples:
display.cpp ( display::draw_tile() )
game.cpp
multiplayer.cpp
The following are a few reasons why I think it would be nice if, at least in the future, care is taken to avoid long functions:
Automated testing.
It is not possible to test a function like play_game() effectively using automated regression or unit tests. Why? Because complex functions which do not take input, process it, and returns output, and also has GUI code are hard to test. On the other hand, if play_game() makes use of many smaller functions, these could be tested, thus effectively also testing play_game().
Automatic or member variable?
In the new coding conventions, David tell us to append an underscore to all member variables, because then it will be easier to tell whether a variable is an automatic or member variable.
I believe the real problem is the use of long functions; if most functions were at most about ten to twenty lines, it would be easy to tell because there would be fewer automatic variables.
Is it important? David thought it was sufficiently important to mention in the coding conventions.
Plain good software engineering.
In just about any textbook or online discussion you will find about software architecture, you would be advised to use short functions. I believe this is because a short function is much easier to understand than a long function.
---------------------------------------------------------------
2. Examples of poor comments.
Let me give a couple examples of what I find problematic in the comments:
menu::change_item():
A comment claims change_item() is "dangerous", but does not say why. In other words, you need to look at the implementation in order to use the method. Unfortunately, it is not obvious from the implementation of change_item() either (it is just a single line with an assignment), so you need to look at all lines where the items_ member variable is used.
config::save_file():
This one had no comments in the previous version. I think such a function really should state what happens in case of failure, and it is obvious that saving a file can fail. If the file may have been corrupted but there is no way to tell, which was previously the case, there should be a comment stating that is the behaviour.
---------------------------------------------------------------
3. Suggestions for improving the documentation.
Based on the above, I propose the following changes:
4. Generated documentation.
In addition to improving comments, I suggest we also add automated developer documentation.
I asume you are all familiar with tools such as Doxygen or Javadoc, but just to be sure, here is a brief explanation:
Doxygen parse the source code, like a compiler, and output documents describing the static structure of the source code. It looks much like a header file, but it is crossreferenced, indexed, has automatic diagrams, and more. The overhead of using Doxygen is minimal. Rather than this:
...you just use comments with three slashes rather than two, like this:
Then we add a doxyconf file to the root project directory and the documentation can be built by typing 'doxygen'.
Example of what it can look like.
---------------------------------------------------------------
5. Regression testing.
If the documentation is improved, it will be easier to implement regression tests.
I asume you are all familiar with this, but just in case, a regression test is a set of test cases intended to automatically test parts of the source code.
The comments of a function should be seen as a contract of what the function must do. The regression tests will verify whether the function implement its part of the contract, i.e. that it produce the correct output.
An example of a test case:
Why go to the trouble of writing a program to test this, when we can just go through the source line by line and look for ourselves? Because modifying the source can introduce bugs. When, or before, changes are checked into CVS, the regression tests can be run to see whether the changes honour the functions interfaces.
I realize there is overhead to this, but it would not affect you if I developed the test cases. Also, it is not wasted time if it catch bugs after or before they are introduced.
In this post, I wish to argue what I believe the benefits of improved developer documentation is, and what problems I see with the current version in CVS in this regard.
I welcome all suggestions, and would like to know what you think of it.
I realize there are many other things which need to be done, such as bug fixing, but I believe some of these suggestions would make it easier to maintain the code, which I hope will result in a more proactive and systematic approach to bugfixing. Aditionally, I think it will be easier for new developers to understand the source.
If you wish, I could work on implementing the suggestions for a single source file, so you can get an idea of what I mean and whether it is worth it.
---------------------------------------------------------------
Table Of Contents
1. Why shorter functions?
2. Examples of poor comments.
3. Suggestions for improving the documentation.
4. Generated documentation.
5. Regression testing.
---------------------------------------------------------------
1. Why shorter functions?
Some of the functions in the source code are very long. Examples:
display.cpp ( display::draw_tile() )
game.cpp
multiplayer.cpp
The following are a few reasons why I think it would be nice if, at least in the future, care is taken to avoid long functions:
Automated testing.
It is not possible to test a function like play_game() effectively using automated regression or unit tests. Why? Because complex functions which do not take input, process it, and returns output, and also has GUI code are hard to test. On the other hand, if play_game() makes use of many smaller functions, these could be tested, thus effectively also testing play_game().
Automatic or member variable?
In the new coding conventions, David tell us to append an underscore to all member variables, because then it will be easier to tell whether a variable is an automatic or member variable.
I believe the real problem is the use of long functions; if most functions were at most about ten to twenty lines, it would be easy to tell because there would be fewer automatic variables.
Is it important? David thought it was sufficiently important to mention in the coding conventions.
Plain good software engineering.
In just about any textbook or online discussion you will find about software architecture, you would be advised to use short functions. I believe this is because a short function is much easier to understand than a long function.
---------------------------------------------------------------
2. Examples of poor comments.
Let me give a couple examples of what I find problematic in the comments:
menu::change_item():
A comment claims change_item() is "dangerous", but does not say why. In other words, you need to look at the implementation in order to use the method. Unfortunately, it is not obvious from the implementation of change_item() either (it is just a single line with an assignment), so you need to look at all lines where the items_ member variable is used.
config::save_file():
This one had no comments in the previous version. I think such a function really should state what happens in case of failure, and it is obvious that saving a file can fail. If the file may have been corrupted but there is no way to tell, which was previously the case, there should be a comment stating that is the behaviour.
---------------------------------------------------------------
3. Suggestions for improving the documentation.
Based on the above, I propose the following changes:
- Reorganize some of the code, so there will be fewer long monolithic functions.
- Add more comments, in particular to interfaces (function signatures and classes).
- Add auto generated developer documentation (I prefer Doxygen)
- Add unit tests to some of the more mature classes and functions.
4. Generated documentation.
In addition to improving comments, I suggest we also add automated developer documentation.
I asume you are all familiar with tools such as Doxygen or Javadoc, but just to be sure, here is a brief explanation:
Doxygen parse the source code, like a compiler, and output documents describing the static structure of the source code. It looks much like a header file, but it is crossreferenced, indexed, has automatic diagrams, and more. The overhead of using Doxygen is minimal. Rather than this:
Code: Select all
//brief explanation.
//
//longer, more
//elaborate explanation
class menu {
}
Code: Select all
///brief explanation.
///
///longer, more
///elaborate explanation
class menu {
}
Example of what it can look like.
---------------------------------------------------------------
5. Regression testing.
If the documentation is improved, it will be easier to implement regression tests.
I asume you are all familiar with this, but just in case, a regression test is a set of test cases intended to automatically test parts of the source code.
The comments of a function should be seen as a contract of what the function must do. The regression tests will verify whether the function implement its part of the contract, i.e. that it produce the correct output.
An example of a test case:
Code: Select all
// test whether load_file() will correctly throw an
// exception if a file is not found.
...code to test this...
I realize there is overhead to this, but it would not affect you if I developed the test cases. Also, it is not wasted time if it catch bugs after or before they are introduced.
Jacob,
Thank you for your post. I agree with most of your ideas. Shorter functions would indeed be great for all the reasons you give, and we should work towards better modularity.
Something else we can work towards is trying to modularise things into classes a little better. For instance, the class turn_info used to be an even bigger function, instead of a class. Turning big functions into classes allows member variables to be maintained instead of passed to many different sub-functions.
I think that distinguishing between local and member variables using a naming convention is important regardless of the length of functions. Even in short functions, it can be confusing if you don't instantly know if a variable is a member or not.
Although I do think that commenting interfaces is important, I think that with strict use of const, and other C++ idioms such as pointers vs references, much of an interface can be implicitly documented. One of the biggest things needed is a comment to say what exceptions a function might throw. For instance, I think that,
is as good as,
The first example is nice and terse, and is as obvious as the second version. Only if it's not obvious under which conditions an exception might be thrown should the comment be more verbose.
...
I do think that regression testing is a good idea, although I do express the reservation that much of the code will not be possible to regression test (even if it is better broken into smaller functions).
If you're willing to write the test cases, that'd be even better
...
I'm happy with the use of Doxygen, and if all we have to do is put three slashes for Doxygen comments, that'd be great.
David
Thank you for your post. I agree with most of your ideas. Shorter functions would indeed be great for all the reasons you give, and we should work towards better modularity.
Something else we can work towards is trying to modularise things into classes a little better. For instance, the class turn_info used to be an even bigger function, instead of a class. Turning big functions into classes allows member variables to be maintained instead of passed to many different sub-functions.
I think that distinguishing between local and member variables using a naming convention is important regardless of the length of functions. Even in short functions, it can be confusing if you don't instantly know if a variable is a member or not.
Although I do think that commenting interfaces is important, I think that with strict use of const, and other C++ idioms such as pointers vs references, much of an interface can be implicitly documented. One of the biggest things needed is a comment to say what exceptions a function might throw. For instance, I think that,
Code: Select all
void write_file(const std::string& fname); //throws file_write_error
Code: Select all
//This function will throw a file_write_error if it fails to write to the file
void write_file(const std::string& fname);
...
I do think that regression testing is a good idea, although I do express the reservation that much of the code will not be possible to regression test (even if it is better broken into smaller functions).
If you're willing to write the test cases, that'd be even better
...
I'm happy with the use of Doxygen, and if all we have to do is put three slashes for Doxygen comments, that'd be great.
David
“At Gambling, the deadly sin is to mistake bad play for bad luck.” -- Ian Fleming
-
jacob
David, I agree it would be great to modularise functions into classes. This would have several desirable results, in my opinion.
This is fine with me, but I do hope it will be used consistently. I did spot a few source files where this convention was not used. I will make a note of always using this convention for member variables, and hope other developers will do the same.distinguishing between local and member variables using a naming convention is important regardless of the length of functions.
Ok, this is fine with me too. I am used to more verbose comments, but I will (and already have) scaled down on it. I like to have overview of source files, too, and overly long comments which state the obvious can sometimes get in the way. Still, I think some of the source files would not be worse off with clarification or a brief explanation. It is easier to read your own code than others.The first example is nice and terse, and is as obvious
I am sure you are right, but still, I think it is better than nothing. I could start with things that lend themselves well to this, like the config class (I think), and gradually implement more of this. However, I think it only makes sense to do so where existing methods and functions in header files do not change too often, as then the regression tests would have to be updated.I do express the reservation that much of the code will not be possible to regression test
I will customize a doxyconf configuration file for Wesnoth and post it here afterwards. The default configuration file may possibly not be well suited.I'm happy with the use of Doxygen,
To clarify, this convention is used only for non-public member variables. I have updated the documentation in Wiki.jacob wrote:This is fine with me, but I do hope it will be used consistently. I did spot a few source files where this convention was not used. I will make a note of always using this convention for member variables, and hope other developers will do the same.distinguishing between local and member variables using a naming convention is important regardless of the length of functions.
The main violator of this that I know of is the config class, which started life as a struct with a public interface, it'll eventually be fixed.
You are welcome to put longer comments if you want. IMO it's best to document things with the interface first, and use comments for anything that is still not clear.jacob wrote:Ok, this is fine with me too. I am used to more verbose comments, but I will (and already have) scaled down on it. I like to have overview of source files, too, and overly long comments which state the obvious can sometimes get in the way. Still, I think some of the source files would not be worse off with clarification or a brief explanation. It is easier to read your own code than others.The first example is nice and terse, and is as obvious
David
“At Gambling, the deadly sin is to mistake bad play for bad luck.” -- Ian Fleming
-
jacob
I have attached a configuration file for Doxygen, along with a few other files, which should be unzipped in the project root directory.
Note that Graphviz needs to be installed in order to see the diagrams. If it is not, there will just be dead links instead of diagrams. It can generate class diagrams too, but they got too big for the display class, so I changed it to a dependency graph instead. The search feature is implemented with PHP, so it does not work locally. The generated files currently take up about eight MB, but this can be reduced by not including the code browsing feature.
Of course, methods are not actually documented because it is necessary to use /// rather than // for comments. Other than that, various special commands can be used in these comments, but we do not have to use them. I particularly like the \todo, \throw, \pre and \post commands, though.
Once generated by typing 'doxygen' from the project root directory, the documentation can be found in:
doc/doxygen/html/index.html
It still needs a little more work, but it does show roughly how I think it could be organized.
Note that Graphviz needs to be installed in order to see the diagrams. If it is not, there will just be dead links instead of diagrams. It can generate class diagrams too, but they got too big for the display class, so I changed it to a dependency graph instead. The search feature is implemented with PHP, so it does not work locally. The generated files currently take up about eight MB, but this can be reduced by not including the code browsing feature.
Of course, methods are not actually documented because it is necessary to use /// rather than // for comments. Other than that, various special commands can be used in these comments, but we do not have to use them. I particularly like the \todo, \throw, \pre and \post commands, though.
Once generated by typing 'doxygen' from the project root directory, the documentation can be found in:
doc/doxygen/html/index.html
It still needs a little more work, but it does show roughly how I think it could be organized.
It's great to see that you've decided to use Doxygen to document the code. I have a few questions though (without having read the file itself):
Which style do you have in mind? Doxygen can be configured to use several different styles. I myself prefer the Javadoc style, e.g.
/**
* Some stuff
*/
But I guess tripple-slashes are ok as well. I think it looks much nicer and is easier to read if you leave a space before starting the comment though.
///
/// Stuff
///
In which way do you specify the brief desrciption? I prefer the Javadoc style here as well (JAVADOC_AUTOBRIEF=yes), but I guess any way is ok.
Ah, and there should probably be a wiki-page describing how to use doxygen and which styles are used. I can probably write the wiki if you don't have the time (going a way for a week, though, so it has to be when I come back).
cheers,
- enki
Which style do you have in mind? Doxygen can be configured to use several different styles. I myself prefer the Javadoc style, e.g.
/**
* Some stuff
*/
But I guess tripple-slashes are ok as well. I think it looks much nicer and is easier to read if you leave a space before starting the comment though.
///
/// Stuff
///
In which way do you specify the brief desrciption? I prefer the Javadoc style here as well (JAVADOC_AUTOBRIEF=yes), but I guess any way is ok.
Ah, and there should probably be a wiki-page describing how to use doxygen and which styles are used. I can probably write the wiki if you don't have the time (going a way for a week, though, so it has to be when I come back).
cheers,
- enki
-
jacob
David is the main developer, so I think he should decide. As he prefers brief comments, I figured /// would be a better match, because most of the current comments are one-liners. This style can also be used for longer comments. In other words, it would be like my previous example:enki wrote: I myself prefer the Javadoc style,
/// Brief description.
///
/// Longer description.
I also think this is more readable, but again, the majority of the code does not because this is how David seem to prefer it. I have other preferences too, but I can adapt.enki wrote: I think it looks much nicer and is easier to read if you leave a space before starting the comment though.
The Javadoc way. This was also chosen out of a desire to keep comments short, so it will not be necessary to use a \brief special command.enki wrote: In which way do you specify the brief desrciption?
Yes, there should be such a document, and as far as I am concerned, you are welcome to write it, but I did not want to write one before people are ok with this.Ah, and there should probably be a wiki-page describing how to use doxygen
I think that if you have three slashes before the comment, a space looks better. However I think it should be at the programmer's discretion.jacob wrote:I also think this is more readable, but again, the majority of the code does not because this is how David seem to prefer it. I have other preferences too, but I can adapt.enki wrote: I think it looks much nicer and is easier to read if you leave a space before starting the comment though.
David
“At Gambling, the deadly sin is to mistake bad play for bad luck.” -- Ian Fleming
I can write the "How to use Doxygen" wiki-page once it's been used in a few classes (just so I understand how people will use it in wesnoth).
I haven't really used Doxygen (mostly been working with Javadoc the last couple of years, hince my fondness for that style) in a few years and I guess that lot of things have changed (or at least been added) but it's a perfect opportunity to reacquaint myself with it again
Oh, and I can probably document some classes and send as patches if you like.
I haven't really used Doxygen (mostly been working with Javadoc the last couple of years, hince my fondness for that style) in a few years and I guess that lot of things have changed (or at least been added) but it's a perfect opportunity to reacquaint myself with it again
Oh, and I can probably document some classes and send as patches if you like.
Regressiontesting
Jacob, what framework (if any) do you have in mind for the regressiontesting? I've worked a bit with JUnit, which is quite nice, and I believe there are similar frameworks for C++. Just a quick google resulted in CppUnit and Unit++, but I'm not sure about how good they are. Do you have anything else in mind? I'm not sure if these frameworks are suitable for wesnoth, but the might be worth considering.
One problem with regressiontests is that if you change the interface you probably have to update the tests as well. If the interfaces aren't stable, it might lead to a lot of extra work?
One problem with regressiontests is that if you change the interface you probably have to update the tests as well. If the interfaces aren't stable, it might lead to a lot of extra work?
-
jacob
Re: Regressiontesting
Sounds great.enki wrote: I can write the "How to use Doxygen" wiki-page once it's been used in a few classes
This would also be great, though I do not currently have the privileges to get the patches accepted myself.enki wrote: I can probably document some classes and send as patches if you like.
I was thinking of CppUnit, but I still need to ponder how suitable it is to Wesnoth. If you have a particular preference, we could discuss it.enki wrote: what framework (if any) do you have in mind for the regressiontesting?
This is true and something which must be taken into account. My plan for this is to have a look at the CVS and see which header files appear not to have been modified too much for a while, though additions are ok. This seems managable using the CVS web client at Savannah.enki wrote: if you change the interface you probably have to update the tests as well. If the interfaces aren't stable, it might lead to a lot of extra work?
Re: Regressiontesting
I don't have any particular preferences since I've only used JUnit, the other two I just found today. Have you used CppUnit? Is it actively being developed? If it works like JUnit (and it should, since it's a portjacob wrote:I was thinking of CppUnit, but I still need to ponder how suitable it is to Wesnoth. If you have a particular preference, we could discuss it.enki wrote: what framework (if any) do you have in mind for the regressiontesting?
) I think it can be used for some parts of wesnoth at least. It's always tricky to write tests for gui-programs.Sounds good.jacob wrote:This is true and something which must be taken into account. My plan for this is to have a look at the CVS and see which header files appear not to have been modified too much for a while, though additions are ok. This seems managable using the CVS web client at Savannah.enki wrote: if you change the interface you probably have to update the tests as well. If the interfaces aren't stable, it might lead to a lot of extra work?
-
jacob
Re: Regressiontesting
Enki, yes, I have used both CppUnit, though not as much as I probably should, and JUnit and they are similiar. I believe it is being actively developed. I will try out a few alternatives.
I will post when I have a better feeling for which parts of the game it would make sense to regression test.
I will post when I have a better feeling for which parts of the game it would make sense to regression test.
Re: Regressiontesting
Sounds great. Pleace let me know your conclusions, I'm always interested in new testing toolsjacob wrote:Enki, yes, I have used both CppUnit, though not as much as I probably should, and JUnit and they are similiar. I believe it is being actively developed. I will try out a few alternatives.
I can try to look into this myself, won't be for a week or two, though. I'm off for a week now and I think I'm going to have loads of job-related work waiting for me when I come back.jacob wrote:I will post when I have a better feeling for which parts of the game it would make sense to regression test.
cheers,
- enki
-
jacob
On IRC, we discussed making the documentation available online, possibly building the documentation on the server with a cronjob. How do I get in touch with Jaramir?
I have attached the latest version of the documentation, which includes better namespace documentation and a brief howto on commenting, which enki could perhaps use as a starting point. I am not sure whether to send this here or as a patch.
I have attached the latest version of the documentation, which includes better namespace documentation and a brief howto on commenting, which enki could perhaps use as a starting point. I am not sure whether to send this here or as a patch.