Clean code

[Kendall Ponder]

Junilu,
You have given me a lot to think about. It may take a while for me to process it but when I do I will give a response. Thanks for all of the ideas and all the time you spent on responding to my code.
kp

[Junilu Lacar]

A lot of the code I suggested can replace the code that you have, especially the advanceXXX methods.

The Play.advanceFrom(Base) method might have this implementation:

public enum Play {
	SINGLE {
		@Override
		final protected int gain() {
			return 1;
		}
	},

	DOUBLE {
		@Override
		final protected int gain() {
			return 2;
		}
	},

	TRIPLE {
		@Override
		final protected int gain() {
			return 3;
		}
	},

	HOME_RUN {
		@Override
		final protected int gain() {
			return 4;
		}
	};

	protected abstract int gain();

	public Base advanceFrom(Base start) {
		Base newBase = start;
		for (int i = gain(); i > 0; i--) {
			if (newBase == Base.HOME)
				break;
			newBase = newBase.succ();
		}
		return newBase;
	}

	private static final Random generator = new Random();
	
	public static Play randomPlay() {
		final int max = Play.values().length;
		return Play.values()[generator.nextInt(max)];
	}
}

enum Base {
	AT_BAT, FIRST, SECOND, THIRD, HOME;
	
	Base succ() {
		return EnumUtils.succ(Base.class, this);
	}
}

The succ() method can be implemented to call a utility such as what I have here: https://coderanch.com/t/606842/java/java/built-pred-succ-enumerated-types

Looking at that code, I might even start questioning whether Play should be an enumeration or not.

And of course, I'd write the tests first:

@Test public void single_advances_from_at_bat_to_first() {
    assertThat(SINGLE.advanceFrom(AT_BAT), is(FIRST));
}

@Test public void single_advances_from_first_to_second() {
    assertThat(SINGLE.advanceFrom(FIRST), is(SECOND));
}

@Test public void single_advances_from_second_to_third() {
    assertThat(SINGLE.advanceFrom(SECOND), is(THIRD));
}

@Test public void single_advances_from_third_to_home() {
    assertThat(SINGLE.advanceFrom(THIRD), is(HOME));
}

// similarly
@Test public void double_advances_from_at_bat_to_second() {
    assertThat(DOUBLE.advanceFrom(AT_BAT), is(SECOND));
}

// .. and so on

This code works; I tried it.

Of course, you'd have to figure out where to put logic when advancing bases is not as straightforward. But again, this would be a great place to start.

[Junilu Lacar]

As an example of how ruthlessly you should refactor your code, here's one that I just made:

// change this
public static Play randomPlay() {
    final int max = Play.values().length;
    return Play.values()[generator.nextInt(max)];
}

// to this
public static Play randomPlay() {
    int random = generator.nextInt(Play.values().length);
    return Play.values()[random];
}

The second version makes it clearer that we're picking out a random Play value, IMO. The previous version detracts from that thought a little with the max variable. You could go one step further:

private static int randomPick() {
   return generator.nextInt(Play.values().length);
}

public static Play randomPlay() {
    return Play.values()[randomPick()];
}

All depends on how ruthless at refactoring you want to be.

[Junilu Lacar]

Kendall,

I think it's great that you're reading Uncle Bob's "Clean Code" book and taking the advice there to heart. A couple of books that I think complement CC very nicely:

* Pragmatic Unit Testing in Java 8 with JUnit by Jeff Langr

* Understanding the Four Rules of Simple Design by Corey Haines

Good luck with writing cleaner code!

[Kendall Ponder]

I just ordered the JUnit book. Between it, Clean Code and writing code for an apprenticeship program I'm working with I may disappear for awhile before I go through all of your suggestions. I graduated with an EE/Computer Eng degree back in 1983 (my first programming class was in Fortran with punch cards). My second Fortran class made a big deal of "Goto less programming". I worked at Emerson Electric as a micro-coder on an in-house parallel processor from 1983-86 and then joined the Army Rangers. When I got out I got my Master's in EE and wrote my thesis in Lisp and did some C programming. I then went into teaching and in the meantime the programming world turned to Object Oriented programming, Unit testing TDD etc. I am retiring from teaching in a week and have been learning Java for the last few months to try to get back into programming. My point in telling you all of this is to explain why some of your comments such as code being procedural and writing the test first didn't make sense to me at first because those weren't issues in 1986. Thanks for the help and I'm sure I will have more questions in the future. I may try to develop the Baseball program from scratch using TDD and this board as a resource.

