Refactoring Log

  Legend:

testing

Unit Test

coding

Source Changes to make the tests run, remove bugs etc.

refactoring

Refactorings

thinking

saved states and thoughts

The log file written wile refactoring emw.metaworks.DBServerImpl:

testing

Unit Test testSetup for instantiating DBServerImpl  

coding

using finalize instead of shutdown as this does not exit the JVM via System.exit(0)

testing

testGetInstance for getting instance of DBServerImpl

coding

finalize also has to unbind server from RMI-Registry, as multiple instances without quitting the virtual machine, occupying the same socket port

refactoring

renamed bindString to bindName and making it a global variable

refactoring

made port a global variable

coding

bindString and port are used in finalize() to unbind the server

refactoring

splitting method finalize() into two methods - disconnect() and unbind()

coding

as DBServerImpl is designed for singleton use, a method to unInstance it is added to the finalize method

coding

for preventing finalize() to be called twice (a second time by the garbage collector) a finalized state variable was introduced,

coding

unbinding only if not called from a local VM, i.e. if it was not bound to a separate address

refactoring

accessor for bindName was added

coding

finalize() is extended to act differently on RMI-bound and local servers

coding

due to the impossibility of removing an installed registry, the exception the createRegistry() call creates is silently ignored

coding

commenting out System.exit(0) as this causes the test environment to exit as well

coding

new refactoring is to extract the methods needed for setting up the rmi binding from startServer() it is called bindServer()  

refactoring

as:

      try {
        port=new Integer(RuntimeProperties.u().getProperty("DB_PORT")).intValue();

      } catch(Exception e) {}

is duplicated very often throughout the class, it is extracted to an own method, getPropertyInt

refactoring

to catch the exception an default value must be supplied

testing

Unit Test testGetPropertyInt is written and runs from the first run

coding

bindServer() returns now a boolean value instead of using System.exit(0) when failures occur, this boolean value is stored in the variable isBound

thinking

state 2 saved

coding

remove a lot of commented out and testXX() code (409 lines, of 1400) to a separate file DBServerImpl.java.removed, to make the class more comprehensible  

testing

rerun tests (problems with reloading actual class files in the Swing Version, switching to text version, test results should also be written to a file to not interfere with outputs from tested class)

thinking

state 3 saved

refactoring

replace other uses for getPropertyInt in constructor, moved try clause to not longer watch these lines

refactoring

added failure warning for getPropertyInt

testing

tests run, the testSetup and testGetInstance are commented out because they consume to much time (connecting to db)

refactoring

moved all initialization (e.g. reading properties file and setting variables according to properties) to method initialize(), to collect these things at one place

testing

test getInstance broke, as bindName is now created regardless of, bound or not bound, changing test to check to isBound flag

testing

test commented in again ;)

refactoring

as start Server does only contains delegation it is inlined into the constructor

refactoring

looking at dbConnect reveals that the JDBC driver is looked up each time a connect try is done, move it out of the loop

thinking

considering dbConnect to return a Connection object instead of boolean result, but delayed that decision

refactoring

a test is added to check RMI stuff, binding and unbinding the server, checking if the correct object was bound and performing a method call on it

refactoring

moved getPropertyInt directly to RuntimeProperities

testing

tests run correctly after adaption to moved reference

refactoring

setDescriptors() and getDescriptors don't require testing methods as they only delegate their work  

thinking

as the different databases differ in their syntax, I think about creating a DatabaseFormat class, which gets the responsibility of formating the raw query/update data to correct SQL sentences

thinking

because the syntax order differs as well not only the symbols, the formation of a template method with appropriate calls to the DatabaseFormat is not possible

thinking

the class should cover, SQL terminal symbol names, formation of sentences of the structured query language and additional formating of String and Date objects

thinking

either the complete functionality of querying the Kriterium or Data objects to form SQL sentences is moved to the DatabaseFormat or its Methods are only called with the extracted data

testing

