[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
> I should have free time on friday. I'll try to commit it then.
Well, today isn't friday, but I've commited it. Note that I didn't get
round to doing all the source cleanup I had intended to do.
Attached is my original `referee report' for the demo. Many of the things
in there are corrected or dealt with in the current version, but not all
of them. Some help there would be appreciated, though I'll get back to
some of them later this week.
There also isn't a working buildsystem. I'll commit my own for the time
being, which I use myself in more or less this form. It works fairly well
for *nix, DJGPP and MinGW, but it isn't ideal in this case.
Evert
Log message:
Imported the skater demo game.
There are some fixes and code cleanup that may be nescessary. In
particular:
* I used indent to format the code. This should be mostly ok, but proably
not everywhere.
* Some function names are in MixedCaps instead of underscore_notation.
Should be fixed for consistency. A sed guru may want to help out here.
* Commenting. Much of the code is still not adequately commented.
* It currently comes with an old thanks._tx and readme.txt. Should use
current versions.
* Consistency in indenting of struct elements should be checked as well as
in using/not using extern for external functions (should use extern).
Things I did do:
* Use demo.cfg instead of allegro.cfg to store and load demo-specific
settings.
* Argument list in declaration is never ()
Things that need to be done as well:
* Screen for some unwanted C99-isms. Actually, I compiled the code with
-std=c89 and it passed, so probably no need to do it manually.
* Proper build system.
I'll commit the buildsystem I use myself and used for this demo as well as
a temporary solution, but it needs a better one eventually.
Referee report for proposed new Allegro Demo - current issues.
Code:
* strdup is not an ANSI C89 function, use Allegro's ustrdup instead.
* Use snprintf instead of sprintf throughout.
* Actually, use Allegro's unicode text functions.
* Two warnings:
level.c:230: warning: unused parameter 'radius'
physics.c:85: warning: unused parameter 'r'
Either remove those parameters, or find a use for them. Or document why they
are unused.
* Justify why Allegro's dialog manager isn't used for the menu code. There's
no need to use the standard widgets, but implementing a new dialog manager
in the demo in addition to the one in Allegro seems overkill. It also doesn't
properly demonstrate Allegro's functionality in the sense that there is
already a dialog manager there.
Cosmetic:
* Indentation is three spaces. Replace all tabs used for a single indentation
level with three spaces. This must be done manually because a tab also
replaces eight spaces at deeper indentation levels. A clever regular
expression can probably take care of this (replace all tabs followed by a
non-white character with three spaces).
* Rename functions and variables to use lower_case_and_underscores instead of
MixedCaps. Can easily use sed to do this.
* Always declare functions with an argument list, or (void). Never ().
* Be consistent in declaring functions in headerfiles with the extern keyword
or without. Prepend all function declarations in headerfiles with an explicit
extern.
* There are inconsistencies in indenting for some defines as well: between
the #define keyword and the symbol there should be just one space.
* Put #include <> lines before #include "" lines, or at least don't mix them!
Technical:
* Is there a reason to store some bitmaps as binary data in the datafile rather
than as a bitmap?
* About the datafile - either strip the origin property, or store it as a
relative filename.
* Would it be possible to use a config file other than allegro.cfg for the demo
settings and call set_config_file() after Allegro's drivers have been
initialised? This is probably minor, but the presence of a fresh allegro.cfg
causes Allegro to ignore the one in my home directory, which means that I
don't get MIDI sound during the game if I start it a second time. This is
arguably an issue with Allegro itself rather than the demo, so mark it as a
`nice to have' feature rather than a requirement.
Gameplay:
* I don't care if you can set default keybindings by distributing a config file
with the demo: there needs to be a default fallback in case no config file
is present. This isn't even difficult, just add it as the default parameter
to get_config_int.
Commenting:
* There is insufficient commenting in the code, even if it is fairly
self-documenting in many cases. For instance: what is demo_textprintf()?
Why is it there? What does it do?
Functions should be preceded by a line or two of comments that describe
their function, large chunks of code should have a descriptive comment too.