[Junilu Lacar]

I just read Haines' book recently and a lot of the ideas I shared in this thread are based on some of the things he discusses in his book. Thanks for sharing more about your background. I've been programming for over two and a half decades now, started on Apple ][s and TRS-80 and CP/M, Turbo Pascal, xBase, and worked my way through RPG, COBOL, Delphi, and finally Java. Now I find myself slowly shifting to Scala and Javascript. TDD is an awesome technique when you get a hang of it but it takes time and practice. Lots of practice. Which is why I really like Haines' book because it has so many examples and different ways to refactor code.

[Kendall Ponder]

My college outfitted a lab with TRS-80s after my Senior year and I thought they were awesome, nothing like having 256K of storage on a floppy disk to go with your 64K of RAM! I'm sure I will eventually get the Haine's book. Thanks again!

[Dave Tolls]

Junilu, out of curiosity, in your Play enum above, why have an abstract gain() method rather than a concrete one that returns a given gain attribute, populated in an enum constructor?
There seems to be a lot of code duplication there with all the Overrides of gain().

Of course, I might have missed something...it has been known.

[Junilu Lacar]

Dave Tolls wrote:Junilu, out of curiosity, in your Play enum above, why have an abstract gain() method rather than a concrete one that returns a given gain attribute, populated in an enum constructor?
There seems to be a lot of code duplication there with all the Overrides of gain().

Of course, I might have missed something...it has been known.

You didn't miss anything, I did. Good catch (pun intended)

Here's the refactored code (also includes something I'm trying out to handle plays that won't necessarily force all players to advance):

public enum Play {
		
	WALK(1, false), SINGLE(1), DOUBLE(2), TRIPLE(3), HOME_RUN(4);

	private final int gain;
	private final boolean movesAll;
	
	private Play(int gain) {
		this.gain = gain;
		this.movesAll = true;
	}
	
	private Play(int gain, boolean movesAll) {
		this.gain = gain;
		this.movesAll = movesAll;
	}
	
	private static final EnumFunctions<Play> func = new EnumFunctions<>(Play.class);

	public Base advanceFrom(Base start) {
		Base newBase = start;
		for (int i = gain; i > 0; i--) {
			if (newBase == Base.HOME)
				break;
			newBase = newBase.succ();
		}
		return newBase;
	}

	public boolean movesAll() {
		return this.movesAll;
	}
	
	public static Play randomPlay() {
		return func.randomValue();
	}
}

enum Base {
	AT_BAT, FIRST, SECOND, THIRD, HOME;
	
	private static final EnumFunctions<Base> func = new EnumFunctions<>(Base.class);
	
	Base succ() {
		return func.succ(this);
	}
}

I'm still toying with the idea around movesAll. Maybe it should be greedy instead to imply that the play advances as many players/bases as possible. I also refactored my EnumUtils from a static class to one that you'd instantiate for a specific enum type. That also helped me get rid of a lot of duplication.

[Junilu Lacar]

When looking for good names, one of the best sources for inspiration is the domain itself. The MLB official rule book refers to singles, doubles, and home runs as "advances" so that's what I'm calling this enum, instead of Play.

This is turning out to be quite an interesting exercise in design and refactoring.

[Junilu Lacar]

The advanceFrom() method name got redundant but it was quite ugly to begin with anyway. I prettied it up a little after changing the name of the enum to Advance:

	public Base from(Base start) {
		Base newBase = start.succ(gain);
		return newBase != null ? newBase : HOME;
	}

I should have taken a cue about the name from the tests in the first place. Figures. The tests still read nicely without any change at all, other than what was made from Refactor-Rename:

@Test 
public void single_advances_from_first_to_second() {
    assertThat(SINGLE.from(FIRST), is(SECOND));
}

@Test
public void single_advances_from_second_to_third() {
    assertThat(SINGLE.from(SECOND), is(THIRD));
}

...

I kind of feel that start.succ(gain) should be clarified though. The no-args successor method just gives the enum value immediately after the current one but I added an overloaded version to give back the Nth successor, null if none.

Meh, a decent IDE should show the JavaDoc comments when you hover over that code anyway. ¯\_(ツ)_/¯

[Kendall Ponder]

Junilu Lacar wrote:The issue I had on seeing the play generated by a Player is that the "player" concept isn't just about who is at bat. There are players who are on the bases, running. There are players in the dugout, waiting for their turn to bat. To have Player generate a random play like DOUBLE, SINGLE, etc. doesn't seem to fit. This is why it would be helpful to see what tests you've written. Tests should provide succinct examples of how the code should be used. If I have to look at the code, there's a lot more I have to sift through. ?

Since you are still playing with the code I will explain the purpose of the program. It is designed to let a user choose a 9 player team with each player having the statistics of their choice and then seeing what the expected number of runs the team would score (or percentage of wins if you play another team) by running a large number of iterations. That allows a user to see if a 40 home run 0.200 hitter is worth more than a 10 home run 0.350 hitter, what success rate of steals makes stealing worthwhile, the best batting order, etc. I am not trying to simulate an entire game, just the ultimate results. So I don't have balls & strikes. I also ignore things like scoring on a fly ball from third. Going from 1st to third on a single, etc. to simplify the program. That is why the player only has an atBat method because that is the only time it matters which player is involved, other than when a player is on first and steals are allowed. I didn't care how many runs an individual player scored so I didn't keep track of who scored, only that a run was scored. The program doesn't care who is up to bat next because there is no decision making in the program like changing pitchers. Obviously you can change the program any way you like but I thought I would give you a general outline of my original big-picture vision.

[Junilu Lacar]

Hi Kendall,

Yeah, I had an inkling that statistics were going to be involved at some point here. That might make having a method in Player that gives back SINGLE, DOUBLE, etc. appropriate but I still think the name atBat itself is not a good one. There's probably a name that will fit better and make you go "Aha! Yes, that's what I meant to say!"

[Kendall Ponder]

Junilu Lacar wrote:Hi Kendall,

Yeah, I had an inkling that statistics were going to be involved at some point here. That might make having a method in Player that gives back SINGLE, DOUBLE, etc. appropriate but I still think the name atBat itself is not a good one. There's probably a name that will fit better and make you go "Aha! Yes, that's what I meant to say!"

I agree. More evidence that Uncle Bob is right that you are always looking to improve names.

[Junilu Lacar]

Tim Ottinger actually wrote the chapter about Names in CC and I think he also has some collaborations with Jeff Langr, "Agile in a Flash" if I'm not mistaken. Anyway, I have a feeling that the Law of Demeter will be important to keep in mind when you program around the collaboration between Player and Statistics. Corey Haines' book has some interesting examples of how applying LoD to reverse perspectives can result in cleaner code. So instead of using a bunch of getter methods to gather values and do calculations, you might have the Player pass itself to a Statistics object:

stats.likelyResult(player);

// or, if calling from a Player instance:
stats.likelyResult(self);

Since the stats object is the "information expert" on the percentages, it's best to keep calculations that involve percentages encapsulated within the Stats class. In other words, you tell it to calculate likely outcomes rather than ask it for information so that you can calculate likely outcomes. Does that make sense?

[Kendall Ponder]

Junilu Lacar wrote:Tim Ottinger actually wrote the chapter about Names in CC and I think he also has some collaborations with Jeff Langr, "Agile in a Flash" if I'm not mistaken.

stats.likelyResult(player);

// or, if calling from a Player instance:
stats.likelyResult(self);

I didn't pay attention to the fact different people wrote different sections. I think one reason names are so difficult is the namer knows exactly what the program does and knows the point they intend to get across. I know your name likelyResult intends to convey the fact that the result is related to the probability of the player having a particular result. But I think if I was reading this the first time I would think it was giving a likely result based on the players stats but it may actually be an unlikely result. A player who hits one triple in two years may hit a triple this time. I also had a tendency to think it was the most likely result (I know the name doesn't say that, but that did pop in my head). It seems to me we need a name which conveys two points. 1) The result is the result of this particular at bat for the player. 2) The likelihood of any particular result is based on the probabilities of that particular player. Your name may do that better than mine but I hope to come up with one better than both.

