The other day I participated in a company hackathon and I decided to make use of the Android camera. I’ve always said that the Android APIs are very bad (to put it mildly), but I’ve never actually tried to explicitly say what is wrong and how it could be better. Until now.
So, the Camera API is crappy. If you haven’t seen it before, take a look for a minute. You can use it in a lot of wrong ways, you can forget many important things, you don’t easily find out what the problem is, even with stackoverflow, and it just doesn’t feel good. To put it differently – there is something wrong with an API that requires you to read a list of 10 steps, some of which are highlighted as “Important”. How would you write that API? Let’s improve it.
And so I did – my EasyCamera wrapper is on GitHub. Below are the changes that I made and the reason behind the change:
setPreviewDisplay(..)
is required beforestartPreview()
, and it in turn is required before taking pictures. Why not enforce that? We could simply throw an exception “Preview display not set”, but that’s not good enough. Let’s get rid of the method that sets the preview display and then makestartPreview(..)
take the surface as parameter. Overload it to be able to take aSurfaceTexture
as well.- We’ve enforced the preview setting, so now let’s enforce starting the preview. We can again throw a “Preview not started” exception from
takePicture(..)
, but that happens at runtime. Let’s instead get rid of thetakePicture(..)
method out of theCamera
class and put it in aCameraActions
class. Together with other methods that are only valid after preview is started (I don’t know which ones exactly they are – it is not clear from the current API). How do we obtain theCameraActions
instance? It is returned bystartPreview(..)
- So far we’ve made the main use-case straightforward and less error-prone by enforcing the right set of steps. But the
takePicture(..)
method still feels odd. You can supply nulls to all parameters, but somehow there are two overloaded methods. Arguably, passing null is fine, but there are other options. One is to introduce aPictureCallback
interface that has all the four methods that can be invoked, and provide a blank implementation of all of them in aBasePictureCallback
class. That, however, might not be applicable in this case, because it makes a difference if you pass null vs callback that does nothing (at least on my phone, if I pass a shutter callback, the shutter sound is played, and if I pass null, it is not). So, let’s introduce theCallbacks
which is a builder-like class to contain all callbacks that you like. - So far so good, but you need to restart preview after a picture is taken. And restarting it automatically may not be a good default. But in the situation we are in, you only have
CameraActions
and callingstartPreview(..)
now requires a surface to be passed. Should we introduce arestartPreview()
method? No. We can make our interface methods return boolean and if the developer wants to restart preview, they should just returntrue
. That would be fine, if there weren’t 4 different callbacks, and calculating the outcome based on all 4 is tricky. That’s why a sensible option is to add arestartPreview
property to theCallbacks
class, and restart only after the last callback is invoked and only if the property is set to true. - The main process is improved now, but there are other things to improve. For example, there’s an asymmetry between the methods for opening and closing the camera. “open” and “release”. It should be either “open” and “close” or “acquire” and “release”. I prefer to have a
.close()
, and then (if you can use Java 7), make use of theAutoClosable
interface, and therefore the try-with-resource construct. - When you call
getParameters()
you can a copy of the parameters. That’s ok, but then you should set them back, and that’s counter-intuitive at first. Providing a simplecamera.setParameter(..)
method would be easier to work with, but theParameters
class has a lot of methods, so it’s not an option to bring them to theCamera
class. We can make parameters mutable? (That isn’t implemented yet) - One of the reasons the API is so cumbersome to use is the error reporting. You almost exclusively get “Came.takePicture failed”, regardless of the reason. With the above steps we eliminated the need of exceptions in some cases, but it would still be good to get better reports on the exact reason.
- We need to make the Camera mock-friendly (currently it isn’t). So
EasyCamera
is an interface, which you can easily mock.CameraActions
is a simple mockable class as well.
My EasyCamera project is only an initial version now and hasn’t been used in production code, but I’d like to get feedback on whether it’s better than the original and how to get it improved. At some point it can wrap other cumbersome Camera functionality, like getting front and back camera, etc.
Broken APIs are the reason for hundreds of wasted hours, money and neurons. That’s why, when you design an API, you need some very specific skills and need to ask yourself many questions. Josh Bloch’s talk on API design is very relevant and I recommend it. I actually violated one of his advice – “if in doubt, leave it out” – you have access to the whole Camera in your CameraActions, and that might allow you to do things that don’t work. However, I am not fully aware of all features of the Camera, that’s why I didn’t want to limit users and make them invent clumsy workarounds.
When I started this post, I didn’t plan to actually write EasyCamera. But it turned out to take 2 hours, so I did it. And I would suggest that developers, whenever confronted with a bad API, do the above exercise – think of how they would’ve written it. Then it may turn out to be a lot less effort to actually fix it or wrap it, than continue using it as it is.
The other day I participated in a company hackathon and I decided to make use of the Android camera. I’ve always said that the Android APIs are very bad (to put it mildly), but I’ve never actually tried to explicitly say what is wrong and how it could be better. Until now.
So, the Camera API is crappy. If you haven’t seen it before, take a look for a minute. You can use it in a lot of wrong ways, you can forget many important things, you don’t easily find out what the problem is, even with stackoverflow, and it just doesn’t feel good. To put it differently – there is something wrong with an API that requires you to read a list of 10 steps, some of which are highlighted as “Important”. How would you write that API? Let’s improve it.
And so I did – my EasyCamera wrapper is on GitHub. Below are the changes that I made and the reason behind the change:
setPreviewDisplay(..)
is required beforestartPreview()
, and it in turn is required before taking pictures. Why not enforce that? We could simply throw an exception “Preview display not set”, but that’s not good enough. Let’s get rid of the method that sets the preview display and then makestartPreview(..)
take the surface as parameter. Overload it to be able to take aSurfaceTexture
as well.- We’ve enforced the preview setting, so now let’s enforce starting the preview. We can again throw a “Preview not started” exception from
takePicture(..)
, but that happens at runtime. Let’s instead get rid of thetakePicture(..)
method out of theCamera
class and put it in aCameraActions
class. Together with other methods that are only valid after preview is started (I don’t know which ones exactly they are – it is not clear from the current API). How do we obtain theCameraActions
instance? It is returned bystartPreview(..)
- So far we’ve made the main use-case straightforward and less error-prone by enforcing the right set of steps. But the
takePicture(..)
method still feels odd. You can supply nulls to all parameters, but somehow there are two overloaded methods. Arguably, passing null is fine, but there are other options. One is to introduce aPictureCallback
interface that has all the four methods that can be invoked, and provide a blank implementation of all of them in aBasePictureCallback
class. That, however, might not be applicable in this case, because it makes a difference if you pass null vs callback that does nothing (at least on my phone, if I pass a shutter callback, the shutter sound is played, and if I pass null, it is not). So, let’s introduce theCallbacks
which is a builder-like class to contain all callbacks that you like. - So far so good, but you need to restart preview after a picture is taken. And restarting it automatically may not be a good default. But in the situation we are in, you only have
CameraActions
and callingstartPreview(..)
now requires a surface to be passed. Should we introduce arestartPreview()
method? No. We can make our interface methods return boolean and if the developer wants to restart preview, they should just returntrue
. That would be fine, if there weren’t 4 different callbacks, and calculating the outcome based on all 4 is tricky. That’s why a sensible option is to add arestartPreview
property to theCallbacks
class, and restart only after the last callback is invoked and only if the property is set to true. - The main process is improved now, but there are other things to improve. For example, there’s an asymmetry between the methods for opening and closing the camera. “open” and “release”. It should be either “open” and “close” or “acquire” and “release”. I prefer to have a
.close()
, and then (if you can use Java 7), make use of theAutoClosable
interface, and therefore the try-with-resource construct. - When you call
getParameters()
you can a copy of the parameters. That’s ok, but then you should set them back, and that’s counter-intuitive at first. Providing a simplecamera.setParameter(..)
method would be easier to work with, but theParameters
class has a lot of methods, so it’s not an option to bring them to theCamera
class. We can make parameters mutable? (That isn’t implemented yet) - One of the reasons the API is so cumbersome to use is the error reporting. You almost exclusively get “Came.takePicture failed”, regardless of the reason. With the above steps we eliminated the need of exceptions in some cases, but it would still be good to get better reports on the exact reason.
- We need to make the Camera mock-friendly (currently it isn’t). So
EasyCamera
is an interface, which you can easily mock.CameraActions
is a simple mockable class as well.
My EasyCamera project is only an initial version now and hasn’t been used in production code, but I’d like to get feedback on whether it’s better than the original and how to get it improved. At some point it can wrap other cumbersome Camera functionality, like getting front and back camera, etc.
Broken APIs are the reason for hundreds of wasted hours, money and neurons. That’s why, when you design an API, you need some very specific skills and need to ask yourself many questions. Josh Bloch’s talk on API design is very relevant and I recommend it. I actually violated one of his advice – “if in doubt, leave it out” – you have access to the whole Camera in your CameraActions, and that might allow you to do things that don’t work. However, I am not fully aware of all features of the Camera, that’s why I didn’t want to limit users and make them invent clumsy workarounds.
When I started this post, I didn’t plan to actually write EasyCamera. But it turned out to take 2 hours, so I did it. And I would suggest that developers, whenever confronted with a bad API, do the above exercise – think of how they would’ve written it. Then it may turn out to be a lot less effort to actually fix it or wrap it, than continue using it as it is.
Recent Comments