Clash Coding Standards Discussion

[tjawatts]

Hi

It would be great if I could get a look at the code. Should be able to iron out any problems with protability and find any OS specific code. Then I could look at the method of deployment if noone else has done that.

Finally, I can do some actual coding, I dont know what to do, I dont want to encroach on anyone elses territory! If you are coding a specific section and would like me to do some sort of subsection for you let me know. Otherwise I can just look at the code and find something which hasnt been implemented yet!

Cant wait to get started!

Tony

[Mark_Everson]

Hi Tony.

Sent the code this AM, tho it isn't completely current as I explained in the email. There shouldn't be any OS-specific code in there.

I think the MP coding that you mentioned in the other thread is a great thing for you to start in on! It will make the demos much more interesting since there's no AI implemented so far.

[Gary Thomas]

This thread seems as good as any to discuss another area that I am a bit uncertain about.

It is my view that the map should be updated every tick, with units possibly partway between squares. Every tenth tick the end of move update takes place.

However the actual means needs to be determined.

The actual tick count could be governed by a turn manager, which can notify all the "moveable" objects that it is time to move. The movement manager should run in a separate thread so that it doesn't interfere with the event dispatcher.

This could be using an observer/observable system, with the update taking place at the end of the notify call.

Personally I don't like the fact that every observer has to check some sort of indicator in order to find what action to take. This seems like a step backwards to the Windows style messaging system, rather than an object oriented approach.

My own preference would be a tailored solution, whereby each move observer implements an interface (called Tick, with a singly method called, for example, move()), and registers with the move manager (and I will have to write the code which stores the registered objects, and calls tickMove on each one, registration wiil be by calling MoveManager.register(Tick object) where object is the registering object). Then the registered objects will automatically perform the correct action without an "if ("TickMove".(String)param))" type of test.

At any time a unit may be partway between squares. It should return the map square was last in, until it has fully made the move to the new square.

Cheers

[Mark_Everson]

Hi Gary:

I don't get all the details on your proposal. Can you give a quick example of which are the "move observers", which "objects" you have in mind, etc.? It was just a abit too unspecific for me. so if you say "observer = square or unit", "object = unit", or whatever is the case, then I'll be able to at least understand the proposal if not fully evaluate its merits!

That said, so long as it works relatively efficiently given the relative numbers of objects etc. that we expect to have, I don't think I'm gonna squawk at whatever approach you'd like to take...

[Gary Thomas]

In the immediate case, the only observing objects will be military task forces. At a higher level will be the turn manager, invoked every 10 ticks.

Cheers

[Gary Thomas]

A comment from Mark, concerning the latest code I sent him, to the effect that there have been a lot of changes, prompted me to make this post attempting to explain what I am trying to do.

There were a number of aspects of the previous code which I felt were unsatisfactory, mainly from the point of view of a new coder coming in and trying to understand what is going on (and I have personal experience of this problem). My comments should be regarded as general advice on some important factors in program design, rather than as criticism.

There are three areas that I want to comment on: Java standards, object oriented design, and programming methods.

1. Java standards.

There is a small but nevertheless excellent book called "The Elements of Java Style" by Allan Vermeulen, Scott Ambler, Greg Bumgardner, Eldon Metz, Trvor Misfeldt, Jim Shur and Patrick Thompson (a cast of thousands) published by Cambridge University Press. This book has a list of 108 "rules". Each rule is explained and justified. The book is pure gold and should be on every Java developer's desk.

It has many comments about the physical appearence of code, about naming and about documentation and about programming style.

For example, rule 6 is "Break up long lines". I personally get very irritated when I have to use the scroll bar to see what the coder has written in column 243. Break the line before column 80. It then becomed readable.

Another example is rule 8, "Don't use hard tabs". Many developers don't realize that what looks fine on their machine will be a complete disaster in someone else's development environment. Always use spaces, then it will look the same in all environments.