I teach high school Physics and a common problem is an object sliding down a ramp with a force opposing it up the ramp. (This is not a coding story so feel free to skip it). Normally we call the the component of gravity pulling down the ramp FD (force down). The problem I ran into was the natural name for the force up the ramp would be FU which has obvious problems. So I changed FD to FH for force horizontal which made sense to me because I meant the force horizontal to the surface of the ramp but my students got it confused with the force horizontal to the ground which we used in other problems so I went back to using FD. The point of the story is it is difficult to predict the meaning a particular reader will put on a name. I have some questions about the rest of your post which I will put in my next post.

[Junilu Lacar]

Kendall Ponder wrote: I think one reason names are so difficult is the namer knows exactly what the program does and knows the point they intend to get across. I know your name likelyResult intends to convey the fact that the result is related to the probability of the player having a particular result. But I think if I was reading this the first time I would think it was giving a likely result based on the players stats but it may actually be an unlikely result. ... it is difficult to predict the meaning a particular reader will put on a name.

This is exactly why the better teams favor collaborative development, either through pair or mob programming. These are the kinds of discussions I end up having with my teammates most of the time when we program as a group. To an outsider, it may seem like we spend an inordinate amount of time picking out good names but the results of our efforts are evident in the increased quality and shared knowledge and understanding of what our software does.

