wood burning stoves 2.0*
The moose likes OO, Patterns, UML and Refactoring and the fly likes OO design question? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Engineering » OO, Patterns, UML and Refactoring
Bookmark "OO design question?" Watch "OO design question?" New topic
Author

OO design question?

Naseem Khan
Ranch Hand

Joined: Apr 25, 2005
Posts: 809
Hi ranchers,

I am designing an application for lawn tennis match using java to track the score of tennis match

The input to the application will just be which of the 2 players won a point. Based on this, the program needs to compute and display the current score.

e.g.,

Input( who won) Output ( sets won by player 1 � sets won by player 2

for the current set in progress games won by player 1 � games won by player 2

for the current game in progress points of player 1 � points of player 2)




I am planning to come up with very robust solution by applying different design principles like ocp etc.

Upto now, what I am able to think is making ScoredCard class as Singleton as there will be only one instance of ScoreCard throughout the match.

Planning to have a Player concrete class and since Player is responsible for updating the score here is two option for updating the same singleton ScoredCard

1. By defining updateScore() method in Player or
2. updateScore(Player p) in ScoredCard class

For a design to qualify as a good OO design, it should be defined in terms of abstraction rather than implementation. Where is the scope of abstraction here?

Any help or pointer would be appreciated

Thanks in advance

Naseem


Asking Smart Questions FAQ - How To Put Your Code In Code Tags
Stan James
(instanceof Sidekick)
Ranch Hand

Joined: Jan 29, 2003
Posts: 8791
Sometimes smallish problems like this wind up having very simple non-OO solutions. See Robert Martin's Bowling Game Kata for a program that started out with a nifty object design and wound up in one class. This may have some lessons for your program as it is a game with players and a sequence of inputs. Or maybe not.

If you still want to turn it into a really interesting object model just as an exercise, I'll have to think harder about your ideas.


A good question is never answered. It is not a bolt to be tightened into place but a seed to be planted and to bear more seed toward the hope of greening the landscape of the idea. John Ciardi
Naseem Khan
Ranch Hand

Joined: Apr 25, 2005
Posts: 809

Originally posted by Stan James
If you still want to turn it into a really interesting object model just as an exercise, I'll have to think harder about your ideas.



First of all, let me thank you, for prompt response.

Well, I do agree with you that this problem can be solved by using simple one or two classes with some non-OO design, but what I am more concerned is with the OO design rather than the output of the program.

