Wesnoth Philosophy II: Where Next?

[Ayin]

Wow, that's a huge reply :)

[ on abstracting SDL ]
1- Unified language.
The SDL functions are C, we're using C++. The app (Wesnoth) should not mix languages - unless its absolutely neccessary and its not the case here. Wesnoth will be hacked on by many people with various language skills. It's not good needlessly demanding higher coding skills - particularly with Free Software. The GUI framework, otoh, has no choice, SDL is C, so the framework must use a mix, but it should not demand app developers do so.

[... advantages of abstracting SDL...]

For "consistency" and "readability", there already is some work done: SDL operations are defined in the files sdl_utils.[ch]pp. Granted, this is not done the Object-Oriented Way, and one should have encapsulated all SDL operations in nice classes, providing some nice syntax to SDL manipulations. Even if I am inclined to think that you slightly exagerate the advantages of providing an OO layer around SDL, I agree that, if it is done well, it may be nice.
The main problem being, of course, doing it well :)

But it should, IMO, stay an OO / C++ version of the SDL API, extended for common surface manipulations. What it should (can?) not be, is a much higher-level abstraction: it would certainly require rewriting large parts of the Wesnoth source code. Using things like class WMap and WHexMap -- by the way, what are the W for? Please do not tell me they are for "Wesnoth" -- would mean rewriting a rather big part of the game. Much work, for a result that would be... Well, different, but better?

I recommend, anyway, reading this excellent text on what the author called "leaky abstractions" http://www.joelonsoftware.com/articles/ ... tions.html

Exactly, STL typdefs are all over the place, not centralised and coherrent.

There also are ints, floats, and other data types all over the place, that's not a reason for us to abstract those :p

[ on wrapping STL data structures ]

This doesn't prevent app developers from using STL if they want to. You see, we take the STL classes and teach them about Wesnoth. They now model Wesnoth data structures and algorithms, not generic abstractions.

If the new class provides significant functionality over the basic STL container, a agree this is good design. If it is just a matter of "wrapping" the STL container, it is over-designing your application, and this certainly would harm readability.

I, for one, certainly would prefer this

Code: Select all

typedef std::vector<unit> unit_list;

unit_list my_list;
my_list.push_back(unit(...));

to something like that:

Code: Select all

class unit_list
{
public:
    unit_list();
    unit_list(unit_list &);
    
    void add_unit(unit &)
private:
     std::vector<unit> list_;
};

unit_list::unit_list()
{};

unit_list::unit_list(unit_list &u) : list_(u.list_);
{
}

unit_list::add_unit(unit & u)
{
    list_.push_back(u);
}

unit_list my_list;
my_list.add_unit(unit);

That is true that the second form may be extended, and provide much more functionality than a simple typedef as shown in the first version. But this is also much more verbose, requires writing additional code, and also requires documenting its interface, all of which lead to:
1) more development time
2) code harder to understand.

You may, when taking such a design decision, take into account the benefits of each solutions: "Do I need the additional flexibility of creating a new class for my data structure, or is extending an existing class just sufficient? If creating a specialized data structure, with a specialized behaviour, is worth the effort (in the way it provides a nicer syntax, makes code easier to understand -- and IMO, just having semantic method names is not enough here -- or is simpler to code,) then go ahead. Else, save yourself and other programmers the effort.

My point here is that having a generic, fits-all, solution to the "how to design code" problem is certainly not a good idea. As some things certainly are to be improved in the Wesnoth code, just getting rid of SDL and only using it in delimited reserves certainly is not, in my opinion, an improvement.

Some more points:

1- Readability.

A) Consistent naming convention.
STL members are horrible! To add/remove to a list you must push_xxx() and pop_xxx(), which is academic jargon. UnitList.addFirst( Unit ), is far more consistent and readable.

de gustibus et coloribus...

I, for one, think MixedCaseIsJustPlainUgly. And that push_back, push_front, etc is a nice and consistent syntax. I won't argue about this, it is just a matter of taste, but be aware that for now, Wesnoth Coding Standards do not include mixed case, but use underscore_separated lower-case.

