3

I'm doing a simple project to manage data for a tabletop game, but I'm mostly using it to get experience about correct coding.

I've just reached a point where I have five classes that are tightly coupled, and I'm not sure whether leaving the whole thing as it is, or refactoring.

What I have is basically this:

  • class ShipTemplate: This class ( that has nothing to do with c++ templates ) has all constant members and contains basic informations about a category of Ships.
  • class TemplateSet: This class contains all ShipTemplates that are currently available to build, and it has a name. It should be stand-alone, since it represents the available technology of each player at any time, so one would be able to save/load different sets at different times.
  • class Ship: This class represents a complete ship, with loadouts, name and other things. It contains a const reference to a ShipTemplate, which the class is not allowed to change, to refer to its basic functionality. It could extend ShipTemplate, but I wanted to keep track of which Ships had a particular underlying ShipTemplate, and it seemed easier doing it like this.
  • class Fleet: This class contains a list of Ships, it has a name and contains other information. It should contain a cost variable equal to the sum of the cost of all Ships in it.
  • class Deployment: This class contains pointers to all the Ships, Fleets, and TemplateSets available to the player. It also needs to keep track of ShipTemplates that are no longer available, but that are still used by already built Ships. It should contain a cost variable equal to the sum of the cost of all Ships available to the Player. It has to manage the transfer of Ships from one Fleet to another. It has to find out which Ships are within a given Fleet, or which Ships have a given ShipTemplate.

Unfortunately every class is pretty interwined with all the others. I thought about different approaches, but I'm not sure if even one of them is the correct one.

  • Use friend statements all over the place, so that if one class modifies something, it can correctly update all the others.
  • Use very long names, like Deployment::modifyShipThrustInFleet, and allow any modification solely through the Deployment class, which will take care of everything.
  • Remove TemplateSets and Fleets and represent them within Deployment, so that it can update correctly cost values/pointers without breaking any "correctness" rule. This too implies that every modification to the system has to pass through Deployment.
  • Insert into lower classes pointers to upper classes, so for example when changing something in a Ship it can automatically update costs of both its Deployment and Fleet.

Is there some other solution I didn't see, or maybe a refactor into more classes that can help me achieve readable, mantainable code?

Svalorzen
  • 5,353
  • 3
  • 30
  • 54
  • 1
    Right off the bat, I'd say forget friend statements. Anything that breaks encapsulation should be avoided. Have you diagrammed your class relationships? Sometimes, doing that alone can give you insight into the structure and makes clear certain refactorings... Also, I wouldn't describe your classes in terms of what they contain, but in terms of what they DO. This is important because thinking of what they contain may encourage greater coupling. – RonaldBarzell Dec 03 '12 at 19:39
  • 1
    @user1161318: friend does not break encapsulation, since you cannot be "friend" of someone if that someone doesn't what it. Before "dropping a language feature", at least complete the analysis. – Emilio Garavaglia Dec 03 '12 at 19:46
  • Anything that allows an outside class to peer in on the inner workings of another class breaks encapsulation. Just because it's given a term and status as a language feature does not change what it is. There's plenty of criticism of friend features out there if you don't believe me. At the very least, the friend feature should be the last thing the OP tries, after exhausting every other solution. – RonaldBarzell Dec 03 '12 at 19:48
  • @user1161318: Please, before posting any talibanism, read this: http://stackoverflow.com/questions/1093618/how-does-the-friend-keyword-class-function-break-encapsulation-in-c The key point is that "friends" are together forming a wider "capsule" – Emilio Garavaglia Dec 03 '12 at 19:54
  • @EmilioGaravaglia, I said others stated this in case you were swayed by appeal to authority. However, this is my PERSONAL opinion and one I follow. I also think this is implied by what OO is supposed to be. This is not the place for a "what is OO" war, so why don't we just agree to disagree and let the OP decide for himself? – RonaldBarzell Dec 03 '12 at 19:59
  • @user1161318 I was imagining that each class should have the minimum amount of methods to be able to manage itself and its own collections. If this wasn't true the number of methods required by any one class would increase exponentially the more layers were below it. Unfortunately this, at least for me, doesn't solve the problem of transferring information up in the chain, and that is basically where I'm stuck. – Svalorzen Dec 03 '12 at 20:02
  • @user1161318: It would be interesting to understand what "authority" is to you and actually means. Nothing bad in having personal opinion, but using words like "ANYTHING", "There's plenty of criticism" (deliberately ignoring there's also plenty of appreciations) is not a fair behavior respect to the OP. And in any case OOP is not C++ and C++ is not Java an Java is not OOP. Each of them has a different idioms to refer to the concept of "object" (that's not the same as "C++ class"). Don't confuse techniques with tools – Emilio Garavaglia Dec 03 '12 at 20:07
  • 1
    Feel free to invite me to a chat if you would like to go over this; however, I don't think filling up the comments of the OP is an appropriate place to do this. We are really off-topic and I think are doing a disservice to the OP if we continue this publicly. – RonaldBarzell Dec 03 '12 at 20:20
  • 1
    @Svalorzen: ok, so one of your concerns is with a method explosion as well? It helps to know the specifics of what you may dislike about your current architecture. – RonaldBarzell Dec 03 '12 at 20:22
  • @user1161318 Yes, basically all four options I mentioned in my question I'm not comfortable with. The method explosion one is basically the "really long names" options ( didn't know it was called method explosion! ). As long as the data is accessible, classes are understandable and the code is clean I don't really care how the design comes about. What I am really afraid is that to modify some lower class variable the code will be so convoluted that it'll be very hard to trace what is happening. – Svalorzen Dec 03 '12 at 20:32
  • Actually, it's not called method explosion; method explosion (or class explosion, etc...) is when you end up with a very large number of methods or classes, etc.... I thought you were dealing with a genuine method explosion, and not just concerned about the names. So with that said, have you tried any of the responses below? Any feedback on where you stand after trying them? – RonaldBarzell Dec 03 '12 at 20:35

