cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ran Tavory (JIRA)" <j...@apache.org>
Subject [jira] Created: (CASSANDRA-741) Refactor for testability: Make class DatabaseDescriptor a real class with member methods, and non-static methods
Date Tue, 26 Jan 2010 07:44:34 GMT
Refactor for testability: Make class DatabaseDescriptor a real class with member methods, and
non-static methods
----------------------------------------------------------------------------------------------------------------

                 Key: CASSANDRA-741
                 URL: https://issues.apache.org/jira/browse/CASSANDRA-741
             Project: Cassandra
          Issue Type: Improvement
          Components: Core
            Reporter: Ran Tavory
             Fix For: 0.6


I've stumbled across this issue when working on CASSANDRA-740 (Create InProcessCassandraServer
for unit tests).

I think the current way of how DatabaseDescriptor is implemented and used isn't great for
testability perspective, readability and extensibility, here's why - 

The class has a giant static{} block that initializes all it's static data members. This static
block is hard wired to read a file named storage-config.xml form a system defined property
path.

 System.getProperty("storage-config") + File.separator + "storage-conf.xml";

This isn't great for testing. What this means is that tests need to create the folder and
put the file there (see scratch code inline the issue CASSANDRA-740). 
Tests would rather be able to configure the class from an InputStream or something like that.
However, the fact that the class initializes itself autonomously isn't helping - a driver
of the class cannot tell it to initialize itself from a different input stream. dependency
injection is a typical solution to these cases.
If the class had a constructor that accepts an InputStream (and an empty one that acts at
the default, reading the storage-config.xml from the file system) that would be nicer for
tests.
It would actually be nicer not only for tests, but also for readability and extensibility
purposes - it would make all dependencies explicit (see more on this below) and it would be
possible to extend the class by inheriting and overriding some of its methods (when everything
is static it's impossible.)

Here's a mockup of my suggestion:

public class DatabaseDescriptor
{
  public DatabaseDescriptor() {
    this(new FileInputStream(System.getProperty("storage-config") + File.separator + "storage-conf.xml");
  }

  public DatabaseDescriptor(InputStream input) {
    // read xml from input
  }

...
}

However, implementing what's suggested above, although the change is rather trivial, means
changing all references to DatabaseDescriptor application wide... and there are more than
200 of those...
It usually looks like this:

public class RingCache
{
    final private int port_=DatabaseDescriptor.getThriftPort();
    final private static IPartitioner partitioner_ = DatabaseDescriptor.getPartitioner();


The above code would have to be changed to:

public class RingCache
{
  final private int port_;
  final private static IPartitioner partitioner_;

  public RingCache(DatabaseDescriptor descriptor) {
    port_ = descriptor.getThriftPort();
    partitioner_ = descriptor.getPartitioner();


This means making the RingCache depend *explicitly* on DatabaseDescriptor since it requires
it in its constructor (and not implicitly, via a static method) so this is good for being
able to test the RingCache in a unit test and is also great for readability - no hidden dependency
loops.

Actually, since the RingCache depends only on the thrift port and the partitioner, but not
an the entire database descriptor, it'll make even more sense to pass only the two in the
constructor. This would again make unit tests easier and code easier to understand (you don't
have to wonder "what is it in the database descriptor that I need to have in order to make
the RingCache work?")

public class RingCache
{
  final private int port_;
  final private static IPartitioner partitioner_;

  public RingCache(int thriftPort, IPartitioner partitioner) {
    port_ = thriftPort;
    partitioner_ = partitioner;



The suggested change is a code quality improvement and testability improvement and should
not affect the logic of the app.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message