test for DBServerImpl.createFrom is created

testing

an test table within the database is needed, the following format is used  

Table employee:

id number(10) // primary key
name varchar(20)
birthday date
salary number(7,2)
age number(3)
skilled boolean
manager number(10) // foreign key to employee

testing

getConn() is implemented for getting a valid connection to the database used by the TestCase; has to be removed later

testing

a testCreateTable is created to create and drop a table

thinking

writing tests consumes a lot of time but one learns much about the own code (in particular its shortcomings), the testing framework and in this case the database

testing

testCreateTable succeeds

testing

a test for creating a DataDescriptor for the testTable

testing

a test for creating a Data object  

coding

added convenient methods for setting and querying data attributes with names rather than only with id's

testing

test for creating a Kriterium object

coding

added a method for setting the operator and the comparison value in Kriterium together

coding

added methods for setting and getting operators by name (Kriterium) and for modification check also by name (Data)

testing

test runs

refactoring

using ExtractMethod on the testCreateTable method to split the creation and removal of the table into two separate entities  

testing

a test for inserting, querying and remove a row into the table is created

testing

as creating a connection to the database within setUp and closing it within tearDown is too costly (regarding the time consumed), a method getStatement is added that provides a valid statement for the database

coding

found an error in DBServerImpl regarding the storing of boolean values in the database, corrected, tests run

testing

creating a test queryData();

thinking

inserting and deleting don't require the database name, otherwise querying does, this produced an error when running the test

coding

added database name e-db to createDescriptor

testing

all tests are now composed of small creation methods

thinking

there are problems creating a database-qualified table within the restrictions of the database, but a non qualified table produces errors when querying

coding

correction of the error when querying

coding

as assertEquals does not cover the comparison of two Data objects, a method named equalsData is added to the Data class

thinking

creating a Data with java.lang.Float is converted to java.lang.Double when stored in the Database even with type float

thinking

type FLOAT in Progress is mapped to Double in java

thinking

a NULL value written in the database is returned as an 0 value when using a integer, this problem seems quite serious as all values seem to be converted to basic values of the Object Types,

coding

problem solved using ResultSet.wasNull() in the DBServerImpl query() method to test for null values after retrieving the objects

thinking

perhaps this must be moved to the DatabaseFormat class

testing

wrote test method for createFrom()

testing

test failed, because a space was missing, corrected the expectation an rerun successfully

testing

added second metadata (foreign key reference employee.manager -> employee.id) for more testing of createFrom

testing

after some error searching the metadata creation and tests run

refactoring

removed some duplicate Code from createFrom, and replaced String with StringBuffer

refactoring

an emw.metaworks.server.DatabaseFormat class is created  

refactoring

createFrom is moved there

testing

testCreateFrom is adapted to the new source of the method, testing is successful

testing

testCreateWhere missed its first run by some spaces and missing parentheses

testing

added second Kriterium to be tested with createWhere

testing

found bug representing boolean values in the database query, but as this one is deeply buried in the code of the database servers createWhere method, it seems as if refactoring has to go first

refactoring

as DBServerImpl relies on ObjectToOracle for formatting Object values, the functionality that resides there has to be moved to the DatabaseFormat class, the references in the DBServerImpl has to be modified as well  

testing

test still run after moving ObjectToOracle.getOra7String() to DatabaseFormat;

coding

modified the moved method to correctly respond to boolean values

refactoring

replaced now incorrect name getOra7String() to formatObject()

refactoring

I would like to overload formatObject for formating the different types but the method is called only with Object objects and not with type specific object

coding

by the way removed a hack associated with wrong formatting of date objects regarding to progress database, as well as the hard wired formatting of boolean values

testing

test still run fine  

refactoring

extracted huge switch statement into new method getExpression()

refactoring

extracted methods for formating NULL values, like and begin expressions to DatabaseFormat

coding

introduced method getOperatorLabel in DatabaseFormat

coding

