• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Liutauras Vilda
  • Junilu Lacar
  • Jeanne Boyarsky
  • Bear Bibeault
Sheriffs:
  • Knute Snortum
  • Tim Cooke
  • Devaka Cooray
Saloon Keepers:
  • Ron McLeod
  • Stephan van Hulst
  • Tim Moores
  • Tim Holloway
  • Carey Brown
Bartenders:
  • Piet Souris
  • Frits Walraven
  • Ganesh Patekar

OO design question?

 
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Originally posted by Frank Carver:
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.



Oops, we have a race condition here...
 
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Never mind. We can sort that out later. I've now got an eclipse project hooked up to the source in svn, so I'm ready to move on.

Meanwhile I'm still lookimng forward to your next move - will it be a refactoring? a new test? a change of emphasis? all of the above?
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
We will hopefully found out this evening!
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sorry, didn't find the time yesterday - had to watch "Pirates of the Caribbean 2" with my best friend...

One minor thing I'm thinking about is restructuring the packages.

What would you think about


[ August 01, 2006: Message edited by: Ilja Preuss ]
 
Frank Carver
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I've seen some projects with this sort of structure, but I've never been sure of the benefit. My ow projects tend to sit with whatever seems an appropriate domain structure for the delivered code, but just a single package for tests. Well, OK. I do sometimes extract out a separate package for mock implementations, as they can often be copied/reused for other projects. But I've never really seen the advantage of mirroring the domain structure in the tests

e.g.


Can you explain a bit about your thinking behind your suggestion? Even better, can you drive it with a test?
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mhh, I don't know - it's just what I'm used to.

OK, let's try the existing structure for some while.
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There are some things that I don't fully like

- the playerNumber and playerIndex methods in TennisContest scare me,
- we don't have unit tests for TennisSet, and
- I don't think that the GameView should have the responsibility of incrementing the set score when a game is won.

I think I will try to tackle the last two, by writing a unit test for TennisSet that makes it responsible for updating the score.

I create an empty TennisSet class and add it to the TestSuite. Just to see that it works, I run the suite and get the expected warning that I have an empty test class.

So let's add a test:



That doesn't compile yet. Eclipse creates the missing method stub for us.

Now the test fails, as expected: junit.framework.AssertionFailedError: expected:<1> but was:<0>

Implementing gameWin is easy:



Ups, test still fails: java.lang.ArrayIndexOutOfBoundsException: -1

I forgot that the player number starts by one, not zero - I swear, this wasn't intentional!

Let's correct the test:



OK, test runs.

Now TennisSet is able to keep track of won games - we only to use it and relieve the view from this responsibility. If I remember correctly, we just need to change the setup of the FunctionTest:



Oops, two problems: TennisSet doesn't implement the interface yet, and the listen method is a misnomer - it's the set which listens to the game, not the other way around.

Let's fix the former first. TennisSet needs to implement GameView.

OK, compiles; tests run.

Now to the refactoring: Let's go with existing Java idioms and call the method addGameListener. As I think about it, there is only one listener allowed, currently, so it should be setGameListener.

That's better. Mhh, the interface then also shouldn't be named GameView, but GameListener.

OK. I also renamed the variables and fields from "view" to "listener". God bless the inventor of refactoring browsers.

Oh, I nearly forgot to rename the MockGameView to MockGameListener. Done.

Tests still run.

The whole point of the exercise was to remove the responsibility from the ScoreCard. Now it doesn't need to implement the View/Listener interface any longer.

I removed the code. Everything still compiles, tests run. Enough for today.
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Originally posted by Ilja Preuss:
- I don't think that the GameView should have the responsibility of incrementing the set score when a game is won.



I meant the ScoreCard, not the GameView, of course... :roll:
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have updated the wiki page http://faq.javaranch.com/view?TennisMatchCode to link directly to the uptodate code in the repository. We still need to update the page when we add classes or change their names.
 
Frank Carver
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Neat. I'm just having a got at my response to your changes ...
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My dictionary doesn't know what "having a got at smthg." means...
 
Frank Carver
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ilja wrote: the playerNumber and playerIndex methods in TennisContest scare me

So that's the first item on my list then.

The offending methods look like:



I agree. These are horrible. The -1 and +1 are purely there to adapt between a domain concept (player number) and an implementation concept (array index). There must be a better way.

