Problem
I wrote this code to read text of a file and use it to initialize objects and place them in a multi-tree data structure. The multi-tree has the base node as an object called theTree, which has an ArrayList that contains a bunch of objects called Party. Each Party has an ArrayList that contains a Creature. Each creature has 3 ArrayLists: artifacts, treasures, and jobs, which contain items of those types.
This is a sample of the .txt I’m scanning:
// Parties format: // p:<index>:<name> p : 10000 : Crowd p : 10001 : Band ... // Creatures format: // c:<index>:<type>:<name>:<party>:<empathy>:<fear>:<carrying capacity>[:<age>:<height>:<weight>] c : 20000 : Gnome : Rupert : 10000 : 91 : 30 : 149 : 176.73 : 206.23 : 31.15 c : 20001 : Magician : Delmar : 10000 : 49 : 31 : 223 : 226.37 : 181.93 : 438.56 ... // Treasures format: // t:<index>:<type>:<creature>:<weight>:<value> // creature = 0 means noone is carrying that treasure at the moment t : 30000 : Marks : 20000 : 291.8 : 82 t : 30001 : Chest : 20001 : 82.8 : 66 ... // Artifacts format: // a:<index>:<type>:<creature>[:<name>] a : 40000 : Stone : 20000 : Chrysoprase a : 40001 : Stone : 20000 : Onyx ... // Jobs for creatures // measure time in seconds // j:<index>:<name>:<creature index>:<time>[:<required artifact:type>:<number>]* j : 50000 : Get Help : 20000 : 2.00 : Stone : 0 : Potions : 2 : Wands : 1 : Weapons : 1 j : 50001 : Swing : 20000 : 8.00 : Stone : 0 : Potions : 2 : Wands : 1 : Weapons : 2
Here is what I wrote to scan the .txt and send the gathered information to the proper constructors. As you can see I utilize a HashMap to store each object in a spot associated with its index. Each object that belongs to it has a value that matches that index. Creatures have an attribute called party. Its party is equal to the index of what party it belongs to. Treasures, Artifacts, and Jobs have a similar field called creature.
public static void readFile(){
String [] param;
param = new String [30];
int findParty;
int findCreature;
char x;
int u;//used to determine what constructor to call in case some data is missing
//HashMap used for an easy way to reference objects during instantiation
HashMap< Integer, Assets > gameAssets = new HashMap< Integer, Assets >();
while ( input.hasNextLine () ) {
String line = input.nextLine().trim();
Scanner getStat = new Scanner(line).useDelimiter("\s*:\s*");
if ( line.length()== 0 || line.charAt(0) == '/' ) continue;
while ( getStat.hasNext() ) {
u = 0;
x = getStat.next().charAt(0);
switch ( x ) {
case 'p' :
for ( int i = 0; i<2; i++){
if (getStat.hasNext() ){
param [i] = getStat.next();
}
}
Party newParty = new Party( param );
SorcerersCave.theCave.addParty( newParty );
gameAssets.put( newParty.getIndex(), newParty );
continue;
case 'c' :
Creature newCreature;
while ( getStat.hasNext() ){
param [u] = getStat.next();
u++;
}
if (u == 7){
newCreature = new Creature ( param [0], param [1], param[2], param [3], param[4], param [5], param [6]);
}else {
newCreature = new Creature ( param );
}
findParty = ( newCreature.getParty() );// == index of parent Node in HashMap
if (findParty == 0 ) {
SorcerersCave.theCave.addCreature( newCreature );
}else {
((Party)gameAssets.get( findParty )).addMember( newCreature );
gameAssets.put( newCreature .getIndex(), newCreature );
}
continue;
case 't' :
for ( int i = 0; i<5; i++){
param [i] = getStat.next();
}
Treasure newTreasure = new Treasure ( param );
findCreature = newTreasure.getCreature();
if ( findCreature == 0 ) {
SorcerersCave.theCave.addTreasure( newTreasure );
} else {
((Creature)gameAssets.get( findCreature )).addItem( newTreasure );
gameAssets.put( newTreasure.getIndex(), newTreasure );
}
continue;
case 'a' :
while ( getStat.hasNext() ){
param [u] = getStat.next();
u++;
}
if ( u == 4 ) {
Artifact newArtifact = new Artifact( param );
findCreature = newArtifact.getCreature();
((Creature)gameAssets.get( findCreature )).addArtifact( newArtifact );
gameAssets.put( newArtifact.getIndex(), newArtifact );
} else {
Artifact newArtifact = new Artifact ( param [0], param [ 1 ], param[ 2 ]);
findCreature = newArtifact.getCreature();
((Creature)gameAssets.get( findCreature )).addArtifact( newArtifact );
gameAssets.put( newArtifact.getIndex(), newArtifact );
}
continue;
case 'j' :
while ( getStat.hasNext() ) {
param[u] = getStat.next();
u++;
}
Job newJob = new Job ( param,( Creature )(gameAssets.get( Integer.parseInt( param [2] ))));
SorcerersCave.theCave.addJob( newJob );
findCreature = newJob.getCreature();
((Creature)gameAssets.get( findCreature )).addJob( newJob );
newJob.target = ( Creature )(gameAssets.get( findCreature ) );
GameInterface.jobHeight = GameInterface.jobHeight + 1;
}
}
}
input.close();
}
I turned this in last weekend, got a good grade, but looking back on this method I’m not very proud of it. It’s ugly, complicated, and the least readable method I have ever written. For my first time performing a task like this it works, but I would never show this off.
Solution
This could benefit from breaking the code down into a number of separate methods to improve readability, however the issue that you will find with code like this is that you end up with lots of parameters on each method to pass the data around.
One effective way to deal with this is through a combination of the Extract Class and Extract Method refactorings as follows:
- Extract the
readFilefunction into a separate class, making it a public method on the new class, and having existing callreadFileon the new class. - Start using Extract Method to break down this large method into smaller methods (by introducing private methods in the new class). When extracting the methods, instead of passing variables into the method as a parameter, move the variable to the class level so that it can be shared amongst methods without using parameters.
By doing this, it will become very simple to aggressively apply Extract Method to break the code down into smaller pieces and make it more readable. Furthermore, by encapsulating the code for reading a file into a separate class, it will clean up the code that calls this function.
First off, you should use a more sane indentation style. The line where u is set to 0 is just crazy :-). I prefer The One True Brace Style.
Beyond that, the code could benefit from breaking the things that takes place in the switch clause up using method calls:
switch(x) {
case 'p':
doSomething();
continue;
case 'c':
doSomethingElse();
continue;
[...]