removed 43 lines of code from the switch statement and replaced it with just one line of code:

 default: 
    return DatabaseFormat.getOperatorLabel(operator) + 
           DatabaseFormat.formatObject(obj);

testing

after clearing some spaces and cases (upcase in this case) the tests run fine

thinking

saved state 5 of DBServerImpl.java

thinking

next method to be tampered with is DBServerImpl.createJoin()

testing

testCreated for createJoin

thinking

as the test encounters problems with not set databases (e.g. null.employee.manager = null.manager.id) i write a method encapsulating the creation of such structures which produces correct results

coding

first some cosmetics, ordering code and using Stringbuffer instead of String

testing

the tests showed up an error I produced when I introduced StringBuffer which did not show up at testCreateJoin() but rather at testInsertQueryAndDelete()

testing

tests run fine now

refactoring

as createJoin has useless parameters they should be removed createJoin(int attributeCount, DataDescriptor dd, Vector fromTableVector) to createJoin(DataDescriptor dd)

testing

after adapting to the new parameter list the tests run correctly

thinking

the next method to be looked at createQuery is small enough and well structured, it doesn't have to be refactored, but rather moved to DatabaseFormat

refactoring

as the query() method that uses all the previously refactored methods is next, I think it is a good time to move the appropriate methods to DatabaseFormat (createWhere(), getExpression(), createQualifiedField(), createJoin(), createQuery()

testing

after moving the methods and adapting the references all test still run fine and even faster as no longer an instance of DBServerImpl is required to perform the tests which does automatically connect to the database

thinking

DBServerImpl shrank by another 100 lines of code  

refactoring

the next method to be refactored is DBServerImpl.query() which is a quite heavy one and which yearns for refactoring

refactoring

extracted all code regarding the creation of the select statement from the query() method into the createSelect() method

thinking

saved state 6

refactoring

refactored the createSelect() method

testing

when refactoring I introduced the error of using the String "table" instead of the variable table, the test failed, and because the method was already refactored I only had to change the code in one place

testing

added some more test data and found another bug regarding the not equals operator in Progress, corrected it as it only appears once now in DatabaseFormat.getOperatorLabel, now all tests run again

testing

added more test data for querying referenced tables by using the "Manager" metadata

refactoring

move the test of an available database statement to an extracted method named getStatement()

thinking

the notion of the external definition if a table has a change date field, should be internalized into the appropriate metadata structure, but not now :)

refactoring

as the code structure (object!=null) ? object.toString():""; appears much to often, it is replaced with calls to the method notNullString(object)

refactoring

it is moved together with andExpressions() to DatabaseFormat as it is a kind of formatting and its more often used there, both methods are introduced at all necessary places

testing

tests are still ok (I'm astonished ;)  

refactoring

I am even more surprised, when pondering about problems arising from the temporary local variable (boolean array) isPartOfQuery for the elements of the Kriterium object, I realized that applying the Replace Temp with Query (120) refactoring could not only be helpful but also allowed it to restructure the createSelect method as it is the method producing the boolean array.

refactoring

So I extracted the code needed for isPartOfQuery() from createSelect() !! code and had a very strange moment when things began to fall in place and the code really communicated me that this was the right thing to do. Suddenly I was able to cut the createSelect() method to its basic task, creating a select part for the fields of the Kriterium object that have to be retrieved from the database. The work of determining which are those fields, doesn't have to do anything with this task but was mingled before in createSelect().

refactoring

In the end I had two methods which were well structured, small and which concentrated on their basic task.

thinking

I think that is the real gain from refactoring. On the way to restructure your code you apply a refactoring and that starts a chain reaction which results in a not previously anticipated but convincingly superb code, that does still all the things it should but is much more focused and clear. Not mentioning the removal of tons of duplicate code by working through it. I don't think one would come to this conclusion that easily when trying to imagine it as an upfront design.

testing

and all test still run fine ;)

thinking