So to get a statistically influenced "at bat result", you'd have to pick a play at random and yet skew the result in favor of past performance trends while still leaving the possibility of getting an "unlikely" play. The only thing I can think of is to set up an array of 100 plays with a distribution of plays based on the player's statistics. You would then shuffle these choices and pick one out at random. This responsibility still seems to be best given to the Statistics object to handle but I can't think of a good name for it. I'm going to go with randomPlay() for now. Sometimes when my team and I are stuck on a name like this, we just use a nonsense one like foo() or blah() and that sometimes forces our brains to work harder to find the right name. YMMV.

[Kendall Ponder]

Junilu Lacar wrote:

Kendall Ponder wrote:

So to get a statistically influenced "at bat result", you'd have to pick a play at random and yet skew the result in favor of past performance trends while still leaving the possibility of getting an "unlikely" play. The only thing I can think of is to set up an array of 100 plays with a distribution of plays based on the player's statistics. You would then shuffle these choices and pick one out at random. This responsibility still seems to be best given to the Statistics object to handle but I can't think of a good name for it. I'm going to go with randomPlay() for now. Sometimes when my team and I are stuck on a name like this, we just use a nonsense one like foo() or blah() and that sometimes forces our brains to work harder to find the right name. YMMV.

What I did was find the probability of all possible outcomes and made a series of cut-offs and compared a random number to them. For example if a player has a 20% of getting a hit, 30% of a walk and 50% of an out the hit cut-off would be 0.2 and the walk cut-off would be 0.2+0.3=0.5. Then if the random number is less than 0.2 it is a hit, else less than 0.5 a walk, else an out. I picked some player stats from the MLB site to find reasonable percentages. When I tested 100,000 trials the distribution matched the probability. I don't have the test code because I didn't know about unit testing at the time.

[Kendall Ponder]

Junilu Lacar wrote:Tim Ottinger actually wrote the chapter about Names in CC and I think he also has some collaborations with Jeff Langr, "Agile in a Flash" if I'm not mistaken. Anyway, I have a feeling that the Law of Demeter will be important to keep in mind when you program around the collaboration between Player and Statistics. Corey Haines' book has some interesting examples of how applying LoD to reverse perspectives can result in cleaner code. So instead of using a bunch of getter methods to gather values and do calculations, you might have the Player pass itself to a Statistics object:

stats.likelyResult(player);

// or, if calling from a Player instance:
stats.likelyResult(self);

Since the stats object is the "information expert" on the percentages, it's best to keep calculations that involve percentages encapsulated within the Stats class. In other words, you tell it to calculate likely outcomes rather than ask it for information so that you can calculate likely outcomes. Does that make sense?

This brings me back to my original post but I didn't have any code to give the question context. Is it better to have the Player class only consist of fields holding a player's statistics and the corresponding getters and setters and have a separate Statistics class which uses a Player object to generate a result for a single player's at bat or just have a Player object which has one public method (other than getters and setters) which generates a result for a single players at bat? I think the Law of Demeter will not be violated in either case. The logic of the code will be the same in either case but in the first case you will have to use getters to access all of the Player data which I think makes the code a little harder to read, but maybe it doesn't for more experienced Java programmers. Does having the Player class hold the player data and have a method which generates an at bat result violate SRP? A related question (at least I think it is related) is there a preferred way to write this program to allow for future modification? What if I want to add new stats to a player? I know I am going to think of new ways to compare and present scenarios. Is writing clean code the main thing to allow for modification? Are there design patterns which would help? Thanks!

