Sensey refactoring
More practice to improve skills
Hello, spectator. Today we will make some refactoring together. There is one interesting library called Sensey. You can find it here.
Foreword
The library project originally looked like this.
First of all we see, that there is somehow appears .idea folder. It isn’t a project folder it’s just IDE’s folder, and it depends on your local machine IDE settings. So it useless for keeping itself in remote repository and we need to delete it. And it had to be the first step. Unfortunately I did it only after few commits, so we will get to that later.
Tests
Before we will make some changes, we need to see, whether there are tests or not. In ideal situation it would be great, if there was not some random tests, but tests that cover our refactoring zone. I know, I know that most of our changes are small and not interesting, but it is just a sample of refactoring process and main point is to show it in details. So we go to test folder and…There is no tests. Okay, let’s write them.
Remember that our goal isn’t to write full coverage tests that will check every single situation, that is possible in the code. I think it will be to much. But instead of that we can write a base pack of tests, that will check just all common cases. It is more than smoke tests, but less then full coverage. We will use Mockito for testing, as it provides very convenient way for creating mocks, stubs and verifying their behavior.
All is good, but there are two problems:
1. As we see Sensey.class contains reference to Context, so it will be pretty hard to write unit tests. In this case we will use Robolectric framework, that provides mechanism for writing Android UI tests without using emulator. And our case is ideally fit our need. Indeed for Sensey.class we don’t need an emulator to run test, instead of that we need just mocking mechanism for android classes. And Robolectric do it for us.
2. In OrientationDetector.class we want to test its method onSensorChanged. It’s seems like other onSensorChanged methods for other ‘Detector’ classes. But we can not use Mockito in this class. Try to watch on this snippet and guess why
@Override
public void onSensorChanged(SensorEvent event) {
if (event.sensor.getType() == Sensor.TYPE_ACCELEROMETER) {
mGravity = event.values;
}
if (event.sensor.getType() == Sensor.TYPE_MAGNETIC_FIELD) {
mGeomagnetic = event.values;
}
if (mGravity != null && mGeomagnetic != null) {
float R[] = new float[9];
float I[] = new float[9];
boolean success = SensorManager.getRotationMatrix(R, I, mGravity, mGeomagnetic);
if (success) {
float orientationData[] = new float[3];
SensorManager.getOrientation(R, orientationData);
averagePitch = addValue(orientationData[1], pitches);
averageRoll = addValue(orientationData[2], rolls);
orientation = calculateOrientation();
switch (orientation) {
case ORIENTATION_LANDSCAPE:
orientationListener.onRightSideUp();
break;
case ORIENTATION_LANDSCAPE_REVERSE:
orientationListener.onLeftSideUp();
break;
case ORIENTATION_PORTRAIT:
orientationListener.onTopSideUp();
break;
case ORIENTATION_PORTRAIT_REVERSE:
orientationListener.onBottomSideUp();
break;
default:
// do nothing
break;
}
}
}
}The answer is because of SensorManager.getRotationMatrix and SensorManager.getOrientation static methods. These functions use internal android state, that can not be mocked somehow through Mockito. And Robolectric again solves our problems.
In the end of writing tests we run them with coverage and see next results.
Of course it’s not perfect tests, but they cover some significant parts of code, that we will refactor, and it is good.
In the end of this step project looks like this
Temporary files and folders
Next step relates to the beginning of post and it is about temporary .idea folder that belongs only to your local copy of project. As it said in the documentation of IntelliJ Idea:
Project Settings
Project settings are stored with each specific project as a set of xml files under the .idea folder. If you specify the default project settings, these settings will be automatically used for each newly created project.
The settings that pertain to a project, are marked with the icon in the Settings/Preferences dialog.
So it’s needed to delete .idea folder, because else after cloning or pulling changes you always will see some xml files as changed, and you will need always revert or commit them to pulling new changes, or you will need always more accurately choose files to commit, or elsewhere you will pollute repository with your local project settings. So those types of folders in repository are pain.
To delete this folder we need to add it to the special list of ignored files or folders in special .gitignore file and then delete this folder and commit this changing.
First of all we will check .gitignore file. And there is no problem in it. It contains all needed items. So the problem is that author maybe just forgot to delete this folder. Thus we just delete .idea local folder, commit those changes and push them. That’s all.
In the end of this step project look like this.
Adding hierarchy for ‘sensor’ detectors
First of all we need to take closer look to variety of detectors in the library. At first there were only so called ‘sensor’ detectors, which provide ways to detect some action through sensor of the device (e.g. light or dark right now). When I started to analyze Sensey library and place my changes to it, there were added detectors of different touches (single tap, double tap) and also detector of scaling.
So in common there are 2 types of Detectors: Sensor detectors and Touch detectors. Last are not important for us, but Sensor Detectors are interesting. All of them have the same structure:
public class TemplateSensorDetector {
//Special listener for this type of detectors
private final TemplateListener listener;
final SensorEventListener sensorEventListener =
new SensorEventListener() {
@Override
public void onSensorChanged(SensorEvent sensorEvent) {
//do something specific to detect some interesting stuff
} @Override
public void onAccuracyChanged(Sensor sensor, int i) {
//actually do nothing here
}
}; //Constructor with passing listener here
}
First, every Sensor detector (Flip, Light, Orientation, Proximity and Shake detectors) has no it’s own logic, only specific anonymous implementer of SensorEventListener interface, that do all the work. Second, some of detectors kept fields that are used only inside implementer of SensorEventListener. For example, ShakeDetector. It has mAccel field, that is used only in onSensorChanged method. And it is placed in inheritor of SensorEventListener. As a result detector is a meaningless class, because it is only the holder of SensorEventListener implementer. Thus, instead of having only one class that can detect some pack of events we have 2 classes. One can detect some event and other only keep first one. So we come to conclusion to delete inner class and make every Sensor detector implement SensorEventListener, so our TemplateSensorDetector will look like:
public class TemplateSensorDetector implements SensorEventListener {
//Special listener for this type of detectors
private final TemplateListener listener; //Constructor with passing listener here @Override
public void onSensorChanged(SensorEvent sensorEvent) {
//do something specific to detect some interesting stuff
} @Override
public void onAccuracyChanged(Sensor sensor, int i) {
//actually do nothing here
}
}
As we remember all of implemented on that moment (when post was written) all of detectors don’t use onAccuracyChanged method. So it would be better to have main parent of every sensor detector that for now will have only two declared methods: onSensorChanged, onAccuracyChanged. And every inheritor will choose, which methods it will implement and which will not. It will reduce number of unused methods and duplication. So we create new SensorDetector class:
public abstract class SensorDetector implements SensorEventListener{
@Override
public void onSensorChanged(SensorEvent sensorEvent) {} @Override
public void onAccuracyChanged(Sensor sensor, int i) {}
}
We make it abstract so no one can create instance of it, but only extend it. Now our ordinary sensor detector will look like this:
public class TemplateSensorDetector implements SensorEventListener {
//Special listener for this type of detectors
private final TemplateListener listener; //Constructor with passing listener here @Override
public void onSensorChanged(SensorEvent sensorEvent) {
//do something specific to detect some interesting stuff
}
}
Little Fixes
One of steps of refactoring is fixing some small shortcomings and duplication.
Duplication in constructors
In several detectors there are two constructors, one with appropriate listener and one with additional parameter (e.g. threshold). And code looked like that:
public LightDetector(LightListener listener) {
this.threshold = 3f;
this.listener = listener;
}public LightDetector(float threshold, LightListener listener) {
this.threshold = threshold;
this.listener = listener;
}
So we just remove duplication by recalling bigger constructor with ‘default’ values:
public LightDetector(LightListener listener) {
this(3f, listener);
}public LightDetector(float threshold, LightListener listener) {
this.threshold = threshold;
this.listener = listener;
}
Initializing of some fields in constructors
In Shake detector there were fields that can be initialized through their declaration. But they were initialized in constructor:
private float mAccel;
private float mAccelCurrent;
private float mAccelLast;public ShakeDetector(float threshold, ShakeListener listener) {
mAccel = 0.00f;
mAccelCurrent = SensorManager.GRAVITY_EARTH;
mAccelLast = SensorManager.GRAVITY_EARTH; this.threshold = threshold;
this.listener = listener;
}
Float value already has 0 as default value so we can remove ‘mAccel = 0.00f;’. Other 2 lines with initialization of mAccelCurrent and mAccelLast we can move to their declaration:
private float mAccel;
private float mAccelCurrent = SensorManager.GRAVITY_EARTH;
private float mAccelLast = SensorManager.GRAVITY_EARTH;public ShakeDetector(float threshold, ShakeListener listener) {
this.threshold = threshold;
this.listener = listener;
}
Uniformity
I don’t know why, but through the code there was broken uniformity of declaring floats (in one place it was ‘3f’, in other ‘(float) 3f’, in other ‘3.0f’) and of using thresholds (in all detectors threshold was a float, but in shake detector it was int, but could be a float, too). So we just fast fix this shortcoming.
Duplication in class Sensey
Now when we finished with detectors, let’s move to Sensey class. It is a main class for whole library that provides many entry points. But in general it has two types of methods:
- Start detection
- Stop detection
More interesting for any sensor detectors, all of ‘start’ or ‘stop’ detection methods for them look like (example for ShakeDetector):
private ShakeDetector shakeDetector;public void startShakeDetection(int threshold,
ShakeDetector.ShakeListener shakeListener) { final Sensor sensor = sensorManager.getDefaultSensor(
Sensor.TYPE_ACCELEROMETER); if (sensor != null) {
shakeDetector = new ShakeDetector(threshold, shakeListener);
sensorManager.registerListener(
shakeDetector.sensorEventListener,
sensor,
SensorManager.SENSOR_DELAY_NORMAL);
}
}public void startShakeDetection(
ShakeDetector.ShakeListener shakeListener) { final Sensor sensor = sensorManager.getDefaultSensor(
Sensor.TYPE_ACCELEROMETER); if (sensor != null) {
shakeDetector = new ShakeDetector(shakeListener);
sensorManager.registerListener(
shakeDetector.sensorEventListener,
sensor,
SensorManager.SENSOR_DELAY_NORMAL);
}
}public void stopShakeDetection() {
if (sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER) != null && shakeDetector != null) {
sensorManager.unregisterListener(
shakeDetector.sensorEventListener);
}
}
As you see, we have several problems in this snippet of code, but for us right now main problem is duplication:
- Duplication of ‘start’ methods for same detectors but with different pack of parameters
- Duplication of ‘start’/’stop’ methods for all sensor detectors
So we create a two new public methods called startSensorDetection and stopSensorDetection. First method will accept SensorDetector instance as parameter and also pack of sensor types, which will be tracked by Detector. Second method will accept only instance of SensorDetector to know what exactly detector we need to stop (as with common ‘addListener’, ‘removeListener’ methods):
public void startSensorDetection(
SensorDetector detector, int... sensorTypes) { final Iterable<Sensor> sensors =
convertTypesToSensors(sensorTypes); if (areAllSensorsValid(sensors)) {
registerDetectorForAllSensors(detector, sensors);
}
}public void stopSensorDetection(SensorDetector detector) {
if (detector != null) {
sensorManager.unregisterListener(detector);
}
}private Iterable<Sensor> convertTypesToSensors(int... sensorTypes) {
Collection<Sensor> sensors = new ArrayList<>();
for (int sensorType : sensorTypes) {
sensors.add(sensorManager.getDefaultSensor(sensorType));
}
return sensors;
}private boolean areAllSensorsValid(Iterable<Sensor> sensors) {
for (Sensor sensor : sensors) {
if (sensor == null) {
return false;
}
} return true;
}private void registerDetectorForAllSensors(
SensorDetector detector, Iterable<Sensor> sensors) { for (Sensor sensor: sensors) {
sensorManager.registerListener(detector, sensor,
SensorManager.SENSOR_DELAY_NORMAL);
}
}
We’ve created here three additional methods for providing more readability. Method convertTypesToSensors converting all passed as parameters sensor types to appropriate sensor if it can, using sensorManager.getDefaultSensor. Then method areAllSensorsValid checks that all sensors, we’ve received from instance of SensorManager are valid. In our case they all must be not null. And last method registerDetectorForAllSensors just calls sensorManager.registerListener for all sensors, which will be tracked by instance of SensorDetector passed as parameter. Thus our startSensorDetection method use same algorithm as any ‘start’ method for Shake, Light, Flip and other detectors.
Now all of our ’start’/’stop’ methods for library sensor detectors will look like (example for ShakeDetector):
private ShakeDetector shakeDetector;public void startShakeDetection(int threshold,
ShakeDetector.ShakeListener shakeListener) { shakeDetector = new ShakeDetector(threshold, shakeListener);
startSensorDetection(shakeDetector, Sensor.TYPE_ACCELEROMETER);
}public void startShakeDetection(
ShakeDetector.ShakeListener shakeListener) { shakeDetector = new ShakeDetector(shakeListener);
startSensorDetection(shakeDetector, Sensor.TYPE_ACCELEROMETER);
}public void stopShakeDetection() {
stopSensorDetection(shakeDetector)
}
And after one more step of refactoring:
public void startShakeDetection(
ShakeDetector.ShakeListener shakeListener) {
startShakeDetection(new ShakeDetector(shakeListener));
}public void startShakeDetection(int threshold,
ShakeDetector.ShakeListener shakeListener) {
startShakeDetection(new ShakeDetector(
threshold, shakeListener));
}private void startShakeDetection(ShakeDetector detector) {
shakeDetector = detector;
startSensorDetection(detector, TYPE_ACCELEROMETER);
}public void stopShakeDetection() {
stopSensorDetection(shakeDetector);
}
So, in the end of this big step we currently have:
- Main parent for all sensor detectors
- Deleted unused inner classes
- More uniform code
- Reduced duplication of starting and stopping sensor detection methods by introducing start/stop methods for any instance of SensorDetector and reusing them in each specific entry point.
And in the end of this phase we updated tests according to new usage of SensorDetectors. Now run them. And as we see, all pass.
In the end of this phase code looks like this.
Attaching sensor types to detectors
As for now, if we go through the Sensey class, we will notice that all of sensor detectors starts their detection for already specified list of sensors. Moreover any SensorDetector inheritor checks in onSensorChanged callback, that changed exactly tracked sensor (for more security). So we can assume that, if the user of this library wanted to create its own SensorDetector inheritor, he specified it for only restricted pack of Sensors with types, for example, sensorType1, sensorType2 etc. So he needed always call startSensorDetection(customDetector, sensorType1, sensorType2) and that’s bad, because of duplication. Why we can not specify sensor types only once?
Thus we come to conclusion, that specifying types of sensors is work of each specific inheritor of SensorDetector. So we change our SensorDetector:
public abstract class SensorDetector implements SensorEventListener{
private final int[] sensorTypes; public SensorDetector(int... sensorTypes) {
this.sensorTypes = sensorTypes;
} @Override
public void onSensorChanged(SensorEvent sensorEvent) {
if (isSensorEventBelongsToPluggedTypes(sensorEvent))
onSensorEvent(sensorEvent);
}
} protected void onSensorEvent(SensorEvent sensorEvent) {} @Override
public void onAccuracyChanged(Sensor sensor, int i) {} int[] getSensorTypes() {
return sensorTypes;
} private boolean isSensorEventBelongsToPluggedTypes(
SensorEvent sensorEvent) { for (int sensorType : sensorTypes) {
if (sensorEvent.sensor.getType() == sensorType) {
return true;
}
} return false;
}
}
Now we pass all sensor types in constructor of each inheritor of SensorDetector. Also we add our ‘security’ checking in onSensorChanged by calling isSensorEventBelongToPluggedTypes method and we provide new onSensorEvent method that is needed to be overridden instead of onSensorChanged. In this way we encapsulated logic of choosing tracking sensors to SensorDetector.
So, for example, constructor of ProximityDetector will look like:
public ProximityDetector(
float threshold, ProximityListener proximityListener) {
super(TYPE_PROXIMITY);
…
}And constructor of OrientationDetector will look like:
public OrientationDetector(
int smoothness, OrientationListener orientationListener) {
super(TYPE_ACCELEROMETER, TYPE_MAGNETIC_FIELD);
…
}Finally we update startSensorDetection in Sensey class to:
public void startSensorDetection(SensorDetector detector) {
final Iterable<Sensor> sensors =
convertTypesToSensors(detector.getSensorTypes()); if (areAllSensorsValid(sensors)) {
registerDetectorForAllSensors(detector, sensors);
}
}
Fixing bug of leaking detectors
It seems that we close to finish. But there is one more thing with sensor detectors that always points us to itself. It are Sensey class fields:
private ShakeDetector shakeDetector;
private FlipDetector flipDetector;
private OrientationDetector orientationDetector;
private ProximityDetector proximityDetector;
private LightDetector lightDetector;They used by Sensey to remember what detector we’ve created and when. And since we have reference to them, we can close it. Isn’t it?
Actually not quite. Let’s try to write test, that breaks this rule. We will try to start proximity detection with two different listeners and then try to stop both of them:
@Test
public void detectNoListenerWithStoppingTwoSameDetections() {
addSensor(TYPE_PROXIMITY);
ProximityDetector detector1 =
startProximityDetection(mock(ProximityListener.class));
ProximityDetector detector2 =
startProximityDetection(mock(ProximityListener.class)); sensey.stopProximityDetection(fakeListener1);
sensey.stopProximityDetection(fakeListener2); assertFalse("Sensor manager need to contain no detectors",
shadowSensorManager.hasListener(detector2));
assertFalse("Sensor manager need to contain no detectors",
shadowSensorManager.hasListener(detector1));
}private ProximityDetector startProximityDetection(
ProximityListener listener) { sensey.startProximityDetection(listener);
return getDetector(ProximityDetector.class);
}private void addSensor(int type) {
shadowSensorManager.addSensor(type, mock(Sensor.class));
}private <T extends SensorDetector> T getDetector(
Class<T> detectorClass) {
//somehow try to get instance of storable detector
}
If we run this test, it will fail, as we expected. This happens because when we start proximity detection second time Sensey changes value of proximityDetector to new one and loose reference to first detector. Therefore first assert line is executed normally and second fails.
To fix bug we need to choose one of two ways or use both of them:
- Change ‘stop’ detection declaration. It will pass listener, that was passed to ‘start’ method, as a parameter. Thus we can find appropriate inner detector by our listener.
- Remain declaration, but now ‘stop’ method will stop all detection of same type that was started.
I’ve chosen first variant, since I thought that for client more necessary possibility of stopping some concrete detectors not just all of them.
Now we need add map from detector’s listeners to instance of created detectors. But unfortunately we have no common interface for listeners, therefore this map will be from Object to SensorDetector:
/**
* Map from any of default listeners (
* {@link FlipListener flipListener},
* {@link LightListener lightListener},
* {@link OrientationListener orientationListener}
* {@link ProximityListener proximityListener}
* and {@link ShakeListener shakeListener})
* to SensorDetectors created by those listeners.
*
* This map is needed to hold reference to all started detections
* <strong>NOT</strong> through
* {@link Sensey#startSensorDetection(SensorDetector)},
* because the last one passes task of holding reference of
* {@link SensorDetector sensorDetector} to the client
*/private final Map<Object, SensorDetector> defaultSensorsMap =
new HashMap<>();
Then we introduce two methods ‘startLibrarySensorDetection’ and ‘stopLibrarySensorDetection’. They are similar to ‘startSensorDetection’ and ‘stopSensorDetection’. Actually they wrap these methods but also add logic of working with map:
private void startLibrarySensorDetection(SensorDetector detector, Object clientListener) {
if (!defaultSensorsMap.containsKey(clientListener)) {
defaultSensorsMap.put(clientListener, detector);
startSensorDetection(detector);
}
}private void stopLibrarySensorDetection(Object clientListener) {
SensorDetector detector =
defaultSensorsMap.remove(clientListener);
stopSensorDetection(detector);
}
Now we can change all ‘start’/’stop’ methods for library detectors (with changing declaration of ‘stop’ method). Example for ProximityDetector:
public void startProximityDetection(
ProximityListener proximityListener) { startLibrarySensorDetection(
new ProximityDetector(proximityListener),
proximityListener);
}public void startProximityDetection(
float threshold, ProximityListener proximityListener) { startLibrarySensorDetection(
new ProximityDetector(threshold, proximityListener),
proximityListener);
}public void stopProximityDetection(
ProximityListener proximityListener) {
stopLibrarySensorDetection(proximityListener);
}
Finally we can remove these lines:
private ShakeDetector shakeDetector;
private FlipDetector flipDetector;
private OrientationDetector orientationDetector;
private ProximityDetector proximityDetector;
private LightDetector lightDetector;Also we need to change our tests, because we changed declaration of public methods. And we need to change our bug-test:
@Test
public void detectNoListenerWithStoppingTwoSameDetections() {
addSensor(TYPE_PROXIMITY);
ProximityListener fakeListener1 = mock(ProximityListener.class);
ProximityListener fakeListener2 = mock(ProximityListener.class);
ProximityDetector detector1 =
startProximityDetection(fakeListener1);
ProximityDetector detector2 =
startProximityDetection(fakeListener2); sensey.stopProximityDetection(fakeListener1);
sensey.stopProximityDetection(fakeListener2); assertFalse("Sensor manager need to contain no detectors",
shadowSensorManager.hasListener(detector2));
assertFalse("Sensor manager need to contain no detectors",
shadowSensorManager.hasListener(detector1));
}
It passed, and also all other tests are passed, so our bug is fixed.
In the end of this phase the code looks like this.
And in the end of all refactoring test coverage looks like:
Afterwords
During the refactoring of Sensey library we’ve made changes of public methods, that anyone can use. I now that it’s bad. Backward compatibility, I now :( But my post is just example of how any programmer can make something for project, something, I think, important without creating it’s own super-mega-project. Of course, in ideal we needed to remain all wrong methods, but just mark them as deprecated. But I think it is not so necessary here.
As I said. I just tried to show you, that if you want to improve your skills, if you want to make something important, if you want to work with new features of some framework, if you make more practice, it isn’t necessary to build your own super application that conquer the hearts of millions users. Sometimes you can just help other guys.
And of course my article does not claim to be a perfect example of refactoring.
Thank you all to paid your attention to it.
I will glad to hear your comments and suggestions.