The main area that I want to stress is the Documentation Conventions section, which starts out with

Document the public programming interface of your code so others can use it correctly and effectively. Document the private interface and internal implementation details of your code so others can maintain and enhance it.
Always assume someone who is completely unfamiliar with your code will eventually have to read and understand it. In fact, if enough time passes, your own code may become unfamiliar, so this person may even be you!

JavaDocs are a critically important feature of Java, and one of the most significant aspects of the language. The JavaDoc comments should not be coding explanations (these have the normal // comment style) but structural explanations of the type found in the Java documentation. List every parameter, and the return value if any. Don't skip this or the next person will be left wondering why it was left out.

2. Object oriented design.

The phrase "object oriented design" is a quite meaningful description of what is wanted. Make things into self-contained objects wherever possible. There is another very good book "Effective Java" by Joshua Bloch, published by Addison-Wesley, which covers many of these areas.

There are two aspects which I felt were severely lacking in the code I found.

One was a tendency to have long list of data fields in an object, covering all the different aspects of the object's behaviour. Each time someting new came up some more fields were added. This distressing habit is actually perpetuated in the Borland JBuilder environment. If you use their drag and drop methods to construct a GUI, it just gives a long, undigestible, and incomprehensible list of components. For this reason very few (none?) experienced deveopers use the drag and drop system. Organizing everything later is more difficult than writing it correctly, by hand, the first time.

As an example, the MapSquare object in Clash holds the data realting to an individual square on the map. This includes

• The physical terrain
• Economic resources
• Military units present
• Population information including ethnic groups
• Ownership information

This led to a formidible list of fields.

One of the things I have been doing is to reduce this list. So far, I have grouped the physical terrain data into a subsidiary class called, strangely enough, TerrainData, and similarly grouped the military data. The rest I have my eye on, but there are more pressing matters.

Another area I have been progressing is the idea of empty classes which do nothing except announce their presence, or perhaps return an identifying string. There are a whole lot of very good reasons for using such classes (they replace the enum construct of C++). In Clash, categories of militay elements ("foot", "horse" and so forth) and of terrain ("flat", "rolling" and so forth) are designated in this manner. Each class is immutable and has only one static instance. The advantages of this approach (compared to int indices or string names stored as a field) are enormous. The pattern is called the "typesafe enum pattern".

Apart from non-functional code (see below) my greatest hate is for dependencies - I make minor change in one place, and errors crop up all over the place. The primary reason for OO is to eliminate or at least minimize this kind of thing. One way to achieve this is through the use of defined interfaces, as in the Java language itself.

3. Programming methods.

This is a more general category, but covers how one writes programs. An excellent source for this is the XP (extreme programming) web site www.xprogramming.com or in the associated book, "Extreme programming explained" by Kent Beck, published by Addison Wesley, or the other books in the same series.

Some of the aspects of XP may be subject to debate, but there is one area which every programmer should take to heart.

Don't write code "just in case". Don't write code "for now". Don't write code "because it will be needed later". Above all, don't write code that doesn't do anything, on the assumption that somebody will write the useful bits later.

The reason is that good code is constantly being refactored (see one of the best programming books ever written - "Refactoring" by Martin Fowler (of Gang of Four fame), published by Addison Wesley). By writing skeletal code you are saying "I know it all - this is how you should write your code. Of course I can't bothered doing it myself, that is for you lower orders". This does not sit well with the person who has to write the code.

On a more practical level, what usually happens is the the next coder leaves all the skeletal stuff in, but writes parallel code to do what is required. The other stuff can hang around for years because nobody is quite sure what it does or whether it does anything at all.

In summary, every line of code should do what is required, no more, but certainly no less.

In pursuit of this I have been systematically removing classes that do nothing. The code isn't lost - I have a zipped up copy of the source code for nearly every day for the last four months. In the extremely unlikely event that is is needed it can be recovered.

Cheers

[Mark_Everson]

Gary:

Thanks for taking the time to write this excellent summary of the things you think are wrong in the existing code! I'll try to re-read it when I get back to coding Econ so I can try and improve my coding skills, which are admittedly poor compared to a pro like yourself.

[Gary Thomas]

It wasn't meant to be a description of what is wrong with the code - it was meant to be an indication of how code can be written to make it easier for the next person taking over the project. My experience has been that almost no projects have tyhe same coder from start to finish, so a very important part of writing code is to make it as clear as possible for your possible (or probable) successor. The main area is documentation. In some ways bad code that is well documented is better than good code without documentation.

Cheers.

[F_Smith]

Just me again. Forgive me for posting and running, but it's almost 2 a.m. and I've put in 12 hrs today.

This is a good, solid list of coding style and usage suggestions. I agree down the line.

I'm sure I've broken all of them at one point or another! Sorry about that.

I'm especially guilty of not remebering to comment my code. The only defense I can offer is that my time was quite limited, as it was I barely found time to write the code I was able to churn out. In fact I've keep quite a list of excuses from a to z handy for just such an occasion, if you want to hear more rationalizations . . .

I especially agree with the OO concerns you raised

But what you say about 'MapSquare' puzzles me. In the last version of the code I have, we had a fully OO 'MapSquare'. It encapsulated Terrain objects, ethnic group objects and a 'mapSquareEconomy' object, among other things. Has that changed? Isn't there a 'Terrain' object in MapSquare to hold the terrain data?

[Gary Thomas]

It is just that I get uneasy if an object has more than around four fields. The original fields for MapSquare were:


private GameData data;

private String name;

private Vector terrain;
private Terrain primary_terrain;
private Terrain secondary_terrain;

private int terrain_code;

private boolean explored, city;

private boolean selected;

private Civilization controlling_civ;
private Province controlling_prov;

private Vector ethnic_groups;
private Vector task_forces;

private int x_loc;
private int y_loc;

private MapSquareEconomy mapSquareEconomy;

private PopupMenu menu;
private Dimension pref_size;


There are five fields here which relate to terrain. In addition the army data is only a vector, hence has no methods to manipulate the army data. This produces dependencies.

The present version is:


/**
* Terrain data for movement and display purposes.
*/
TerrainData terrainData;

/**
* The data relating to armies present in this square.
*/
ArmyData armyData = new ArmyData();

/**
* Economic data
*/
private MapSquareEconomy mapSquareEconomy;

/**
* Social data
*/
private Civilization controlling_civ;
private Province controlling_prov;
private Vector ethnic_groups;

/**
* x-dimension (horizontal coordinate of the square) in an isometric grid.
* Positive is left to right.
*/
private int x_loc;
/**
* y-dimension (vertical coordinate of the square) in an isometric grid.
* Positive is up to down.
*/
private int y_loc;


The social data is unchanged - its impact on D5 is pretty much nil.

The TerrainData class encapsulates all the movement code, as well as the images to be displayed, and provides aframework for the whole ecology model if it becomes relevant to the game.

The ArmyData class has a number of methods, including all the combat methods. Rather than a single vector, it has a map, keyed by civilization, each with a list of armies. Ultimately I expect that that list will be replaced by a TempTaskForce which provides a substantial amount of extra funcionality (the unit image to display and the total size of the force, for example).

The location data has two fields, replacing them with a single coordinate object hardly seemed worth it.

Cheers

[Gary Thomas]

I forgot to mention that I was pretty impressed by the huge framework that you generated. I assume that you worked horrendously long hours on it.

My commission was, essentially, get D5 working by mid June.

Pity I am not doing all that well at meeting the deadline...

Cheers

[LDiCesare]

Gary, quit being modest. You put so much code that I sometimes spent a week only unzipping your files, checking a few things, and receiving a new zip...
I agree with your coding standards. I tend to be too lazy (or not enough, since I maintain my code too) to do it all right all along.

[F_Smith]

Pity I am not doing all that well at meeting the deadline...

It sounds like you're doing a tremendous amount of work. This is just a project that a team of people could take a year or more on. Wish I still had time to help, but you know how that can be.

With 'Terrain' in MapSquare, what you're seeing is 3 different storage strategies for the 'Terrain' object. I didn't remove the old functionality because the issue wasn't settled yet.

The first was the 'int' -- Terrain Code. Version 4 of the game was entirely procedural. When I suggested that we make an OO version, I was roundly rejected. So I built 'The Beast', the govt editor. In building that, I didn't want to spend time building Terrain objects just for that demo, so I used a simple int.

Then, after winning the OO holy war, I built the OO architecture. At first I had a single 'Terrain' object, 'primary_terrain'.

But then massive payoffs became apparent from having multiple 'Terrain' objects per square. First I put a 'secondary_terrain', but it proved too limited. Then I made the Terrain objects a vector, but that proved to not gain the effects I wanted.

For what it's worth, I've since solved that issue in my mind -- a single 'Terrain' object per square, and the 'Terrain' object itself encapsulates other 'Terrain' objects as 'sub-terrain'. So a 'lake' can contain an 'island'. Or 'plains' can contain a small forest. That kind of thing. I personally do not like using a single object for all types of terrain in a square, or breaking 'Terrain', 'Ground Cover', etc, up. I like a distinct object for each, distinct type of 'mapsquare area' you might have to deal with.

Ya'll have specifically dropped the 'multiple terrains per square' concept I believe, so this may be completely useless to you. This is just f.y.i. Your 'TerrainData' is the same thing as my 'Terrain', unless I misunderstand. No problem there.

I would suggest that the 'movement' code doesn't belong in 'Terrain', however. I believe that 'movement' belongs in 'Transportation' objects. 'Horses', 'Boats', 'Planes', etc should contain the info about what Terrain they can travel and how they negotiate each type of terrain.

I would also suggest 'ArmyData' (the same thing as the 'TaskForces' objects in the old code, unless I miss my guess) is going to need to be stored as a collection of 'armies' -- one per 'GamePiece' in the square. Like the vector of 'TaskForce' objects I had. Similar to the vector of 'EthnicGroup' objects. I do not like the concept of all Armies in a square being a single object, personally. I would break that into an object per army.

The 'explored', 'city', 'selected', 'Popupmenu' and 'pref_size' all belong in the 'MapViewer' or 'MapPanel' -- the panel that displays the map, which I was still working on at the time. I hadn't migrated them yet, at that point.

But please, please don't take this as criticism, only suggestions. I think you're doing a wonderful job. Seriously, I'm very excited about what you're doing. And I can't wait to play Demo 5.

Good luck!

[Gary Thomas]

I was quite confused about the multiple terrains, and took the easy way out but leaving them there but not using them. I might have acted differently if I had known the history (documentation! but I do understand the constraints you were working under).

We decided that multiple terrain types (now called landforms) in a square wasn't a good idea - if we were going to do that we would have a lot of tile design problems, and the equivalent could be achieved by making the map squares smaller (which I have always wanted to do). Now we have LandForm classes, LandCover classes and Artifact classes. I expect that Resource classes may be added to this.

I am very glad that you won the OO war. I probably would not have agreed to do any coding if you hadn't. I learnt OO using Turbo Pascal 5.5 around ten years ago, then went through C++, finally to Java, which is very much my language of choice.

Mind you, OO design isn't always a simple matter, but done well it makes maintenance and extension hugely easier.

Cheers

[Mark_Everson]

Originally posted by F_Smith
Version 4 of the game was entirely procedural.

I dispute this statement! D4 was the best OO I knew how to write at the time... Admittedly not to very high standards, and I'm sure with many defects, but written with some OO design tenets in mind.

But what especially bothers me is that you, F_Smith, Know that statements like that pull my chain, and you persist in making them!