I must say when experiencing this moment of clarity, I have to agree with MARTIN FOWLER who states that refactoring is a lot of fun. In my humble opinion refactoring has even more gains that programming from scratch, because not only you take a bad thing and turn it into a high quality piece of code without much thinking about but also you can learn much more about the system in general and in detail, about programming style and design and about the inhoerent structure which is needed to solve a problem, but which is ususally disguised by many lines of bad code.

thinking

saved state 7

refactoring

extracted the retrieval of the Data objects to an own method retrieveData()

testing

tests run without problems  

refactoring

although I'd like to refactor the query() method further, I stop here because further refactoring would have to introduce change date aware metadate and to deal with multiple return types for each part of the select statement which is created separately. As I can't predict if those parts are concatenated everytime in the same way without changes, I also don't add a method for creating the whole select statement string at once by calling the creation methods itself.

thinking

now I'm done with the first half of the DBServerImpl class which lost almost one third of its code.

coding

changed createQualifiedField to createQualifiedName covering now all possible combination of valid parameters to create a qualified reference symbol.

testing

test confirmed

refactoring

refactored delete by using Extract Method to move a part of the code into createDeleteStatement wich can be transferred to DatabaseFormat, and some of it into doCommit() which deals with committing the changes made be delete() and deleteAll()

testing

I'd like to add a test for tables with multiple oid's, but this would require to get the Data class to make the oid values contained available using one method

coding

changed the Data class

testing

modified the tests to work with two oids and it worked

refactoring

used MoveMethod to move createDeleteStatement to DatabaseFormat

testing

tests failed due to a wrong String constant in DatabaseFormat

testing

tests run

refactoring

deleteAll skipped as it didn't require refactoring

thinking

fast forward to line 597 of 975, as the methods betwenn were already refactored, saved state 8

thinking

next method to be refactored is a query method that executes a SQL string directly and returns the results as Object[][]

testing

first writing test

testing

the old problem of java.util.Date stored and java.sql.Date retrieved showed up again, now I set the Date values as instances of java.sql.Date but this problem is in need of further evalutation

refactoring

refactoring query() by introducing getStatement()

refactoring

replaced variable col_count by column_count

refactoring

changed Vector of Vector to Vector of Object[] as this lowers the conversion efforts

testing

tests run fine

thinking

thought about using ExtractMethod to extract the functionality of converting a Vector of Arrays to an own method but as this is not that much code and as it is not duplicated by now I leave it in place

refactoring

removed empty method initTableDescriptor() as it is used nowhere

thinking

next method is updateData which is also quite large and the last method of such importance and size, its now approximately 210 line long :(

testing

a test for the inserting functionality of update() ran all the time by now and it is expanded now to run a second time to update the values

thinking

the date object seems to have a failure in design, updating data values occurs only on Data values that were read from the database, not for newly created ones

thinking

it would be better to distinguish the methods of updating and inserting or doing a previous test if the row of data already exists within the database (which costs a lot of time btw)

testing

actually I'm marking the data values to be updated (instead of being inserted as coming from the database), and I have to supply a nonexisting (i.e. null) Date value to its constructor, meaning there is no last_change field in the database

testing

modified in that way that the test does first insertion of the data values and second updating with changed values, which are subsequently read for verification from the database

testing

test runs now

refactoring

added method getExpression(field,operator,object) to DatabaseFormat as this is used everywhere instead of getExpression(operator, object)

refactoring

introduced getStatement() into update()

refactoring

moved the code within the outer for loop of update into an extracted method of update(Data,user_oid)

refactoring

extracted a method createUpdateStatement() from the single update() method

testing

tests run

refactoring

replaced weird String adding constructions with calls to concatExpressions(getExpression())

testing

tests threw an exception as I missed the SET statement within the query, corrected

testing

tests run

refactoring

extracted method createOidExpression() from createUpdateStatement() which resembles the code used in delete()

testing

tests run

thinking

btw. multiple oids work fine now as well

refactoring

MoveMethod for createUpdateStatement and createOidExpression to DatabaseFormat

testing

tests run fine ;)

