I'm not going to discuss global things like design patterns and so on, as there's enough literature on that topic. Instead, I'd like to attract your attention to some small things that still affect lucidity and maintainability of a code.
First of all, if you give access to something that shouldn't be used in production code, it gets used sooner or later. Just because it exist. The argument that it's an internal stuff doesn't work as it absolutely shouldn't be matter, writing you a library or isolated project! Treat people who will add anything to your internal codebase the same way as if they use your existing code as a different library.
Thus, if for some reason in test, you need to call something that should not be called on prod — use reflection or whatever else but do not expose it in production code! And there's no reason to afraid of reflection for test proposes, at the end, you using Mockito, shouldn't it be bad in that case as well?
I've heard quite a lot, aweird strange argument that «JavaDoc means that code is not self-documented». I wouldn't talk about that self-documented code in reality is myth — it's an absolutely separate topic. JavaDoc is about providing a contract with your code. And again, it doesn't matter, writing you a library or isolated project! Only on private methods, I can agree, that JavaDoc can be skipped. Who is writing one service in your project shouldn't go and read all implementations of other services or DAOs or whatever that he's using. He should be able to open an interface and read JavaDoc and that is it!
For example, if you're writing something that returning Map, and using LinkedHashMap to provide some specific order — you have to mention that in JavaDoc! Otherwise, who is using that service can't rely on the order as it wasn't part of the contract, and nothing will prevent you to silently change that behaviour later.
The other popular argument: «Tests are better description». Of course, BDD is a useful thing and it's a pretty nice way to check your code behaviour, but someone who will write one service in your codebase should investigate all tests of services (neither their implementation) that he will need to use before doing that! You have to provide a contract!
The same reason as in a previous paragraph. To write new service we want just open interface and read about a contract of services that we'll use. We are not interesting in the implementation!
Test only things that you really should! There's absolutely no sense to make a unit test for controller that doing nothing more than simply delegate everything to a service.
There's no sense to test your domain layer. I saw some examples when people test sterilisation of a POJO by Jackson, explaining that as they're testing that annotations is putted correctly… Come on! Tests are description of how your code logic should work and Jackson annotations is also description! Your just cannot test that, as if you write one description wrong, why you decided that by adding another one you will avoid mistake and not just copy it? Furthermore, for any change in your model, now you will have to change same description in two different places!
This particular example was even more ridiculous as ObjectMapper was created in each test, so, has nothing with the real one in a Spring context. Thus, because of different configurations it may serialise differently in prod at the end…
If your description is correct, you can check only by integration tests. Unit tests for serialisation/deserialisation checking nothing other than serialisation/deserialisation works. And if it is a part of some framework and not part of your code — do not test it!
For example, Spring core brings lifecycle of beans to you and instruments to work with it and all services should be beans inside the context. If you want, for example, to do something with a DB before any of beans start using it, use ContextStartedEvent or BPP for for your DB connector bean but do not create separate object for the DB connector! Otherwise you will create second configuration that have to be maintained exactly the same way as the first one and this is the field for potential bugs. Don't reinvent the wheel if you already have a proper instrument!
If you catch an exception, always log it or throw forward (but don't do both). If you want to use another exception, wrap the original one in it! Even in situations when exception happened due to invalid user input. For example, you catch some ParseException and want to throw IllegalArgumentException that will be handled later and the 400 code will be sent. Wrap ParseException and log everything in your handler before sending the response! When something goes wrong (for instance, there's a bug and expected input gets 400) you will be extremely glad to see full stacktrace in you logs and not just IllegalArgumentException that can't tell exactly which part of the request failed.
Exception — for something that goes not as was expected! Something happened — you throw, but if you want to do some additional operations (like call to some services or DB) to figure out the state — it should not be exception any more: create a proper class that you will call Report or whatever and return it to the user, but do not provide information by throwing exception to the top in that case!
Functional approach is not a silver bullet. Sometimes it just making code more complicated and less readable. If you want just to convert List to Set, without any additional operation in between, there's absolutely no reason to do it through .collect(toSet()). When you have List with some items that should be converted to Map<K, List>, or something even more tricky, that require some merge — just do it through .forEach, it'll be much nicer than toMap with two mappers and merge function.
I thought that for most of developers that things are obvious. Unfortunately, I've found out that it isn't a case… Hope that this small article will push to rethink approach…
Polluting code with 'test-only' methods or access modifiers
First of all, if you give access to something that shouldn't be used in production code, it gets used sooner or later. Just because it exist. The argument that it's an internal stuff doesn't work as it absolutely shouldn't be matter, writing you a library or isolated project! Treat people who will add anything to your internal codebase the same way as if they use your existing code as a different library.
Thus, if for some reason in test, you need to call something that should not be called on prod — use reflection or whatever else but do not expose it in production code! And there's no reason to afraid of reflection for test proposes, at the end, you using Mockito, shouldn't it be bad in that case as well?
Avoiding of JavaDoc
I've heard quite a lot, a
For example, if you're writing something that returning Map, and using LinkedHashMap to provide some specific order — you have to mention that in JavaDoc! Otherwise, who is using that service can't rely on the order as it wasn't part of the contract, and nothing will prevent you to silently change that behaviour later.
The other popular argument: «Tests are better description». Of course, BDD is a useful thing and it's a pretty nice way to check your code behaviour, but someone who will write one service in your codebase should investigate all tests of services (neither their implementation) that he will need to use before doing that! You have to provide a contract!
No interface for single implementation of a service
The same reason as in a previous paragraph. To write new service we want just open interface and read about a contract of services that we'll use. We are not interesting in the implementation!
Overtesting in unit-tests
Test only things that you really should! There's absolutely no sense to make a unit test for controller that doing nothing more than simply delegate everything to a service.
There's no sense to test your domain layer. I saw some examples when people test sterilisation of a POJO by Jackson, explaining that as they're testing that annotations is putted correctly… Come on! Tests are description of how your code logic should work and Jackson annotations is also description! Your just cannot test that, as if you write one description wrong, why you decided that by adding another one you will avoid mistake and not just copy it? Furthermore, for any change in your model, now you will have to change same description in two different places!
This particular example was even more ridiculous as ObjectMapper was created in each test, so, has nothing with the real one in a Spring context. Thus, because of different configurations it may serialise differently in prod at the end…
If your description is correct, you can check only by integration tests. Unit tests for serialisation/deserialisation checking nothing other than serialisation/deserialisation works. And if it is a part of some framework and not part of your code — do not test it!
Afraid of framework's 'magic'
For example, Spring core brings lifecycle of beans to you and instruments to work with it and all services should be beans inside the context. If you want, for example, to do something with a DB before any of beans start using it, use ContextStartedEvent or BPP for for your DB connector bean but do not create separate object for the DB connector! Otherwise you will create second configuration that have to be maintained exactly the same way as the first one and this is the field for potential bugs. Don't reinvent the wheel if you already have a proper instrument!
Throw away original exceptions
If you catch an exception, always log it or throw forward (but don't do both). If you want to use another exception, wrap the original one in it! Even in situations when exception happened due to invalid user input. For example, you catch some ParseException and want to throw IllegalArgumentException that will be handled later and the 400 code will be sent. Wrap ParseException and log everything in your handler before sending the response! When something goes wrong (for instance, there's a bug and expected input gets 400) you will be extremely glad to see full stacktrace in you logs and not just IllegalArgumentException that can't tell exactly which part of the request failed.
Businesses logic is built on exceptions
Exception — for something that goes not as was expected! Something happened — you throw, but if you want to do some additional operations (like call to some services or DB) to figure out the state — it should not be exception any more: create a proper class that you will call Report or whatever and return it to the user, but do not provide information by throwing exception to the top in that case!
Using streams everywhere
Functional approach is not a silver bullet. Sometimes it just making code more complicated and less readable. If you want just to convert List to Set, without any additional operation in between, there's absolutely no reason to do it through .collect(toSet()). When you have List with some items that should be converted to Map<K, List>, or something even more tricky, that require some merge — just do it through .forEach, it'll be much nicer than toMap with two mappers and merge function.
I thought that for most of developers that things are obvious. Unfortunately, I've found out that it isn't a case… Hope that this small article will push to rethink approach…
dimpon
Good post! Exactly what I'm fighting with on my current job! Sad that so simple things needed to be explained! Go ahead!