2 Answers2

2

Just some thoughts.

When thinking in object oriented way, thinking about real-life objects lays out the view, what you should describe in program. And what is object in real-life? It is a "thing" which has certain features and exposes some functionality.

For example ShipTemplate is a blue-print of the ship. It should define size, layout, part types and quantities (DieselEngine, SteamEngine, AntiAircraftGun, UnderwaterMines, etc), and how they are connected with each other.

Ship on the other had is constructed according to blueprint - it should have all part instances. For example it might have two DieselEngines and three AntiAircraftGuns. And it is correct, that ship does not inherit from blueprint. Blueprint is only a description of ship, not it's parent.

Now, each type of the object (blueprint, part, ship) has it's own properties and functionality. For example, each engine consumes some amount of fuel and can increase speed of the ship to some value. Why not have base class for Engine, which has these features? (inheritance). The same goes for the guns (lets call it ShipWeapon). There is of course a big difference between mine-gun and anti-aircraft gun, but they are both guns, they are both mountable on the ship, they both have weight, ammo type, reload time, ammo capacity, whether gun is operating.

So these are some properties of the objects. What about functionality? Other important concept of OO design is that each object has (encapsulated) some functions which can be done with it (and it may or may not alter objects state). For example ShipWeapon should have method Fire(), which maybe should decrees amount of ammo in it. Or try to target at first with Aim(sometarget). Engine on the other hand would have Start(), Stop(), SetSpeed(speed). Note that these would work internally on the object and it's state. Ship might have SetCourse(direction, speed), which would start it's engines at required power and orient its rudder. Also ship might have Colide(ship). And Hit(typeofattackinggun), which would iterate through all parts of ship and damage some randomly (and set IsOperating for a gun, or turn off one of the engines, etc.)

As you can see you can go into a lot of detail while designing OO approach. Its also very good to know when to stop - just how much detail (or accuracy) you really need for you program to work.

Also, there could be a global World, which would hold all ships. And so on..

There is other part of program, the infrastructure. How you data objects (ships, worlds, players) are managed, how they know and interact with each other. For example each ship as an object can be observed by global map and each ship would notify it about movement (observer pattern). Or global world would query state of each ship at some time intervals, based on global clock. Or...

I guess what I was trying to say is to stick to main OO principles - encapsulation, inheritance, polymorphism. And there is a lot of literature out there for object-oriented design, design patterns, etc., which is useful. Wiki entry is a bit "academic" but has main definitions, which make you think :) Also look at SOLID

PS. And it is usually a sign of a bad design to do everything in a single class.