refactoring

replaced duplicate code in DatabaseFormat.delete() with createOidExpression()

testing

tests ok

refactoring

Extracted Method createInsertStatement() from the second part of update()

testing

tests ok

refactoring

Extracted Method supplyOid() for inserting a new, valid oid if it is not exitent. As this method, i.e. the way new oids are created, is project specific, it should be overwritten as needed.

testing

tests run

coding

introduced the concatExpressions() instead of String concatenation in createInsertStatement()

coding

added method getOidArray to the class Data as this covers most of the ugly workarounds which were introduced by Data.getOids()

refactoring

Moved Method createInsertStatement() to DatabaseFormat to sit around with its siblings

testing

tests still run

refactoring

commented out method DBServerImpl.getLfdNr() as this is specific to a project (much like supplyOids), which should be overwritten by an subclass of DBServerImpl. Therefore it should be marked abstract.

thinking

I am done with the DBServerImpl class !!

thinking

the class shrank from 1235 loc (DBServerImpl.java.0) to 648 loc, with 278 loc moved to the class DatabaseFormat, and produced 220 lines of this report ;)

thinking

ideas for refactoring report, unfortunately they appeared to late to me to be used in a sensible way: timestamps, perhaps durations of the steps, number and duration of the tests (I must acknowledge, that I've written not enough tests but rather modified the testfile sometimes for another type of test (e.g. 2 oid's instead of one). My 10 tests run now in less thant 36 seconds which is quite long :( but due to the database connection inevitable.

refactoring

TODO: as the extracted methods evolved during the refactoring and were not available from the beginning the class has to be reexamined which pieces of code would benefit from the application of the extracted methods (e.g. to remove code duplication)

coding

added DatabaseFormat.createQualifiedName() and DatabaseFormat.concatExpressions() to DBServerImpl.createSelect()

refactoring

change static calls in DatabaseFormat into public method class to a singleton pattern instance of this class, which should be overwritten for the database specific formatting (e.g. ProgressFormat and OracleFormat)

coding

added getInstance() to DatabaseFormat which returns a single instance

coding

enhanced getInstance to work with class names to return also those classes which are descendants of DatabaseFormat, internally the successfully created instances are stored within a hashtable

testing

added tests for getInstance() which run fine

refactoring

DBServerImpl should have an config file parameter that denotes the database, an could be used to get the appropriate instance (much like getDriver())

coding

added config file parameter named DBMS_NAME

coding

added variable databaseFormat which substitutes static call to DatabaseFormat, and which is set by using DatabaseFormat.getInstance(DBMS_NAME)

testing

tests still run perfectly

thinking

as the current DatabaseFormat covers the syntax and specialities of the Progress DBMS, its functionality should be partially moved to a subclass named ProgressFormat

refactoring

Using the Extract Subclass refactoring to create the appropriate subclass moving the necessary parts of the methods to the subclass

refactoring

started with formatObject() which does special formating for the Boolean and Date values in ProgressFormat

testing

tests run

refactoring

next in row is createUpdateStatement() whose results' syntax differs to much in Progress and Oracle

refactoring

the constant DELETE is also moved to ProgressFormat, because it covers a different syntax as well

testing

all tests stills run

thinking

I think all of the methods moved to ProgressFormat should cover the differences to DatabaseFormat

thinking

now another subclass named OracleFormat can easily be created, (it is instantiated automatically by calling DatabaseFormat.getInstance("Oracle")), the same methods have to be moved there but containing other code

testing

the OracleFormat should be tested as well but unfortunately I don't have access to an Oracle System right now, a new test class must be created as well, as the assertions in the current test class clash with the syntax produced by OracleFormat, ideally this would be accomplished by subclassing DBServerImplTest to DBServerImplTestOracle

thinking

DBServerImpl should be overwritten on a per project basis, it has to be reexamined which parts are still project and (hopefully none) database dependent