[Junilu Lacar]

I don't know of any specific pattern for this particular case. My guideline is more general: keep code clean and code only for what you need today. Sandy Metz puts it nicely with this tweet:

Sandy Metz wrote:Don't write code that guesses the future, arrange code so you can adapt to the future when it arrives.

https://twitter.com/sandimetz/status/441241600077725697

The benefit of doing stepwise refinement and proper TDD is that you get a lot of practice in refactoring your code and adapting it to suit new requirements even with just what you know today.

[Junilu Lacar]

Kendall Ponder wrote:Is it better to have the Player class only consist of fields holding a player's statistics and the corresponding getters and setters and have a separate Statistics class which uses a Player object to generate a result for a single player's at bat or just have a Player object which has one public method (other than getters and setters) which generates a result for a single players at bat? I think the Law of Demeter will not be violated in either case. The logic of the code will be the same in either case but in the first case you will have to use getters to access all of the Player data which I think makes the code a little harder to read, but maybe it doesn't for more experienced Java programmers. Does having the Player class hold the player data and have a method which generates an at bat result violate SRP? A related question (at least I think it is related) is there a preferred way to write this program to allow for future modification? What if I want to add new stats to a player? I know I am going to think of new ways to compare and present scenarios. Is writing clean code the main thing to allow for modification? Are there design patterns which would help? Thanks!

A couple of things that may help you:

Anemic Domain Model - a Player class with only getters and setters is an Anemic object. Martin Fowler (Refactoring) and Eric Evans (Domain Driven Design) are strong advocates for Rich Domain Models. This means that behavior and responsibilities of objects are the main focus of design.

Object orientation is mainly about behaviors and assigning responsibilities. The data that is encapsulated by an object is secondary, IMO, and serves to help the object perform its responsibilities. Without interesting behavior, an object is just a glorified bucket for data. Note that this is not the only way to organize your program logic though. The functional style of programming requires a very different way of thinking and sometimes flips object-oriented concepts entirely around. See http://www.cs.princeton.edu/courses/archive/spr98/cs333/lectures/16/sld001.htm

[Kendall Ponder]

I am half-way through the Test Code book and am getting ready to work through the examples in the CC book. I haven't started the 4 design principle book yet. I want to start applying what I think I have learned now rather than wait until I have finished all of the books. I am going to rewrite the baseball code trying to use what I have learned. I started with a Player class. The SRP of the class is to generate the result of an at bat with the probability of any result depending on the statistics stored in the player object. The code follows. I put the getters/setters at the end. The test code follows this code. The test book may give a better way to test in the second half, but since the method I am testing gives a distribution I don't see how I can avoid testing to see if the answers fall within a given range. Which means a test may fail sometime when nothing is broken which I know is something to avoid. I am not a statistician so I don't have a solid reason for picking the range I did. Also, I have a printPlayer method which I haven't written a test for yet. Should I overide toString instead of having a printPlayer method? Thanks to anyone who is still following this thread.

package module;

public enum AtBatResult {
WALK, SINGLE, DOUBLE, TRIPLE, HOMERUN, OUT
}

package module;

import java.io.Serializable;


public class Player implements Serializable{
	

	/**
	 * 
	 */
	private static final long serialVersionUID = -528563255318771579L;
	private int atBats;
	private int hits;
	private int singles;
	private int doubles;
	private int triples;
	private int homeRuns;
	private int walks;
	private int stolenBases;
	private int caughtStealing;
	
	private int plateAppearances;
	private double singleCutOff;
	private double doubleCutOff;
	private double tripleCutOff;
	private double homeRunCutOff;
	private double walkCutOff;
	private double percentageStolenBases;

	
	

	public Player(int atBats, int hits, int doubles, int triples,
			int homeRuns, int walks, int stolenBases, int caughtStealing) {
		super();
		this.atBats = atBats;
		this.hits = hits;
		this.doubles = doubles;
		this.triples = triples;
		this.homeRuns = homeRuns;
		this.walks = walks;
		this.stolenBases = stolenBases;
		this.caughtStealing = caughtStealing;
		this.singles=hits-doubles-triples-homeRuns;
		
		 this.plateAppearances=atBats+walks;
		 this.singleCutOff=(double)this.singles/this.plateAppearances;
		 this.doubleCutOff=(double)this.doubles/this.plateAppearances+singleCutOff;
		 this.tripleCutOff=(double)this.triples/this.plateAppearances+doubleCutOff;
		 this.homeRunCutOff=(double)this.homeRuns/this.plateAppearances+tripleCutOff;
		 this.walkCutOff=(double)this.walks/this.plateAppearances+homeRunCutOff;
		 this.percentageStolenBases=(double)stolenBases/(stolenBases+caughtStealing);
	}