B) Abstract complex data structures.
You can customise STL classes to do Wesnoth specific things, like combine two STL classes into one Wesnoth class, e.g. UnitList, and then search the data structure using custom members, e.g. UnitList.FindRanged( attackMagic ) and UnitList.FindTrait( "Scholarship" ) (heh). The STL (or any class for that matter) should not be required reading to understand the app. I don't care how the computer handles UnitList, I just want to know how the game works. Once the game logic is understood, finding (logical) errors or making improvements is much easier.

As I stated above, there is no single, fits-all answer here. If your UnitList only has the need for an iterable interface, there is no need to define a new class. If it needs additional features, then it is worth considering extending it (but it may be a good idea to keep the compatibility with STL iterable objects, so your new class can be used as such.) Keep in mind that any sufficiently skilled C++ programmer knows how to use STL APIs.

Abstraction, data hiding, etc. concepts are a good thing, used wisely. But over-using them is just overkill, and may make your project drown.

C) Avoid iterators.
Don't you just hate them? Let the Wesnoth classes handle them and simply pass and receive Wesnoth objects. KISS

Err, iterators are one of the most fundamental concepts of C++ programming. You may say that C++ iterators' syntax is ugly, I won't argue that, it actually is quite ugly. But if you need to iterate over some stuff (list the units in your UnitList class, for example), you will end needing some kind of way to iterate over the list.

D) Avoid obscure syntax.
std::list<std::map<int,std::vector<Tile> > >(), is tough to read - nevermind understand, but TileList() is instantly groked.

Just use typedefs then:

Code: Select all

typedef tilelist std::vector<Tile>;
typedef tilemap std::map<int, tilelist>;
typedef list_o_tilemaps std::list<tilemap>;

Anyway, I fear that what you are trying to do is like saying "I don't like the way Wesnoth is programmed, so I will program it differently". While the design decisions in Wesnoth are prone do be discussed (this is not rocket science, after all,) you should understand that those are not considered "bugs one should fix", but features, so you will certainly have to prove, and not just with theory, that your choices in design are better than ours (well, Dave's, actually ;) ). I am not hostile to this, but... This certainly is very much work for you, for a very uncertain result. Anyway, I wish you good luck ;)

[Jyhem]

I'd like:

* An "import campaign" functionality. We intend to remove all but the finished campaigns. This is good. But then we should make it easier for players to add other campaigns. After all, after 1.0 is out, 2.0 will probably be rather unplayable for a while. So telling people "grab the new version, there are 3 more scenarios at the end of the campaign" won't do anymore.

* A link or a mention of the wesnoth website, so people know where to get upgrades, new campaigns, whatever (tee-shirts ? )

[Dave]

Ayin has mostly answered things very well. I just have a couple of things to add...

Wesnoth will be hacked on by many people with various language skills. It's not good needlessly demanding higher coding skills - particularly with Free Software.

Simply, Wesnoth is written in C++. People will need to know C++.

We have fairly high coding standards in Wesnoth, so people who work on it will need to know C++ well.

Wesnoth is written in one language: C++. We aim to make all code in Wesnoth conforming to the C++ standard. There is no mixing of languages here.

Each developer rewrites the same function using different algorithms.

Hmm....which code in Wesnoth has the same algorithms written in multiple places?

STL members are horrible! To add/remove to a list you must push_xxx() and pop_xxx(), which is academic jargon. UnitList.addFirst( Unit ), is far more consistent and readable.

I disagree entirely. I prefer the standard library names.

The STL (or any class for that matter) should not be required reading to understand the app.

The 'STL' is part of C++. Knowing C++ is required to understand Wesnoth code. So yes, you need to understand the STL to understand Wesnoth code.

To me this is like saying that you shouldn't have to understand hashes to understand Perl code, or have to understand dictionaries and arrays to understand Python code.

David

[Blackbeard]

Thanks for the replies, I know you have better things to do. You have given me much to think about (and some work ), so I won't reply now, but when I do, I'll try to be brief.

[Dave]

Oh one other thing: you should (edit) NOT publically derive classes from standard containers. They are not designed to be derived from. They do not contain virtual destructors.

David

[Ayin]

I assume you meant "you should NOT publically derive (...)," ?

[Dave]

I assume you meant "you should NOT publically derive (...)," ?

Oh yes, sorry

David