Luckily, now that these methods have been abstracted into a common base class, we are almost in a position to change them without impacting other classes. Unfortunately, a quick check shows that "playerIndex" is used in TennisSet, and "playerNumber" is used in TennisGame. Let's start by bringing all those references home to TennisContest.

In TennisSet we have:



An "extract method" gives:



Then it's a simple matter to "pull up" the new method to TennisContest, co-incidentally changing its visibility to "protected".

Run the tests. Good, all still pass.

In TennisGame, the situation is a little more complicated. It's not obviously amenable to an extract-and-move. The offending call is in:



The trick here, I think, is to "pull up" all the winner-notification stuff:



Sure the names will be a bit out of place, but we can fix that when we have made the main change.

To prove that we have achieved the goal of removing all external references to our two problem methods, change them to private:



and Re-run the tests again. Good.

OK, that's the first step done. Now, to solve this properly we need to change how scores are stored. An array indexed by "player" seemed a reasonable idea at the start, but now it is causing trouble, so it has to go.

However, in order to achieve that, we also need to "bring home" external references to the score array.

Most of the references to this field are in TennisContest, which is fine. Unfortunately, there are also two references in TennisGame:



Let's change those array references to methods for the moment, usnig the asme extract-and-move as before:



This is all beginning to feel a bit clumsy, but trust me, it will get simpler in the end.

Again, I check this by marking the score array as private in TennisContest and re-running the tests. Good.

Next step, change "score" from an array to a Map:



This naturally enough gives a bunch of compilation errors. The good thing is that because we made sure all the references were moved back to TennisContest, that's the only class with errors. So let's fix them:

For example:


becomes:



Looks worse, doesn't it? Don't worry. Get the code compiling again first, then we can address these issues.

To get the code compiling quickly, I ended up with this:



Compilation is good, but unfortunately all the tests now fail.

Remember when I changed the score from an array to a map? I took away the bit that set the values to zero. We could do the equivalent in an initialiser block, but I perfer a constructor:



That fixes most of them. But there are still a few errors. It looks as if the failing ones are the three which test that a Set is updated when a player wins a game (one in TennisSetTest, and two in TennisFunctionalTest).

A diagnostic print in TennisSet shows that Win is being called:



I reckon this is all still to do with the two types of player number. And guess what. "notifyWinner" still calls "playerNumber" which we thought we had got rid of:



So let's remove that call:



That, and changing the symbolic constants to be the "real" player numbers fixes all the tests:



Best of all we can now remove the horrible conversion methods that prompted all this in the first place.

We still need to clear up that mess of object/int conversions, though.

All our tests are working, and the code is in the source-repository, so let's take a bold step. Let's change the player symbols to Objects. For clarity in diagnostics, let's make them actually Strings, but make sure that they are always passed around as opaque objects.

This does affact a few classes in our system, but the changes are not hard.



After that, most of the changes are removing the "new Integer()" wherever PLAYER_ONE or PLAYER_TWO are used, and changing the type of a few parameters. Atter that it's just a matter of replacing player numbers with text in the tests.

Eventually the tests run again. And as a nice confidence boster, that diagnostic print is still there in TennisSet, and prints out:



Remove it anyway, so the tests run clean.

It might look like a good place to stop, after all that, but there is still a little more. Remember I complained about all those little methods "point", "addPoint", "playerOneWinsThePoint" all doing very similar things? Well I reckon we can get rid of most of them. Our new player objects are almost good enough to be used as a public interface. All that is needed is for the text of the strings to make sense, rather than being something chosen by me. So let's make them constructor parameters:



These parameters need to be rippled up through TennisGame and TennisSet.

Tests all work again. Good.

I think we are nearly done, but one thing still bugs me. In TennisFunctionalTest we now have:



All that set up stuff looks too ugly and detailed for a functional test, and ther's a worrying chance of the player symbols being different in the Game and Set, which could cause horrible bugs. So I'll move it into the ScoreCard constructor:



This leaves the functional test setUp much nicer:



However, the tests currently call the "..wins.." methods dierctly on a game object. Let's simplify them into a single method, then use the ScoreCard as a proxy:

in TennisGame:



in ScoreCard:



A few minor changes to the tests to use the new interfaces, and removal of the old ones, and the code is smaller again.

And all the tests run. Time to check in again and get back to some other work.
[ August 03, 2006: Message edited by: Frank Carver ]
 
Frank Carver
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My dictionary doesn't know what "having a got at smthg." means...

It means I sometimes type before thinking. I should have been "having a go".

Sorry.
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Originally posted by Frank Carver:

It means I sometimes type before thinking. I should have been "having a go".



Well, you are not alone - google has an incredibly high hit count for "having a got at"...
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mhh, would it be OK if we used Java 5 for this exercise?
 
Frank Carver
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I guess so.

Do you want it just for the autoboxing, or for something else?
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Generics could simplify the use of the score Map. And I'd like to see what happens if we make the players an enum.
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
OK, time for a little bit new functionality. Let's tackle "deuce"!



That should fail. And it does: junit.framework.ComparisonFailure: expected:<...1 Deuce> but was:<...0 40-40>

Oops, there is something wrong in this test - a typical copy-and-past error. Let's correct that:



Now the test fails with the expected error message: junit.framework.ComparisonFailure: expected:<...Deuce> but was:<...40-40>

It will take some time to make that test succeed, but I don't want to always check the number of failed tests when working test first at the unit level. So I will just remove this test from the suite again, by changing its name:



Now we are green again, but know about our goal. When we think we have reached it, we can add the test again and see whether we are right!
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
OK, we first need a new test for TennisGame. But when I open the TennisGameTest, I see a lot of duplication. Refactoring tests is as important as refactoring production code, so I'd like to tackle that first.

One thing that bugs me are the player strings all over the place. In fact I think those shouldn't be strings at all - I prefer it to use the compilers ability to find my typing errors. (I know that this is unusual for someone from the more extreme corner of the Agile community - call it my personal quirk... ).

I will commit the current state of the code and then try to introduce an enum. If we don't like the result, we can always roll back the change.

The Player enum is really simple:



We now could look for a smooth migration path from Strings to the Player enum, but I feel jaunty (?) today, so I will try to change it in one big swoop. I'll just use the text "find and replace" functionality.

After the text replacement, I need to do an "organize imports" for the tests, and change all the constructors from the Type String to Player.

OK, I also needed to change the types of the fields, of course, and the type of the ScoreCard.point method parameter.

Now it compiles again - let's see wether the tests run. They do. Good. Let's commit.
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Now we actually can get rid again of the playerOne and playerTwo fields in ScoreCard and TennisContest. Let's start with TennisContest:



As we are already touching the reset method, we can also make use of autoboxing:



Tests still run. Let's get rid of the constructor arguments.



We can also remove them from TennisGame and TennisSet. In fact, for TennisSet, the default constructor suffices now. And really like to delete code!





Tests still run! Now we do the same to ScoreCard.

Again, tests still run. Again, time to commit.
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Now let's take a look at an example of a game test:



I like that better than before, but your mileage may vary. Anyway, there is still some duplication left.

For example, take the line

assertEquals(null, listener.winner());

which is repeated over and over again. I will extract it into an assertNoGameWinner() method.



Let's also see what we can do with those lines like

assertEquals(1, game.getScore(Player.TWO));



I like to refactor my tests in these ways - if we later change the way our production class works, we will likely have much less work to do adjusting our tests.

For reasons of symmetry, let's also extract that last assert statement.



We should also adjust the other test, of course.



Tests still run. Now it's *really* time to write a new test. Remember? We wanted to implement "deuce"...
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So let's write a unit test for "deuce".



As you can see, I decided that the game would know whether it is in a "deuce" state. I also made sure that a deuce means that there is no winner yet. I assume that once we are in a deuce state, we don't care about the player's actual scores anymore, so I don't test for it.

Of course this doesn't compile yet. We need to creat that isDeuce method.



Now the tests should fail. And they do.

Making the test pass is easy:



Yes, that's right - all tests pass! But that's wrong, isn't it? We need more tests...



That test fails and forces us to correct the production code.



Ouch, that fails. I guess the "3" is wrong.

Yes, the testDeuce fails, so it still thinks there is no deuce. That should probably be a "2".

Now the testNoDeuceBefore40_40 fails. The last block actually already is a 40-40. Stupid me...