	public AtBatResult resultOfPlayerAtBat(){
		double atBat=Math.random();
		if(atBat<singleCutOff){
			return AtBatResult.SINGLE;
		}else if (atBat<doubleCutOff){
			return AtBatResult.DOUBLE;
		}else if (atBat<tripleCutOff){
			return AtBatResult.TRIPLE;
		}else if (atBat<homeRunCutOff){
			return AtBatResult.HOMERUN;
		}else if (atBat<walkCutOff){
			return AtBatResult.WALK;
		}else{
			return AtBatResult.OUT;
		}
	}

	public int getPlateAppearances() {
		return plateAppearances;
	}

	public Player (String string) {
		//  returns a Player object with default values
		this.atBats = 550;
		this.hits = 151;
		this.doubles = 27;
		this.triples = 2;
		this.homeRuns = 15;
		this.walks = 50;
		this.stolenBases = 10;
		this.caughtStealing = 5;
		this.singles=hits-doubles-triples-homeRuns;
		
		 this.plateAppearances=atBats+walks;
		 this.singleCutOff=(double)this.singles/this.plateAppearances;
		 this.doubleCutOff=(double)this.doubles/this.plateAppearances+singleCutOff;
		 this.tripleCutOff=(double)this.triples/this.plateAppearances+doubleCutOff;
		 this.homeRunCutOff=(double)this.homeRuns/this.plateAppearances+tripleCutOff;
		 this.walkCutOff=(double)this.walks/this.plateAppearances+homeRunCutOff;
		 this.percentageStolenBases=(double)stolenBases/(stolenBases+caughtStealing);

	}


	
	public Player(Player source){
		atBats = source.atBats;
		hits=source.hits;
		singles=source.singles;
		doubles=source.doubles;
		triples=source.triples;
		homeRuns=source.homeRuns;
		walks=source.walks;
		stolenBases=source.stolenBases;
		caughtStealing=source.caughtStealing;
		
		plateAppearances=source.plateAppearances;
		singleCutOff=source.singleCutOff;
		doubleCutOff=source.doubleCutOff;
		tripleCutOff=source.tripleCutOff;
		homeRunCutOff=source.homeRunCutOff;
		walkCutOff=source.walkCutOff;
		percentageStolenBases=source.percentageStolenBases;
	}

	public void printPlayer( ){
		System.out.println("At Bats  Hits  Singles  Doubles  Triples  Home Runs  Walks  Stolen Bases  Caught Stealing");
		System.out.format("%8d %5d %8d %8d %8d %10d %6d %9d %16d%n",this.getAtBats(),
																	this.getHits(),
																	this.getSingles(),
																	this.getDoubles(),
																	this.getTriples(),
																	this.getHomeRuns(),
																	this.getWalks(),
																	this.getStolenBases(),
																	this.getCaughtStealing());
	
	}
	
	public int getHits() {
		return hits;
	}

	public void setHits(int hits) {
		this.hits = hits;
		this.singles=hits-this.doubles-this.triples-this.homeRuns;

	}

	
	public int getAtBats() {
		return atBats;
	}

	public void setAtBats(int atBats) {
		this.atBats = atBats;
		this.singles=hits-this.doubles-this.triples-this.homeRuns;
}

	public int getSingles() {
		return singles;
	}

	public void setSingles(int singles) {
		this.singles = singles;
		this.singles=hits-this.doubles-this.triples-this.homeRuns;
	}

	public int getDoubles() {
		return doubles;
	}

	public void setDoubles(int doubles) {
		this.doubles = doubles;
		this.singles=hits-this.doubles-this.triples-this.homeRuns;
	}

	public int getTriples() {
		return triples;
	}

	public void setTriples(int triples) {
		this.triples = triples;
		this.singles=hits-this.doubles-this.triples-this.homeRuns;
	}

	public int getHomeRuns() {
		return homeRuns;
	}