I want to use the design principles e.g OCP (Open/Close) Principle, LSP (Liskov's Substitution Principle), Interface segregation principle, CRP (Composite Reuse Principle) etc, as applicable.

As these design principles talk about abstractness rather than concreteness and CRP talks about favouring composition over inheritance for achieving the polymorphism.


Thanks

Naseem
Ilja Preuss
author
Sheriff

Joined: Jul 11, 2001
Posts: 14112
Originally posted by Naseem Khan:

Upto now, what I am able to think is making ScoredCard class as Singleton as there will be only one instance of ScoreCard throughout the match.


That's not a good reason to make it a singleton. The singletoness is not a property of the ScoreCard, but a property of how it will currently be used in your system. Implementing it in the ScoreCard class itself will make your system unecessarily rigid and reduce reusability.

What if at some time you get the requirement that there have to be two or more matches at the same time?


The soul is dyed the color of its thoughts. Think only on those things that are in line with your principles and can bear the light of day. The content of your character is your choice. Day by day, what you do is who you become. Your integrity is your destiny - it is the light that guides your way. - Heraclitus
Naseem Khan
Ranch Hand

Joined: Apr 25, 2005
Posts: 809
Thanks Sir,

So how to approach this problem in OO manner?

Naseem
Stan James
(instanceof Sidekick)
Ranch Hand

Joined: Jan 29, 2003
Posts: 8791
I often start out with Communication diagrams (I still call em Object Interaction) just on pencil & paper or whiteboard. Focus on responsibility so you're asking an object to do things; avoid getting its data and doing the thing yourself.

You can start with "domain objects", things that exist in real life. But software is likely to add some implementation objects and drop some domain objects along the way. For example, Player might well disappear.

Sometimes when responsibilities are not yet clear you might introduce a controller or manager that orchestrates the whole scenario. Later you can often move the responsibilities to better places and make the controller go away.

I regularly recommend Scott Ambler's over view of The Diagrams of UML 2 for good examples. Bear in mind you're doing modeling as exploration and brainstorming, which is quite different from modeling for documentation or modeling for code generation. Be prepared to erase and change things a lot before they settle down and make sense. ANd be warned you can get stuck in this phase for ever. You can always nail down an idea one day, wake up the next morning and see a flaw or think of a whole new approach.

Another method is to start testing and coding and rely on Test Driven Design and emergent design. That technique may be a bit advanced at this point, but the Agile crowd makes it work for them. Ilja will likely jump in and describe it.
Frank Carver
Sheriff

Joined: Jan 07, 1999
Posts: 6920
It may seem too obvious to mention, but one thing you really need is a clear understanding of the problem you are trying to solve.

In this respect this challenge is very like the "bowling" one used in several other cases. You have provided us with one example sequence of numbers (which could and should be used directly as a unit test case), but what you have not provided is any attempt at a definition of the underlying rules and processes.

For those of us who don't follow tennis your example data is just a sequence of effectively unrelated numbers. I had exactly the same problem with the bowling example. I still don't really understand bowling scoring, so despite my many years of O-O design, I can't design a good bowling scoring system.

I could easily write a few lines of code which would produce the output you have shown, given the input you have shown. But I'm pretty sure my code would be "wrong" for all or most other inputs.

In my experience it is very worth the effort to take the time to write down the "rules", particularly in a nice, stable, challenge like this. You will probably find that when you try to write concise and unambiguous rules, some of the system objects will become quickly apparent.

Why not try it - write down your understanding of tennis scoring here, and we'll see if it makes enough sense to write code for.


Read about me at frankcarver.me ~ Raspberry Alpha Omega ~ Frank's Punchbarrel Blog
Naseem Khan
Ranch Hand

Joined: Apr 25, 2005
Posts: 809

Originally posted by Ilja Preuss

That's not a good reason to make it a singleton. The singletoness is not a property of the ScoreCard, but a property of how it will currently be used in your system. Implementing it in the ScoreCard class itself will make your system unecessarily rigid and reduce reusability.

What if at some time you get the requirement that there have to be two or more matches at the same time?



After some thought process to your post, I think ScoreCard class can still remain the good candidate for the singleton class. If for example, this singleton design has to cater the requirement of two different matches, I can anyways configue my singleton to return two instances instead of just one (may be through some configuration file. The configuration value can tell you how many instances is needed for the singleton, which basically signifies how many matches are being played currently). Singleton is basically a controlled creation of objects. There is a one to one mapping between ScoreCard instance and a match.

Comments please??

Thanks
Naseem
[ July 21, 2006: Message edited by: Naseem Khan ]
Frank Carver
Sheriff

Joined: Jan 07, 1999
Posts: 6920
Singleton is basically a controlled creation of objects.

Yes, but a very specific one, suited to very specific uses.

Can you explain why you don't think it is feasible to just create whetever ScoreCard instances you need and pass them around as necessary, without all the limitations and problems associated with a Singleton.

And as an aside, if "There is a one to one mapping between ScoreCard instance and a match", why not just put the scoring behaviour in a Match class? Do you need a ScoreCard at all?

I apologize if I am misunderstanding what you are trying to do here, but it seems that the concept of a "score card" is something imposed by the physical constraints of doing the process manually, rather than by the objects in the system.
Ilja Preuss
author
Sheriff

Joined: Jul 11, 2001
Posts: 14112
Originally posted by Naseem Khan:

After some thought process to your post, I think ScoreCard class can still remain the good candidate for the singleton class. If for example, this singleton design has to cater the requirement of two different matches, I can anyways configue my singleton to return two instances instead of just one (may be through some configuration file. The configuration value can tell you how many instances is needed for the singleton, which basically signifies how many matches are being played currently).


How would the Singleton know when to return which instance?
Naseem Khan
Ranch Hand

Joined: Apr 25, 2005
Posts: 809
Originally posted by Ilja Preuss
How would the Singleton know when to return which instance?


Well, in that scenario,ScoreCard singleton can maintain the Map containing the key value pairs. Key will be Match1,
Match2 etc, and values will be scoreCard1, scoreCard2 (instances of ScoreCard) respectively.

Now retrieving the correct scorecard instance will be governed by passing the correct match number to the ScoreCard
singleton class. Based upon the match you pass, corresponding match scorecard shall be returned.

From the on going discussion thread, what I felt is that disscussion is not as per to certain context. Having singleton
might be good idea somewhere and not having singleton elsewhere might be a good design too.

So I am going to put my design of the Tennis Case Study here and invite all the ranchers to comment on the same

So far for the Tennis Case Study with OO Design, following are the main classes which I have come
up with:


TennisDelegate [A business delegate class meant for masking the clients from the complex business details]
TennisMatch [A domain object which captures the business logic for the system]
ScoreCard [singleton class, for simulation of real scorecard in a tennis match]


The architecture (This is more of a sequence diagram) is as follows :



TennisMatch Class is a kind of business object (domain object). TennisMatch class contains a method
called getMatchScore(int whoWins), by passing the player who wins. This is the main method to update
the ScoreCard class.

All the business logic (tennis rules) for updating the ScoreCard goes here. It makes sence to add
all the busines logic here as TennisMatch is a business object. I am not going into the implementaion
details of the getMatchScore(player) as it is outside the scope of the current discussion.


Brief about TenisMatch class :
This class HAS-A relationship between following classes
(a) ScoreCard (one instance of ScoreCard in the TennisMatch class)
(b) Player (Two instances of Player in the TennisMatch class)

When TennisMatch is instantiated by the TennisDelegate, it initializes the TennisMatch object by
creating a scorecard instance and two player instances. This is analogous to the real world senario
that, when a match starts(instance initailzed in object modelling context), two players will be
on the court and there will be one score card for that particular match. So basically TennisMatch
class has one scorecard, and 2 player instances.


Although it makes sense to create a Player class as player is a real entity in the tennis match, but I cant make out the
usefullness of the player class apart from its name. Other attributes of the player class might include informations like
service consistency, speed, stamina, quickness, strength, or accuracy, but rightnow I dont need these details for the player,
as of now but as requirement evolve, these attribute might be the part of the player class. So is it really a good idea
to go with a player class with just name attribute, keeping in mind that other attributes might pinch in too later.

Brief about the ScoreCard class :



In my design, player instance is just used to update the individuls set/game/point values depending upon the input of
the player. As an example, if player 1 has been passed to the getMatchScore(player1), so it means I will have to update
the player1's details like setPlayer1, gamePlayer1 and pointPlayer1 (ofcourse by using the business logic/tennis rules).


Please comment on the over all design.

Let me thank you for showing the patience while going through the post

Thanks
Naseem
[ July 21, 2006: Message edited by: Naseem Khan ]
Stan James
(instanceof Sidekick)
Ranch Hand

Joined: Jan 29, 2003
Posts: 8791
I think your Singleton choice is premature yet. Let's start at the very highest level, just inputs and outputs. At this point we haven't even found a ScoreCard yet, so we can ignore that problem for a little while. I made up "Interface" as the user interface. The user probably does something on the UI to cause each Interface call.

Here we've described how the system as a whole behaves. Next we can drill into how ScoreKeeper works. If it's very complex we might want separate diagrams for a new match, a point, a point that decides a game, a point that decides a match, etc. Should it have a SoreKeeper have a Match object? Should Match have Sets and Games?

BTW: I just learned everything I know about tennis scoring on Google in 30 seconds. I'm sure you'll come up with improved scenarios to test and implement.
[ July 21, 2006: Message edited by: Stan James ]
Ilja Preuss
author
Sheriff

Joined: Jul 11, 2001
Posts: 14112
Naseem,

your design so far very much looks to me like you tried to "model the real world". This is a very common approach, but in my opinion one that leads to inferior software systems. The purpose of a software design is to make a system easy to develop and maintain, so the forces of the code are much more important.

Unfortunately, I don't feel I have the time to go with you through this - fortunately, Stan already gave you a quite good start.

You might also want to take a look at this article by Robert C. Martin, which discusses the topic on a different, quite interesting example: http://www.objectmentor.com/resources/articles/CoffeeMaker.pdf

The whole book this is an excerpt from, as well as his book "Agile Software Development - Principles, Patterns and Practices" are very good sources of information for everyone interested in OOD. There are also more of his articles on the object mentor website.
Ilja Preuss
author
Sheriff

Joined: Jul 11, 2001
Posts: 14112
Originally posted by Naseem Khan:
Well, in that scenario,ScoreCard singleton can maintain the Map containing the key value pairs. Key will be Match1,
Match2 etc, and values will be scoreCard1, scoreCard2 (instances of ScoreCard) respectively.

Now retrieving the correct scorecard instance will be governed by passing the correct match number to the ScoreCard
singleton class. Based upon the match you pass, corresponding match scorecard shall be returned.


I see. That's quite a complicated "Singleton", though, isn't it?

What would that give you that simply having each Match maintain a reference to its ScoreCard does not?
Frank Carver
Sheriff

Joined: Jan 07, 1999
Posts: 6920
As usual, I agree with the comments by Stan and Ilja, it looks like your application design is both too complex, and too limited.

Let's take an example - you have a class "Player". But all that seems to happen in your diagram is that a Player class is instantated. I have tried really hard to imagine what its behaviour might be but I can't think of anything. This is what I mean by too complicated. A class with no obvious behaviour is often sugch a sign.

The only use for a Player class I can think of is to hold associated details for a physical player, information such as name, age, sex etc. While this may seem a sensible option, it fails completely for a doubles match. This is what I mean by too limited.

Following the comments from Ilja : your design so far very much looks to me like you tried to "model the real world". This is a very common approach, but in my opinion one that leads to inferior software systems. The purpose of a software design is to make a system easy to develop and maintain, so the forces of the code are much more important.

My feeling from this situation is that there is no real need for Player, TennisDelegate or ScoreCard, at least while ScoreCard is 1:1 with Match.

Even in your examples, a player is just a number. Even if you want a slightly more friendly interface, a player could just be a text string "Andre Agassi", "Tim Henman", etc. I'm wary of introducing an abstract concept such as TennisDelegate without any obvious reason for it, especially as its sole job seems to be to return a ScoreCard. I can see that a ScoreCard might be a possible candidate for an object, if it was not 1:1 with a Match. While they are 1:1, though, I'd strongly recommend just dealing with a single abstraction, which (IMHO) should probably be Match.

Things I can see a use for which do not appear in your diagram are: some code to convert match wins (i.e. 3-1) to Tennis scoring (i.e. 40-15), and some code to determine victory conditions, which seem mildly complicated.

Also, your model as described doesn't seem to cater for the game/set/match progression, and whatever the victory conditions are for each of those. I have a gut feeling that I might need to introduce some new non-Tennis concept here (let's call it "contest") to represent the common features between a game, a set and a match (such as keeping, incrementing, and returning two scores).