Algirdas
  • 677
  • 1
  • 6
  • 15
  • I'm trying to stick as much as possible to OO principles, that's basically the whole point of this project. However, the part I'm usually troubled with is exactly with the infrastructure. Usually is easy coming up with basic method, but as soon as relationships become complicated I don't know where to put my hands. In this case my problem relies mostly in the updating of costs related to some Ship change. – Svalorzen Dec 03 '12 at 20:45
  • If you have some view of the ship, which displays cost, it might be the observer of ships cost, which is a sum of all ships parts costs. Cost is a property of ship part and adding/removing part from the ship would trigger notification (through event or observer) to single (or multiple) view. [Observer pattern](http://en.wikipedia.org/wiki/Observer_pattern). Selecting a ship and opening some window (view) should start observing the ship's cost (attach to event or as observer), and any updates to the ship cost would reflect in the view. – Algirdas Dec 03 '12 at 21:02
  • I thought about this. Do you think such a solution is warranted for a small problem like this? It does sound overkill in some way, but maybe I'm just imagining it. – Svalorzen Dec 03 '12 at 21:04
  • Based on your requirements it seems straightforward. But yes, sometimes it is overkill and over-engineering. If this is a project to learn OO, why not try this? And see for yourself - how it is implemented, does it solve the problem or does it creates a dozen more? – Algirdas Dec 03 '12 at 21:09
  • You can have a simple event Ship.CostChanged. Or a IShipObserver with CostChanged(Ship ship), PartAdded/PartRemove (to redraw ship). Or a specific IShipCostObserver with CostChanged, CurrencyChanged, etc. (Point of this is, that ship has parts and it 'has' cost. View should be just a view). Infrastructure is always about what you need at what granularity. Investing in it at first may save time later (using, refactoring, etc). And some times it is more a philosophical question :) – Algirdas Dec 03 '12 at 21:29
  • Thanks! Now I have a couple of ideas on how to continue =) – Svalorzen Dec 03 '12 at 22:08
1

Now that you have described haw you want to represent the various data, before defining the complete relations, try to complete the description by defining the "protocols":

What can each class be able to do to the others? WHat methods and rules between methods are needed to achieve your goal?

Once you have defined how classes act on each other you will most likely discover what is candidate to be private, what is public and what level of friendship must exist between the parties.

May be is not your case, but -usually- when complex relations exist, one possible pattern can be the use of a "communication bus class", that expose the action that can be "sent" to the various object, each having a private interface and being friend of ... the bus itself (an only the bus).

EDIT

Following Svalorzen comment:

It depends on the side you are watching it. This will, in fact, introduce multiple level of "privacy", allowing to implement encapsulation on a wider unit that the class itself. Whether this is good or bad is a matter of context, not idiom.

Instead of having just classes with everything private (for no-one else) or public (for anyone), you have a "capsule" that is a "club" (the "club of the classes having 'bus' as a friend") and a "club manager" the is the real "filter towards the public" (and hence the real OOP object), allowing certain methods that need to interact with more classes private parts at a same time, to do that inside the club only.

The deny of "friendship" is nothing more than a misconception that confuse techniques with tools, making OOP objects the same as C++ classes. That's -generally speaking- a FASLE IDIOM. C++ classes can be smaller units than OOP objects (think to the pimpl idiom: what is the "object" there?). The fact that a class can be a friend of another doesn't make it a friend of anyone, hence the private parts are not made public. You are just defining another level of privacy where encapsulation apply the same as with "private". It just apply on a wider group. That "wider group" plays, respect to OOP, the same role a non-friend class plays.

The misconception that "friend breaks encapsulation" has nothing to do with the concept of OOP. It has to do with the way OOP has been implemented in Java, that is a completely different language respect to C++. In C++ friendsip is just a construct to "group thimgs together" just like class, struct, templates, inheritance, membership etc.

What OOP relation (composition, inheritance, linking...) has to be mapped to what C++ construct, unlike in java, when the language philosophy is defined to be one-way only, is not defined by the language itself and by it's standard library.

The mapping "OOP object = C++ class" is just a common cultural misconception inherited from the past, when C++ has no templates, no lambdas, no friendship, nothing more than classes (and was in fact called "C with classes") when the only way to implement an OOP hierarchy was through classes,since that was the only way to create a hierarchy relation using that time c++ constructs.

Nowadays I can even implement an OOP system using C++ members and implicit conversion for "OOP inheritance" and C++ private inheritance for "OOP membership". Or I can implement an OOP object with a "cluster of classes (or mat be labdas)", defining its run-time behavior (think to std::locale and related facets).

Starting a design with the OOP object == C++ classes idioms is in fact cutting away two degrees of freedom C++ adds to program design, restricting your mind to what C++ was more than ten years ago.

Emilio Garavaglia
  • 20,229
  • 2
  • 46
  • 63
  • How would the bus class be implemented? I mean, if every class had to send its own actions to this class, it would have to have access to everything. Wouldn't this be as bad as simply cutting away layers and grouping stuff in big clumps? – Svalorzen Dec 03 '12 at 20:44