Now the tests run all green. Phew. Time to commit...
 
Ilja Preuss
author
Posts: 14112
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Now TennisGame knows about Deuce. Seems to me the only thing missing is the ScoreCard knowing about it, too. Let's write a test for that.

We don't yet have a ScoreCardTest - shame on us. Well, it's never to late to start...



Mhh, I'd actually prefer not to have to code all the calls to the point method just to test that the ScoreCard knows how to show a Deuce. I'd like to mock the game, but for that, we have to pass the game into the score card again. Let's change that first. We can also remove the point method from the ScoreCard again.

Tests run. Now we need to introduce an interface for the TennisGame to be able to mock it. It only needs to contain the methods getScore(Player) and isDeuce(), because that's all the ScoreCard needs to know. TennisGameScore might be a good name. I create the interface by using the "Extract Interface" refactoring that Eclipse provides.





Now we can finally write that test:



It fails as expected, because the getScore method gets called.

Let's correct the implementation:



Tests run!

It's time to activate that acceptance test again.

That one passes, too! Success! And enough for today...
 
Frank Carver
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I read through those last few posts with a wry smile. It seems as if we each have a different "vision" for where the design is going, and it "flips" and "flops" each time we hand over.

If were were in the same place, I'd definately want to spend some time now discussing class responsibilities. I think we have slightly different ideas about what each class is for. I don't usually use CRC cards much myself, but this seems like a good occasion to use them.

Can you think of a good way to do something like this in the asynchronous way we are currently working?

Ilja wrote: One thing that bugs me are the player strings all over the place. In fact I think those shouldn't be strings at all - I prefer it to use the compilers ability to find my typing errors.

Ha! That'll teach me to proceed using Strings without driving it with a test. I even had such a test in mind, but I got so caught up in the process and writing about it that I never added that one.

I'll admit that I was also feeling the itch of the same String text appearing lots of times. My approach, however, would have probably been a smaller step - such as to note that they only appear in the test cases, and use a simple "extract constant" refactoring to get the compiler on my side

Now I'll synchronise code and take a good look at where it has got to, then I'll come back with a response later.
 
Frank Carver
Sheriff
Posts: 6920
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
OK. I hadn't really thought about it before, but I reckon one of the reasons I don't use enums much is because they evoke in me the same sort of unease as Singletons. Hard-coding an enum reference deep in a call hierarchy ties things too closely for my liking. I guess that's where I am "extreme".

Anyway. Given that the unwritten rules of this game seem to be that you have to work with what you are given, my first step is to localize the use of the Player enum. A quick check using Eclipse's reference finder shows that Player enum is used (not including the tests) in ScoreCard, TennisContest, and TennisGame. I'll try and limit it to ScoreCard for the moment.

The use in TennisContest is a bit smelly anyway. It currently does:



I can replace this with a slightly more wordy, but more generic, solution:



As Ilja has already "raised the bar" and added a dependency on Java 5, let's use the considerably neater "for-each" syntax:



That looks good, but suddenly almost all the tests fail. The reason? The initial call to "reset" in the TennisContest constructor is looping round an empty Map, and resetting no scores. Whoops.

Now this is a bit tricky. My aim for this refactoring is to remove dependencies on Player from TennisContest, so I can't hard-code Player.ONE and Player.TWO in the constructor. Even though my natural inclination would be to pass in the player objects, that doesn't seem to be where Ilja wants to go, so let's look at alternatives.

Currently the TennisContest constructor looks like:



A few possibilities occur to me:

  • Pass in some sort of list of players, and insert them into the Map
  • pass in a pre-populated Map object and use it as-is
  • pass in a pre-populated "prototype" map, and copy it


  • The first option is not much different from passing the players in individually. The second runs the risk of accidentally using the same map for two objects, so I plump for the third:



    The downside is that we now pass in a specific type of Map, which I don't usually like to do.

    Now, I'd love to be able to use Java's Cloneable interface here, but (IMHO) Java's type system is screwed up around the operation of the "clone" method. So even though HashMap implements "Cloneable" and "Map", and has a working "clone" method, it can't be used in this case. Cloneable has no methods, not even "clone()". huh?

    So let's create a small new interface, CloneableMap which extends Map with a "clone" method:



    The TennisContest constructor now looks like:



    This has, of course, broken the classes which extend TennisContest, TennisGame and TennisSet. The TennisGame constructors look like:



    Let's just pass through the CloneableMap:



    TennisSet currently has no explicit constructors, so let's add one:



    Now the domain classes all compile, but several of the tests don't. All of them fail because they are calling the no-arg constructors of TennisGame and TennisSet.

    Now, I could create some sort of player map objects in each of these tests, but that seems needless duplication. All of the tests are testing the same sort of tennis games, so why not share a base class between the tests:



    Now all we need to do is to make all our test case classes inherit from this one, and pass the "players" field to the TennisGame and TennisSet constructors.

    The whole lot now compiles again, but I know that when I run the tests there will be a problem. And I'm right. Now all the tests fail. Because I have not initialised that "players" object.

    Now, I'd love to be able to just assign a HashMap to it, but HashMap doesn't implement our new CloneableMap interface, even though it has all the methods - we need another tiny adapter class:



    Now we can add it bit of initialisation code to TwoPlayerTestCase:



    And everything works again. All the tests pass.

    Phew. Check it in.
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Originally posted by Frank Carver:
    I read through those last few posts with a wry smile. It seems as if we each have a different "vision" for where the design is going



    Not only that, but actually quite different preferences of style. But that is part of what makes it so interesting...


    and it "flips" and "flops" each time we hand over.



    Yes, but also partly because I like to experiment. And I don't think it really flops back into the same position - the design really isn't like I envisioned it, I've already learned a lot from your ideas.

    Of course this would be much easier if we were sitting in the same room...

    If were were in the same place, I'd definately want to spend some time now discussing class responsibilities. I think we have slightly different ideas about what each class is for. I don't usually use CRC cards much myself, but this seems like a good occasion to use them.



    Yes. I've recently heard about cardmeeting.com, but it seems to be down right now...

    I'll admit that I was also feeling the itch of the same String text appearing lots of times. My approach, however, would have probably been a smaller step - such as to note that they only appear in the test cases, and use a simple "extract constant" refactoring to get the compiler on my side



    I thought about that, but somehow using Strings felt inadequate to me. I guess that's one of the basic difference of our styles - I like the expressiveness of explicite classes, whereas you seem to prefer the flexibility of "primitives"?

    I'm sure we will find a good mix.
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Originally posted by Frank Carver:
    OK. I hadn't really thought about it before, but I reckon one of the reasons I don't use enums much is because they evoke in me the same sort of unease as Singletons. Hard-coding an enum reference deep in a call hierarchy ties things too closely for my liking. I guess that's where I am "extreme".



    Mhh, to me, enums really are like typed constants. And I'm not that much concerned about using them all over the code in this case. After all, it is hardcoded all over the place that we are dealing with exactly two sides (the whole deuce idea wouldn't make sense otherwise). I don't expect it to make much of a difference what objects we use as keys, so I'm not troubled by making the keys globally known. But probably I'm missing something - it wouldn't be the first time I had to "unfactor" a design decision.
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I checked in the .settings folder of the eclipse project, so that we use the same formatting etc. Change it as you like - it's just important to me that I use the same settings.
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    TennisGame.isDeuce still references the Player enum:



    Frank, any idea how you want to tackle that?
     
    Frank Carver
    Sheriff
    Posts: 6920
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    OK, that has removed the Player dependency from TennisSet. Now, can we do anything with TennisGame?

    The only place that Player is referenced in TennisGame is in the "isDeuce" method:



    Now, I need to declare an opinion here. Personally I feel quite strongly that this method (and especially the tests that call it) are the wrong way to go.

    Back when I was thinking about how to deal with a game win, I deliberately rejected the idea of an accessor method on TennisGame to fetch the winner. This sort of polling "pull" functionality seems completely at odds with the way a tennis game actually works. I really don't see any justification for client code to keep asking the TennisGame what its status is, or for the TennisGame to expose such details of its internal workings.

    Driving the creation of this method with tests is (for me, at least) an example of how test-driven developement can sometimes drive poor designs as well as good ones. It's all too easy to quickly create a test, then create an implementation which matches it, even though neither add much value to the solution. Worst of all though, enshrining the use of such a method in the test cases makes it seem like a real customer requirement, and makes it much too "solid" and hard to change.

    Anyway. Rant over. Now to "fix" it

    Let's consider some possibilities:

  • add a "callback" like we did for the winner
  • move the "deuce" decision to ScoreCard
  • introduce a new class to manage "deuce" processing


  • This is a tough decision. In many ways it comes down to class responsibilities as I mentioned above. Here's how I see the class responsibilities at the moment, even though I feel that Ilja may have a different take.

    The job of TennisContest is to provide common behaviour between the "contest" classes TennisGame, TennisSet, and a potential TennisMatch.

    The job of the "contest" classes is to manage the score of a particular contest, including notifying anyone interested of a win condition.

    The current job of ScoreCard is to manage the overall scenario, including "wiring" together contest classes and player definitions and acting as a "facade" to isolate clients from implementation details, This class also has the job of displaying the scores in a textual format.

    Written like that, ScoreCard seems heavy compared with the others. It has too many different responsibilities.

    I can see the potential for three separate class roles here. One to manage the scenario and wiring up the collaborators. One to act as a client facade, and one to format output.

    It seems that my steps in this refactoring game have tended toward leaving ScoreCard with the facade and scenario roles, whereas Ilja's steps have tended toward leaving ScoreCard with just the formatting roles, and kicking the facade and scenario roles back into the tests.

    I feel quite strongly that the functional tests should not need to know about the implementation of game, set, match and player objects, and how they are wired together. I want to see functional tests which just say "create a tennis competition between these two players. here is a sequence of points scored, what's the score card?" This was, after all, the nearest thing to a customer specification that we had, at the start of this thread.

    For this to work we need some sort of facade, but this need not be the current ScoreCard class.

    It has changed several times, but the current ScoreCard is almost all formatting. So let's change its name to TennisFormatter.

    Now let's rewrite the functional tests in terms of a facade class and see what we need to make to hook that up with the existing implementation.

    The current "setUp" method in the functional tests looks like:



    I want to replace that with somehing much simpler. How about:



    To make this compile, we need to create a Tennis class with the right constructor. Luckily Eclipse can do this for us. Then we just need to paste in the old members and set up code from the fuctional test:



    Hmm. Now this wonn't compile because it is using the "players" map we set up in the test case base class. Let's copy over the setup from there for now:



    There are things about that which I don't like, but my main job at the moment is to get things compiling again, so I'll defer these changes.

    Now that looks much more like a functional test set up to me. It breaks a lot of tests. Let's fix them one by one.



    This one won't compile because we have no "scoreCard" member any more. Let's provide a "scoreCard" method on our Tennis facade:



    and search/replace all the functional tests to use it:



    Getting better, but the next test case is still broken, too:



    There is no "game" object. Let's add a "point" method to our tennis facade:



    and find/replace as before to use it in the functional test case:



    Excellent, it all now compiles again, and all the tests pass. I'll check in the changes, and continue with the next step in the next post.
     
    Frank Carver
    Sheriff
    Posts: 6920
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Ilja wrote Frank, any idea how you want to tackle that?

    Looks like we overlapped a little. These descriptive posts take so darn long to write.

    As for the formatting, I was getting fed up with tabbing looking awful, so I opted for four-spaces and reformatted the whole project. I'm sure the original problem was at my end as I have been doing this on two different machines with two different sets of Eclipse defaults. It shouldn't happen again.

    Anyway, I've still got a bit more to go before I hand over this chunk, so I'll get back to it.
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Here is a little refactoring:

    As I said earlier, the use of the Map in TennisContest can be simplified a little bit by using generics (plus autoboxin):



    Another thing we can make use of are covariant return types in CloneableMap:



    This simplifies the call to clone() in TennisContest to



    I'm not sure whether you will like it, Frank, but somehow I do...

    OK, now I will go to bed, before I wreak more havoc...
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Oops, sorry for the overlap - guess I was a little bit to enthusiastic.

    If you have trouble merging my changes, don't hesitate to just override them.
     
    Frank Carver
    Sheriff
    Posts: 6920
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I'm trying to commit some of my changes and getting the following:

    commit -m "introduced new Tennis facade to align ScoreCard concepts
    updated functional tests to use new facade" E:/projects/tennis2/TennisMatch/org/javaranch/ScoreCard.java E:/projects/tennis2/TennisMatch/org/javaranch/Tennis.java E:/projects/tennis2/TennisMatch/org/javaranch/TennisFormatter.java E:/projects/tennis2/TennisMatch/tests/ScoreCardTest.java E:/projects/tennis2/TennisMatch/tests/TennisFunctionalTest.java
    Deleting E:/projects/tennis2/TennisMatch/org/javaranch/ScoreCard.java
    Adding E:/projects/tennis2/TennisMatch/org/javaranch/Tennis.java
    Adding E:/projects/tennis2/TennisMatch/org/javaranch/TennisFormatter.java
    svn: Commit failed (details follow):
    svn: Commit failed (details follow):
    svn: COPY of '/svnroot/jfcfixtures/!svn/bc/13/JavaRanch_TennisMatch/trunk/org/javaranch/ScoreCard.java': HTTP/1.1 502 Bad Gateway ('https://svn.sourceforge.net')

    This is strange, because I can do other svn operations (I updated your changes, and committed some other files, for example)

    Have you ever seen this sort of thing before?
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Originally posted by Frank Carver:
    The only place that Player is referenced in TennisGame is in the "isDeuce" method:



    Now, I need to declare an opinion here. Personally I feel quite strongly that this method (and especially the tests that call it) are the wrong way to go.



    Frankly, I didn't see it as the final solution - actually I expected it to be a temporary one. I mainly used it as the "simplest thing that could possibly work" to drive the "deuce algorithm".

    Back when I was thinking about how to deal with a game win, I deliberately rejected the idea of an accessor method on TennisGame to fetch the winner.



    Which was an interesting idea I liked quite a lot!

    This sort of polling "pull" functionality seems completely at odds with the way a tennis game actually works.



    I can see how it was a good solution for "game winner". The deuce thing looks a bit different to me - it doesn't really depend on how tennis works in general, but whether our scoring display will work "push" or "pull".


    I really don't see any justification for client code to keep asking the TennisGame what its status is, or for the TennisGame to expose such details of its internal workings.



    I guess we need to make it a real, usable application to decide that. We probably should have done that much earlier. If you like, I could try that next when it is my turn...

    Driving the creation of this method with tests is (for me, at least) an example of how test-driven developement can sometimes drive poor designs as well as good ones. It's all too easy to quickly create a test, then create an implementation which matches it, even though neither add much value to the solution. Worst of all though, enshrining the use of such a method in the test cases makes it seem like a real customer requirement, and makes it much too "solid" and hard to change.



    With all due respect, without a true client, your decision to *not* use a getter sounds as arbitrary to me. But I'm not very troubled by it - I don't yet feel that it's hard to change at all...


    The job of TennisContest is to provide common behaviour between the "contest" classes TennisGame, TennisSet, and a potential TennisMatch.

    The job of the "contest" classes is to manage the score of a particular contest, including notifying anyone interested of a win condition.



    Same here so far.


    It seems that my steps in this refactoring game have tended toward leaving ScoreCard with the facade and scenario roles, whereas Ilja's steps have tended toward leaving ScoreCard with just the formatting roles, and kicking the facade and scenario roles back into the tests.



    That sounds about right, yes.



    For this to work we need some sort of facade, but this need not be the current ScoreCard class.



    I'm not sure that we really need that facade, or that it needs to be in the production code, but we will see...

    It has changed several times, but the current ScoreCard is almost all formatting. So let's change its name to TennisFormatter.



    Mhh, I think ScoreCard was just fine. What else is a score card then the formatted view of the current score of a match?
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Originally posted by Frank Carver:

    Have you ever seen this sort of thing before?



    If so, I don't remember.

    From http://www.checkupdown.com/status/E502.html it sounds as if you can't do much but try again later...
     
    Ilja Preuss
    author
    Posts: 14112
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Originally posted by Frank Carver:

    I want to replace that with somehing much simpler. How about:




    Frank, I don't think we have any requirement for actual player names from our customer, do we?
     
    Frank Carver
    Sheriff
    Posts: 6920
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hmm. I'm doing three things at once here. In one window I have the description of my next changes, which (hopefully) should answer some of this stuff. In this window I'm discussing philosophical issues, and in Eclipse I'm swearing at subversion

    My leaning toward naming ScoreCard as ScoreFormatter is solely to make its job clearer. For a long while, all ScoreCard said to me was "facade". I had a mental image of an "electronic score card" running on a PDA and acting like a smart version of a paper form. The user starts it up, enters the player names, then taps a "hot spot" appropriate to the player each time a point is scored. Each time a point is tapped, the overall score is updated on the screen.

    My understanding may be aligned, or at odds, with the original customer's wishes, but all I can be sure of at the moment, is that my understanding differs from yours. So I thought I'd make it clearer by separating the two concepts and creating a class for each.

    I reckon it's much harder to argue about the responsibilities if a "score formatter" than a more abstract "score card".

    As a small teaser, I also have in mind the idea that there are several different ways of formatting tennis scores, so I want to make sure our design doesn't make it too hard to add pluggable behaviour for this later.

    As for the subversion problems I'm getting very fed up with that. I may just push on with my next changes, and try to sort out the commit issues when I've finished.
     
    Frank Carver
    Sheriff
    Posts: 6920
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    That last post was a bit of detour, but I think it might have helped clear up the question which prompted it. Where shall we put the "deuce" processing?

    The new "lite" ScoreCard (a.k.a TennisFormatter) class seems the logical place to me. Its job is all about formatting, and interpreting three pointe each (or four points each, or whatever) as "deuce" sounds like formatting to me.

    So let's leave the TennisGame class just responsible for counting scores and determining a win:



    I know the victory condition stuff is still wrong, but we can add tests to drive that later.

    Let's just move the old method to TennisFormatter and tweak the method recievers a little to make it work.



    To me, it looks much more at home here.

    To make this work, I'm going to unroll Ilja's TennisGameScore changes. These were all there to support the test in ScoreCardTest so I'll begin by commenting that out. I'll also comment out the tests for the deuce behaviour in TennisGame Test. I'm expecting that eventually something like them will end up in ScoreCardTest (or TennisFormatterTest as it probably ought to be)

    Then I'll go though replacing TennisGameScore with TennisGame again.

    TennisGameScore is now no longer referenced, so we can remove it.

    A quick check. All tests pass except a warning from the one we have commented out.

    So let's change the name to TennisFormatterTest and bring it back.



    Obviously this fails to compile. TennisGameScore has gone. TennisFormatter takes a TennisGame as a parameter so we need to pass one in. What we don't want to do is duplicate all the steps to get to a deuce. We need a TennisGame and a way to "poke" scores directly in. Sounds like a subclass to me:



    That almost works. It just needs the "score" array in TennisContest to be marked as protected.

    Good. All tests pass. Now to add a few more "deuce" tests:



    They work OK, but there's a lot of duplication. So let's "extract method":



    That's better.

    Remember back at the start of this session, my aim was to rationalise the use of the Player enum? Well, it's still used in TennisFormatter, but I'm not just going to change it, I'm going to drive the next bit from a test for real user-level functionality.

    Let's add another test to TennisFormatterTest:



    Guess what, it fails with "expected <Advantage McEnroe> but was <40-null>". generating the advantage bit is pretty straighforward now we have done deuce:



    OK, that boolean expression looks a bit messy, but we can sort that out later. The most important thing is that the "Advantage" text is correct and the test only fails because the player name is not shown.

    Somehow we need to get that name. But in order to do so, we need to know which player has the advantage. So let's change the return type of the isAdvantage method to Object:



    Now we are getting closer. It now gives "Advantage ONE". Now we just need to return the correct player name. We don't have access to them in this class, so let's pass them in to the constructor for now:



    Now we can use these variables instead of Player.ONE and Player.TWO in the advantage code:



    and a few small fixes to the constructor calls in TennisFormatterTest:



    and Tennis:



    All tests pass once again.

    Personally, I still feel that there is a certain amount of duplication between the Player enum and the player names used in the formatter, but I can live with that for the moment. I really feel that it is time I let go of the virtual keyboard for a while.

    I still can't get my changes to commit to subversion, so if you wish to continue with this, you can download a snapshot from http://www.servletshop.com/downloads/TennisMatch.zip
     
    Ranch Hand
    Posts: 1296
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I have to say I love this thread, especially the metaphor of a tennis match juxtaposed against the back and forth between Ilja and Frank.
     
    With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!