	public void setHomeRuns(int homeRuns) {
		this.homeRuns = homeRuns;
		this.singles=hits-this.doubles-this.triples-this.homeRuns;
	}

	public int getWalks() {
		return walks;
	}

	public void setWalks(int walks) {
		this.walks = walks;
		this.singles=hits-this.doubles-this.triples-this.homeRuns;
	}

	public int getStolenBases() {
		return stolenBases;
	}

	public void setStolenBases(int stolenBases) {
		this.stolenBases = stolenBases;
	}

	public int getCaughtStealing() {
		return caughtStealing;
	}

	public void setCaughtStealing(int caughtStealing) {
		this.caughtStealing = caughtStealing;
	}

	public double getPercentageStolenBases() {
		return percentageStolenBases;
	}

}

Here is the test code. I know I could break it up into 5 separate tests but the code would take 5 times as long to get the same results and I think the 5 asserts combined accomplish the single task of testing the distribution of the Player class's resultOfPlayerAtBat method.

package module;

import static org.junit.Assert.*;

import org.junit.Test;

public class PlayerTest {

	@Test
	public void playerAtBatResultsWithinTenPercentOfExpectedValues() {
		Player player = new Player("Default");
		int [] arrayToStoreAtBatResults = new int[6];
		double iterations = 100_000;
		double acceptableRange = 0.05;
		
		for(int i=0;i<iterations;i++){
			AtBatResult nextResultOfPlayerAtBat = player.resultOfPlayerAtBat();
			arrayToStoreAtBatResults[nextResultOfPlayerAtBat.ordinal()]++;
		}
		
		double percentageOfWalks = arrayToStoreAtBatResults[0]/iterations;
		double expectedPercentageOfWalks=(double) player.getWalks()/player.getPlateAppearances();
		double percentageOfSingles = arrayToStoreAtBatResults[1]/iterations;
		double expectedPercentageOfSingles = (double) player.getSingles()/player.getPlateAppearances();
		double percentageOfDoubles = arrayToStoreAtBatResults[2]/iterations;
		double expectedPercentageOfDoubles = (double) player.getDoubles()/player.getPlateAppearances();
		double percentageOfTriples = arrayToStoreAtBatResults[3]/iterations;
		double expectedPercentageOfTriples = (double) player.getTriples()/player.getPlateAppearances();
		double percentageOfHomeRuns = arrayToStoreAtBatResults[4]/iterations;
		double expectedPercentageOfHomeRuns = (double) player.getHomeRuns()/player.getPlateAppearances();
		
		assertTrue(Math.abs(expectedPercentageOfWalks-percentageOfWalks)<acceptableRange);
		assertTrue(Math.abs(expectedPercentageOfSingles-percentageOfSingles)<acceptableRange);
		assertTrue(Math.abs(expectedPercentageOfDoubles-percentageOfDoubles)<acceptableRange);
		assertTrue(Math.abs(expectedPercentageOfTriples-percentageOfTriples)<acceptableRange);
		assertTrue(Math.abs(expectedPercentageOfHomeRuns-percentageOfHomeRuns)<acceptableRange);
		
		}

}

[Winston Gutkowski]

Kendall Ponder wrote:I appreciate the amount of time you (and others) have spent looking at the code...

No probs, It's what we live for.

I certainly like Junilu's idea of viewing the bases as a Queue - although I don't think that a SquashableQueue (or possibly a SparseQueue) class has been written yet.

I'm afraid I've only given this thread a cursory glance (it's quite long), so excuse me if it's already been said - but I wonder if you're making full use of your Enums.

Like Junilu, I think your AtBatResult one could certainly be expanded to include things like GroundOut, FlyOut etc, but I also think it could have methods to do things like update stats (because we ALL know how baseball bums love their stats ). Exactly how you deal with things like "grounded into double-play" or registering a successful squeeze I'm not sure, but they're fun things to think about.

IMO, enums are still vastly underused - They are fully-fledged Java objects (even if they are singletons), and they can do all sorts of thing like implement Interfaces and override methods. It just takes a bit of thought.

But then, doesn't everything...?

Great thread. I'll be interested to see how you get on. And good luck.

Winston

Edit: Oops - just noticed I revived this thread. Ah well...

[Kendall Ponder]

I agree with you about Enums. Ritchie Campbell worked a lot with me on just that and if you want to see the results on another thread go here.. I just got a new job about a month ago doing JavaEE programing so I haven't done much with the baseball program lately.