Can you see where I am going with this?
Naseem Khan
Ranch Hand

Joined: Apr 25, 2005
Posts: 809

Originally posted by Frank Carver:
The only use for a Player class I can think of is to hold associated details for a physical player, information such as name, age, sex etc. While this may seem a sensible option, it fails completely for a doubles match. This is what I mean by too limited.



Thanks all for your valuable inputs. Just to make system more extendible, I have tried to take care of scenario of doubles as well apart from singles.

The approach is based upon Factory pattern which is as follows.

  • Abstract class Contestant defines some common methods which is applicable to both Player and Team e.g., hasWon() etc. There can be many more methods depending upon the scope of the problem.
  • Player class extends Contestant.
  • Similarly Team class extends Contestant.



  • Sample Business delegate code I am putting it here just to make things more clear.



    Here is the ContestantFactory class...



    One thing which I don't like about the Match class is that it is acting like a God class as most of the operations are performed by it like calculating and updating the scores. Is there anyway to break its responsibility into pieces?

    How about Contestant updating the score card rather than Match class updating the score card?

    Thanks once again

    Naseem

    [Code edited: by Naseem]
    [ July 23, 2006: Message edited by: Naseem Khan ]
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    Originally posted by Naseem Khan:
    Just to make system more extendible, I have tried to take care of scenario of doubles as well apart from singles.


    To really learn about extensibility, it probably would be more interesting to first fully implement the singles version, and *then* try to extend it.

    But is there really a difference in scoring between doubles and singles???


    The approach is based upon Factory pattern which is as follows.

    1. Abstract class Contestant defines some common methods which is applicable to both Player and Team e.g., hasWon() etc. There can be many more methods depending upon the scope of the problem.

    2. Player class extends Contestant.
    3. Similarly Team class extends Contestant.


    What is the *difference* between those two classes?

    And why did you decide to use a Factory? (I later noticed from your code that you didn't mean an Abstract Factory, but just what I'd call a Creation Method. No polymorphism involved, as far as I can see?)


    One thing which I don't like about the Match class is that it is acting like a God class as most of the operations are performed by it like calculating and updating the scores. Is there anyway to break its responsibility into pieces?


    For sure!

    I'm not very good in up front design, though - I wouldn't get it right, either. Instead, I would heavily rely on Test Driven Development/Refactoring to get a good code structure. The only thing I can say is that I'd doubt I'd get any Contestant class. I wouldn't be surprised if I'd get some States for the different states of the game that lead to different scoring rules, though.

    Would you be interested in going through this in TDD style together? Could take some time, though...
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    How about we begin by going right back to the first message in this thread. Let's start with some unit tests for the behaviour as described:



    This seems a reasonable starting test, so let's write the minimum code to make it work. Something like:



    It's not much, but it compiles and passes the initial test. And it is a perfect working solution for a rained-out Wimbledon

    For other situations, though, always returning no score will probably not be right. let's start adding some tests for some other, hopefully more useful, cases:

    Important to note at this point that I am deliberately using a self-commenting method name "playerOneWinsThePoint". This clearly defines the situation in which it should be called, and (because it has no parameters) requires no error checking or exception handling.



    Now it fails to compile again - there is no "playerOneWinsThePoint" method. The following should really be two steps (1) make the code compile by adding a dummy method (2) fix the compilable-but-failing code. For conciseness, I shall lump these two steps together:



    Cool. Both tests pass. However, a typical tennis match has two players, so we need to check that player two will get his/her points too:



    Same process as before - add a dummy method to make the code compile, then fix the test failure:



    This is now working fine for competitions with no balls played, and ones with one win. Obviously, real games get more complex. Let's provoke this behaviour by adding another test case:



    This time it compiles OK, but still fails. We need to do a bit more than just add some constant strings. This is where design begins to really take place. From here is where each developer might make a different solution, but it's vital to still keep in simple, and not overcomplicate things by anticipating future tests:



    This is beginning to look more like a tennis scorer. Before we get all excited and move on to more tests, I reckon it's time for a spot of refactoring. Personally I get itchy whenever I see duplicated data used by similar code. I reckon it can be refactored to remove the duplication. How about this:



    We can work on this a bit more if it seems a useful approach. If you want to carry on, start by thinking about what the next test might be, and how you could modify the existing code to accommodate it as simply as possible.

    For further reading (as well as the bowling game kata mentioned above) you could read some of my articles in the JavaRanch Journal, (such as here, here, and so on)
    [ July 24, 2006: Message edited by: Frank Carver ]
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    Frank, nice start! (And very much like what I'd have done...)

    Just one suggestion as your virtual pair programming partner - can we please rename the method? match.playerOneWins confused me until I understood that the player didn't win the match, but just scored a ball.

    Perhaps something along the lines of

    match.scoreAPointForPlayerOne

    ?
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    Sure. I've updated the above post. This just underlines my lack of tennis knowledge. I'm still not entirely sure of the difference between a game, a set, a match, and a point.

    You can take the virtual keyboard and conmtinue for a bit if you like.
    [ July 24, 2006: Message edited by: Frank Carver ]
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    This sounds like fun!

    I started a wiki page, where we can post the current state of the code, just to keep the overview: http://faq.javaranch.com/view?TennisMatchCode

    More to come...
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    I think I will add a test for player one scoring two points in succession. (I just looked it up at http://tennis.about.com/cs/beginners/a/beginnerscore.htm - it's realy called "points"! )

    Here is the test:



    The test fails, as expected.

    Let's make it pass as fast as possible:



    Let's see whether the test now passes - yes, it does! Success!
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    Now comes the refactoring step. There are a couple of things that I don't like about the code:

    - the use of Strings makes me nervous - it seems to make the code unnecessarily brittle. First I thought about using integers, because the score are numbers. But then it occured to me that in fact we just have a fixed, ordered set of allowed values (0, 15, 30, 40) - might be a good fit for an enum?

    - the duplication in the tests. Currently, we actually only want to test the score of a single game, but we always have to repeat the zero-zero scores for the match and the set.

    - as I think about it, we are actually testing two things at the same time - that the scoring algorithm comes up with the correct score, and that the score card prints it the way we (currently) want. I would feel more comfortable if we would decouple that. (I probably would concentrate on the scoring algorithm for now.)

    What do you think?
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    OK. I'll drive for a bit.

    I guess I'm not so worried about the use of Strings. I'm pretty confident that a better approach will appear when we actually need it. I'm particularly wary of leaping to soon to an abstraction which will probably fail when we get to the "advantage" and "deuce" stage. Also in the back of my mind is that the terminology seems kind of flexible. A score of 0 is also referred to as "love", for example, and we don't really want to embed one particular terminology too deep in the code.

    The latter two items, though, do deserve attention. I also feel the nudge to separate out the processing of point scoring in a game from set and match scoring. SO let's start by using "extract method" to separate out the scoring of the points in a game:



    The "prefix" bit still looks horrible, it has a generic name which should either be changed to be self-documenting or (shiver) gain a comment. Let's take the first option.

    Note that I do take a bit more than a "baby step" here. In a pure TDD approach, I should not really change two things at once, and here I am introducing two new methods in one refactoring. This is the kind of thing which can develop as you get more used to TDD - the important things are that the new methods have no new code (they are simply extracted wrappers around the existing constant strings), and they are covered by existing tests to make sure they keep working.



    The "score" array is now also looking a little too generic. Let's rename that to "game", so it's more obvious (for the moment, at least) that it is not used for set or match scoring:



    So the full code now looks like:



    That refactoring was all fine and dandy, and leaves the code in a much stronger-looking position for future changes, but this is "test-driven" development, and if we are not adding tests, we are not driving.

    Personally, I don't like the current implementation of the "point" method. However, I can't immediately think of an obvious refactoring to improve it. This is a typical design decision muddle - any reasonably experienced practitioner can see that it looks fragile, and the temptation is very strong to leap in and start coding a "better solution".

    The trouble with leaping to code a "better solution" is that we don't really know how to tell what a better solution is, yet. As an experienced developer, my head is already buzzing with possible improvements, but experience also tells me that most of the solutions I am thinking of would probably either be too complex, or simply wrong. We are starving for information about what is actually needed.

    We must resist the urge to code without a test, but as I can't resist changing that area, I will add more tests to help discover more about where the code should go.



    I was going great adding the sequence of scores, but then hit a "speed bump". At the end, it seems that the current interface for our class is not quite aligned with the real world. I need to assert something, otherwise we can never tell whether the code is working, but everything I can think of to put in here is at odds with how it feels it should be.

    I could put in an assertion that the score resets to "0-0". While this is probably how the code will behave, it's not really a clear expression of what we want the system to do. Instead, it's dangerously close to what I think of as "testing the accidents". The resetting of scores for the game does not have to occur at this stage (it could wait until the next point is scored, or wait for an explicit "reset" method, for example), so I'm wary of forcing this accidental behaviour with a test.

    I would really like to put in some sort of assertion that player one has won the game, but there is currently no class API to do this. I'm reluctant to introduce new client APIs (and force their implementation with a test) without involvement of a stakeholder such as XP's "on-site customer".

    In short, what seemed like a simple test of score counting has opened up a more significant API design decision. One benefit of a TDD approach is how quickly this issue has arisen. (Ilja and I have probably not been working on this for more than 30 minutes between us).

    For now, I'll just aim to get the test working with the last section commented out. Note that I have labelled my comment with "TODO". I usually use Eclipse, which finds such annotations and lists them for me, but even without such explicit tool support, it's still a good idea to develop a process for recording such outstanding issues, so that they don't get missed during later work.

    The first problem is that the "scoreGame" method is private. We haven't really talked about package issues, but in my tests I usually have my tests in a completely different package to the code. If that is the case, then we need to make the scoreGame method public. Some people find this a difficult choice to make. Personally I tend to have designs with plenty of public methods. YMMV.



    Now the code compiles, but fails. The quickest change to make it work is probably something like:



    Man, that's looking even worse now. "if"s, string comparisons, duplicated values. Ugh.

    Let's start by tackling the string stuff. It occurs to me that there is a disjoint between the idea of a "point" and the displayed score. let's separate the two concepts. Points will now be stored (and incremented) as integers, and we add a "display" method to convert them to text on demand:



    OK, "point" is much better - now it does what it intuitively seems it should. The "display" method is pretty ugly, though. And that "return null" at the end is asking for trouble. But I'm going to leave it there, until we get an answer from the customer about what happens after 40. And there's still that "deuce" and "advantage" stuff looming on the horizon.

    Final code now looks like:



    Now I'll lean back and let someone else have the driving seat. Any takers?
    [ July 25, 2006: Message edited by: Frank Carver ]
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    Mhh that's already a little bit better.

    But I'm really a big fan of separating the model from the view. So I'd really like TennisMatch to not know about the string representation of the scorecard at all. I think it would be a good idea to introduce a scorecard which has the responsibility to create the representation for the score of a TennisMatch. We can than unit test the ScoreCard and the TennisMatch independently of each other, which will also simplify the tests.

    I think I will start by renaming the current TennisTest to TennisFunctionalTest (it's still a good idea to have some tests that exercise the collaboration of the two classes). Fortunately Eclipse does this at a press of a button.

    Now, in our setup of the tests, we need to create instances of both classes and wire them together:



    Neither the scoreCard field, nor the class exist yet, so I let Eclipse them for me.



    The tests run again - we aren't using the score card yet!

    Let's rewrite the first test:



    That doesn't compile yet. One quick fix later:



    Now the test fails, as expected. We can make it pass by simply delegating to the match:



    Of course that's not what we actually want, but we first must adapt all the other tests:



    This was trivial, the last test is a little bit more work:



    Again the first implementation of ScoreCard will simply delegate to the match:



    Now our functional tests are fully migrated to the new API.
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    The whole point of the previous change was to get rid of the representation logic in the TennisMatch, so let's do it.

    Until know, only scoring a single game works, anyway, so the fullCard method can be rewritten to



    Tests run.

    Now we can already get rid of scoreCard, scoreMatch, and scoreSet in TennisMatch.

    Tests still run. I like it!

    Now we want to move the scoreGame methods logic to the ScoreCard class. Currently it looks like this:



    Mhh. ScoreCard will need access to the game score of both players. Let's extract getters for that:



    Tests still run.

    The display method actually belongs into the ScoreCard class - it's presentation logic. The easiest way to move it is to first make it static:



    Luckily enough, this works. Now we can use Eclipse's "move method" refactoring to move it to the ScoreCard class:



    I love automated refactorings! Tests still run, of course.

    Now we can simply inline the scoreGame method - the only place where it is still called is in ScoreCard, remember?





    Tests still run. I like the result so far. Do you?
    Don Kiddick
    Ranch Hand

    Joined: Dec 12, 2002
    Posts: 580
    I'm not confident enough to drive right now but I'd like to say I'm enjoying this game of 'refactoring tennis'!
    D.
    [ July 27, 2006: Message edited by: Don Kiddick ]
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    Very interesting. It's not the direction I would have gone, but that's the thing about pair programming - the delight is in the surprises.

    If we were in the same room (or even on some sort of net-collaboration tool) I get the feeling there would have been some hand-waving and whiteboard work at this point.

    In the spirit of the game, I'll proceed based on the code you have "served" to me.
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    The first thing I want to do is to refactor the test cases a little. I like the idea of separating out "functional" tests, but one of the tests in the new TennisFunctionalTests class sticks out as not really being a functional test. So let's create a new test case for it:



    I haven't moved the niggling test yet. The reason is that as soon as we have more than one test case there is a chance that some of the tests might (accidentally or through laziness) not get run. If I were to move the problem test from the functional tests, then just run the functional tests I have actually decreased the test coverage. I could cmopletely screw up the new test case and the code it uses, and never know. Yikes.

    So before we progress I shall introduce a test suite class as a single point of execution, to make sure that our "test button" actually does run all the tests.



    There are several ways of doing this, this is just the habitual way I do it. As long as the end result is a single "button" which will run all the tests we are fine.

    Running this new test we should get a pass from the functional tests, and a complaint about no tests from the match tests. So let's fix that by moving the point progression test to the match tests.



    Oh. It doesn't compile. All the setup stuff is still in the Functional tests.

    Here's another sneaky temptation. It seems so easy to just copy and paste the fields and setup from the functional tests. It would make the tests run and give us that lovely endorphin rush from a green bar. But is it the right thing to do?

    Remember that this new test case is a lower-level unit test. Porting over the whole functional test setup leaves our test as half functional, half unit. And this is where some of the confusion came in which led to the comment and the TODO.

    So let's make this just a test of the bahavior of the TennisMatch class, and remove all references to the score card. We can write score card unit tests later, when we want to develop it a bit more.



    Better. It now compiles. And it is simpler, although it's not really testing the same thing that it was before. At this point, I'd add a note to the project "to-do" list to add a specific test for the number-to-string conversion in ScoreCard, but I won't divert into implementing it, because I still have a failing test to fix.

    That's right. It looks lovely, but it fails. It fails with a null pointer because the match object is never initialised. So we add a setUp method to this test.



    Now it compiles and passes. Phew! Time for a short break, more in a while.
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    OK. There are now two virtual "to-dos" in this project. The one which is irritating me the most is what to do when one player wins a game. That comment in the test bugs me every time I see it.

    Interestingly, though. Ilja's introduction of the ScoreCard class (and the separation of model and view in general) may have offered a solution to this.

    How about if the match object makes a "callback" to tell the view that a player has won a game? That way we don't need to add any of the clumsy enquiry methods to the TennisMatch which were holding us up. Especially as that customer still has not come back with a preference.

    We want to phrase this as a test, in order to drive the devlopment, but it's initially quite tricky to think how to test this behaviour. Just last volley I made sure there was no mention of ScoreCard in the tennisMatch test, and even if we did have an instance, there's still the problem of what API method would we call. After all, what we want to do is just internal behaviour between two objects, with no obvious client API.

    This is where one of the most powerful unit testing tricks I know comes into play. Mock Objects. I find that mock objects of some sort tend to creep into most of my unit tests. There are several mock object "frameworks" around, but in this case we can do something simple and specific to our tests. If we need more complexity later we can always add to it when we need to.

    We drive the implementation of our mock object by writing a test, as usual. In this case, replacing that ugly comment and TODO with something off the top of my head.



    Note that this does not have to be as wordy or error-proof as a client API. This mock object and its enquiry methods will only be used in testing. And, of course, they can always be changed later. The important thing is to get to a passing test quickly without atopping for too much head-scratching.

    To make this compile, we need a "view" object. But what class should it be?

    If our TennisMatch object is going to make a callback, it needs something to call. We know already reckon there is likely to be a mock object to call in unit testing, and a real object in functional testing and live deployment. To keep these separate, I'll introduce an interface, which these two classes can implement:



    For our unit tests, let's make a simple mock version



    As it stands, it implements the interface, but does nothing. That's OK for now. Remember, though, that the point of creating this class was to make our tests compile, so it also needs an enquiry method as used in our test:



    Add a line



    to TennisMatchTest and it compiles. Whoopee!

    But it fails again. Bleh!

    Just as before, the problem is a null pointer indicating that the view object has not been initialised. So we add it to setUp:



    Better, it no longer gets runtime errors, but it still fails because the winner value is never changed from its default of 0. Ah yes. We need to actually implement that callback stuff.

    Let's start by changing the mock view to actually take note of when it is called:



    Now lets update the match to call the view when a player passes 3 points.



    This looks good, but it won't compile because there is no variable "view" in TennisMatch. So let's add one, passed in to the constructor:



    We are getting there, but now neither of our test cases compile, because we have changed the constructor signature of TennisMatch. Fixing it in the TennisMatch unit test is easy:



    Fixing it in the functional test is a bit more tricky. Before I do that I want to at least make sure that the unit test runs. So I'll ignore AllTests an just run the match test.

    Huh? It fails. The winning player is still 0. That's unexpected. All the code looks simple and I can't see immediately what's wrong. So I stick a quick diagnostic in the mock view:



    Note that "outdenting" the diagnostic is another habit of mine. I like such extraneous code to be really visible, so that I can find it and remove it later. It's just a shame that Eclipse doesn't like this approach and tries to indent them avery time

    Anyway. Running ths test prints "win 0". So the methods are being called correctly, but with an incorrect value. Have you worked out why yet?

    It's because our design has muddled two concepts, the symbolic player id (e.g. PLAYER_ONE) and the array index to the array of scores. So the callback method is called with PLAYER_ONE, whch happens to be the array index 0. D'oh.

    A quick way to fix this is to create a conversion method to adapt the array index to a real player number:



    That "+1" is a bit rough, but it will do for now. At least our unit tests pass.

    Now to address the functional tests.

    This is tricky, because we have a circular dependency. we need to pass the ScoreCard object into the TennisMatch constructor, but we also need to pass the TennisMatch object into the ScoreCard constructor.

    There are several ways to approach this. We could change from using constructor parameters to "setters", but this risks running with incomplete objects. We could introduce another layer of object which manages the creation. We could re-merge the two classes and construct them all at once, and so on.

    This could easily use up hours of discussion (aor argument) to decide. But at the moment there is a bigger pressure. We have a test that won't even compile!. This has to be our biggest priority. Once we have a passing test, then we can think about the architectural issues, and refactor the code to get there.

    So I'll "cheat". I'll pass in a null view, and defer the decision about how to deal with the match and score card concepts until we have a test to drive it:



    Look at that. All the tests pass. That null view is a bit smelly, but there are no tests to drive any behaviour into the ScoreCard class (or anywhere else) yet.

    Before I give up for this volley, I reckon we also need a unit test for player two progression, and I've also added some sanity checks to the player one progression test to make sure that nobody wins too soon:



    That leaves me feeling more comfortable. Time to let someone else have a go, I think.

    Remember that the current state of the code can be found at http://faq.javaranch.com/view?TennisMatchCode
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    Now I'm the one who is surprised!

    Frankly, I have no idea where we are going with this Observer idea. Anyway, I suspect we are missing a (functional) acceptance test. Let me try to propose one:



    Does that look right?

    How do we implement it?

    (As an aside, I would have cared more about the deuce-advantage stuff, because that looks more interesting to me. As you said, the surprises of pair programming... )
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    OK. I'm really getting into this challenge, now. (you can probably tell )

    That test fails, of course.

    In the spirit of going for the quick green bar, I'll start by adding a hard-coded version of the listener stuff to the score card:



    For this to stand any chance of compiling we need to add a setScore field, and it's probably wise to implement the interface, too.



    OK. it compiles again, but still fails. Now we need to link it to the match.

    A simple approach seems to be to use a "setter" in the match, and only call the callback method if a MatchView listener has been specified.



    A quick test run reassures me that it compiles, but still fails. It's also getting a bit grubby, and I can feel the pressure to refactor, but I will hold off until the test passes.

    All that should be needed now is to link the score card and match in the functional tests.



    Run again. Hmm. it still fails. Oh yes, I forgot to update the score card code to use the new variable when generating output.



    Run again. Yikes. Now all the functional tests fail. A quick check of the test output shows me the problem: "expected '0-0 0-0 0-0', was '0-0 null 0-0'". Oops. forgot to initialise the variable.



    That's better, the old tests are working again, but the new one fails. Instead of '0-0 1-0 0-0' it is returning '0-0 1-0 40-0'. I remember thinking about what we should do with the game score when a player wins but being uable to decide what to do. Now we have a test to drive it. The match class needs to reset the game scores:



    Run again. Good, all the tests pass. Now (finally) I can get to some of those refactorings.

    As we are looking at that "point" method, let's tackle that one first. Those two lines we just added are crying out for an extract method to give them a sensible name:



    That's better. The line where the match notfies the scorecard still looks ugly and overcomplicated. Let's extract that too.



    And finally for here we'll open out that line to make it more readable.



    A quick re-test. Good, everything still works.

    Now, while I'm in a refactoring mood, some other, broader things are beginning to tug at me.

    It really seems as if the responsibilities of the current TennisMatch class are changing. With each code change, it seems more and more like a class about a "game" rather than a whole match. Match text output has moved to ScoreCard, and even the progression of points in a set seem headed elsewhere.

    So I think it's time for some of those sweeping changes that refactoring editors do so well, but make pre-refactoring tools struggle.

    First, I'll rename TennisMatch to TennisGame

    Then I'll rename MatchView to GameView, and change its method to gameWin. For completeness and to avoid confusion, I'll also rename MockMatchView to MockGameView and TennisMatchTest to TennisGameTest.

    In such changes it's usually useful to do a quick visual check of the code to see if any of the other names now stand out as odd, when I do this i decide to rename the "match" member variables of TennisFunctionalTest and ScoreCard to "game".

    Also it now seems that some of the names are now too long, so I'll shorten TennisGame.playerOneGameScore and TennisGame.playerTwoGameScore to ennisGame.playerOneScore and TennisGame.playerTwoScore.

    Refactorings where I can reduce or remove things always make me happy.

    All of that may seem like a lot of work just to implement one test, but it's important to keep remembering that in TDD, the design is done as the testing and coding progresses. There was no "design phase" before we started, and there will be no "test/debug/fix/retest" phase when we are done. These refactorings are where the design happens. A few minutes of reorganising working code now and then is a small price to pay.

    This post is getting pretty long, so I'll pause here for a while. Next post I'll add a test to drive the development of set scoring for player 2.
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    That looks good! I already feel some forces of what I want to do next, but I'll wait until you are finished (I'm currently at work, anyway )...
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    OK. To fully develop this notification of the winner, it needs to work for player 2 as well as player 1. So we start by mirroring the last test:



    Naturally it fails. It still shows that player 1 won the game.

    This one seems a pretty simple change in ScoreCard:



    Good, it works. But it's horrible. It's got duplicfated code and a fragile "if" as well as those hard-coded strings. Let's separate out the string generation from the score keeping:



    Better, but there's still more to do. That "if" has got to go:



    Finally, I want to change the setScore member variable to a method:



    Run again. Tests still work.

    However, there are still a few problems here. The first is an interesting issue which crops up from time to time. Domain terminology (in this case "set") is conflicting with common programming usage. When we named "gameScore" it seemed a perfectly reasonable name. When we added a similar method for a set, it seemed reasonable to use the same convention. But "setScore" reads as if it is a "setter" for an attribute "score".

    To prevent confusion, let's spin the names round so they become scoreSet() and scoreGame()



    That's nicer. The bigger problem, though is the massive duplication between the new set scoring code and the game scoring code in TennisGame. I feel a TennisSet class coming on.



    Obviously we need a few chanegs to ScoreCare to move the set scorring stuff out to the new class:



    And a change to the functional tests to create and pass in the set object:



    Run the tests again. All still pass. Good.

    Now we have moved all that duplicated code into a new class. This may seem like a waste of effort, but it makes the next step much easier. Extract the common bits of TennisGame and TennisSet into a shared base class which I shall call TennisContest)

    Just before we do that, though, we need to align the two classes a little more. Currently TennisGame has an array called "game" for its scores, while TennisSet has an array called "set". Let's rename both of these to just "score".



    That leaves our new TennisSet class satisfyingly small:



    and trims down the TennisGame class nicely, too:



    I definately get the feeling that some more of the behaviour of TennisGame (such as notifying a listener of a win) will end up in TennisContest, but I won't move them until we have a test to drive the change.

    There were some pretty major design changes as a result of these tests, so I think it's time for me to get on with other stuff and let some one else have a go.
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    Phew, updating the code in my IDE begins to become tedious work. It also seems as if the AllTests suite wasn't up to date in the wiki (fixed it).

    Should we try to get the code into some version control system to make it easier to share it?
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    The thought was occurring to me too. Do you know of a public one we can set up easily?
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    I have a project at source forge for which I'm currently not using the svn server.

    Only disadvantage would be that to get write access, you have to get an account at SF.
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    That's no problem for me, at least. I'm "efficacy" at sourceforge, and I have several projects in verious states there. Feel free to add me as a developer to your project.
    Frank Carver
    Sheriff

    Joined: Jan 07, 1999
    Posts: 6920
    OK. I have signed up. What I'll do (if you have no objections) is work through the history of changes to the files (I have kept a source "snapshot" each time we handed over) checking them in, so that the history of the discussion is reflected in the svn history.

    I don't know how easy svn makes this, but in principle it ought to be possible to link to a particular version of the source code in each message in the thread.
    Ilja Preuss
    author
    Sheriff

    Joined: Jul 11, 2001
    Posts: 14112
    This did take longer than expected, but finally I managed to commit the current source to the SVN repository. The URL is https://svn.sourceforge.net/svnroot/jfcfixtures/JavaRanch_TennisMatch/trunk - Frank, I gave you write access. I also included the Eclipse project files, so if you check it out into an Eclipse workspace, you are ready to go.

    The up to date sources can also be browsed at http://svn.sourceforge.net/viewvc/jfcfixtures/JavaRanch_TennisMatch/trunk/

    Do we still need the wiki page?
     
    I agree. Here's the link: http://aspose.com/file-tools
     
    subject: OO design question?