Todo: Transactions - debiting credits and assigning to someone else
This is something that needs a transaction
Todo: Security on the client (oops)
Don't do that
Comparing two strings
if (a.equals(b)) { ... } |
What if a is null? Ok, how about:
if (b.equals(a)) { ... } |
Oh FFS, what if b is null?
Action
Pre Java 7
if ( a == null ? b == null : a.equals(b) ) { ... } |
Java 7+
if (Object.equals(a,b)) {...} |
Concatenating many Strings using +
String foo = "Foo " + foo + ", bar "+ bar+ ", baz:" + baz; |
Preferred methods
String.format("Foo %s, bar %s, baz %s", foo, bar, baz); |
Or
If there is a lot, use StringBuilder.
Concatenating String in Logger methods
Please don't.
Preferred method
Creating a builder for a class by extending it
class Asset {}
class AssetBuilder extends Asset {}
An AssetBuilder does not enjoy an "is a" relationship with an Asset. "extends" means "is a". I think someone has discovered the builder pattern (popular in 2007 or so) and the whole team thinks its cool. Its a hack. Don't use it. By all means have a builder class. The builder class should not extend the class its building.
Testing a method by cut-and-pasting it into the test and then comparing it against the copy of itself
When methods are not implemented using TDD they can be difficult to test. A classic example is a class that converts an object that contains a list of objects. The non-tdd approach is to write a method that just iterates over the list, and then converts all the objects in place. But how do you test this?
The usual answer is:
- Create some dummy input.
- Give dummy input to converter to get result.
- Give dummy input to test method that generates what the result should be.
- Test that the "result" is the same as "what the result should be".
The problem here is that the method that generates "what the result should be" is a cut-and-paste copy of the actual method. It cannot be anything else. And to determine "what the result should be" the author has to look at the original code. Consequently, any errors in the original code are copied into the test method. The test just tests that the author can cut-and-paste, and provides no test at all.
How should this be tested? Separation of Concerns. The conversion of the elements in the list should be handled by:
- A converter class that takes a Foo and returns a Bar. Implement using Google Guava Function interface.
- The Guava transform method.
So suppose, the class Baz is to be converted to class Quz, and that Baz contains a list of Foo, and Quz contains a list of Bar. We have a BazToQuxConverter, and in this test we verify that all the fields of Baz are correctly converted to fields in Qux. When it comes to the conversion of the list of Foo to the list of Bar, then we test that the FooToBarConverter is called for every instance of Foo in the list and that the matching Bar is in the correct place in Baz's list. We do not need to test the fields in the Foos or the Bars. We only need to test that FooToBarConverter was called, and then we need to verify that FooToBarConverter is properly tested (with its own test class).
Separation of Concerns Violation
Let me try to explain to you, what to my taste is characteristic for all intelligent thinking. It is, that one is willing to study in depth an aspect of one's subject matter in isolation for the sake of its own consistency, all the time knowing that one is occupying oneself only with one of the aspects. We know that a program must be correct and we can study it from that viewpoint only; we also know that it should be efficient and we can study its efficiency on another day, so to speak. In another mood we may ask ourselves whether, and if so: why, the program is desirable. But nothing is gained —on the contrary!— by tackling these various aspects simultaneously. It is what I sometimes have called "the separation of concerns", which, even if not perfectly possible, is yet the only available technique for effective ordering of one's thoughts, that I know of. This is what I mean by "focusing one's attention upon some aspect": it does not mean ignoring the other aspects, it is just doing justice to the fact that from this aspect's point of view, the other is irrelevant. It is being one- and multiple-track minded simultaneously.Edsger W. Dijkstra in his 1974 paper "On the role of scientific thought"
Information Hiding Violation
In computer science, information hiding is the principle of segregation of the design decisions in a computer program that are most likely to change, thus protecting other parts of the program from extensive modification if the design decision is changed. The protection involves providing a stable interface which protects the remainder of the program from the implementation (the details that are most likely to change).
Writing a whole lot of Java and then worrying about version control
Amazingly, a team has twice written code and then asked, "Where should we put this in subversion?"
Determine where the code goes first, and build the module for it, and check that in. Then, if need be create the Jenkins project. These are all things to do before writing any Java.
Not Swarming
If a team is swarming then one person will do the API, another the Application, another the domain, and another the infrastructure. If a single developer checks in a complete top-to-bottom implementation involving all these layers then the team is not swarming. Why does this matter?
It matters because individual developers have different strengths and weaknesses, different levels of experience. When individual stories are done by individual developers, the quality of the software has huge variation. This is what we are seeing in our software. Conversely, when the whole team is involved in developing a single story, the quality of every story is high. This is what we see with the Carlsbad team.
MetaReview: One Task, One Review
Developers should create a review for themselves, one per per task for a given user story. They should not create a review per change set that they commit. Instead, they should use "Edit Details --> Add Content" in Crucible to add later commits to the same review.
Comments are the Exception, not the Rule (Better make sure that comment is useful)
We use JavaDoc comments for API methods only. Otherwise, putting comments in your code costs as many key strokes as the code itself - it's mostly a waste of time. Only add javadoc to methods if it adds significant value to do so (say for explaining in human readable terms a complex mathematical equation or arcane business rule). If you are adding comments to explain something non-intuitive in the code don't! Fix the code! The only javadoc we absolutely require in the code is at the API level. This is documentation shared with clients and communicates more than a simple method declaration does.
Turn off the automatic commenting in IntelliJ (clearing Preferences -> File and Code Templates -> Includes -> File Header removes the "created by" javadoc added to every new Java file). There is no reason to have "created by" comments in your code: we practice collective (team) ownership of code, and version control will track who created and modified code.
The #1 problem with comments is that they get out of date. Tests don't. If a test doesn't match the code, then it doesn't pass. Your tests should clarify and document the intent and purpose of your code.
Never use reflection in your test code
"I have this class that is injected with a property by Spring and it's private. I will use ReflectionTestUtils.setField(...) to set it in my tests." Just create the setter and be done with it.
"I need to set this property on an immutable object used in my test. I will use ReflectionTestUtils.setField(...) to set it in my tests." Just create a new instance of the object and use it.
I've got this class that does X, I'll use it for Y because I'm already using it here for X
"There's a DatabaseException class. I need to throw an exception for a logic (i.e. non-database access) failure. I'm in the infrastructure layer and I'm accessing a Database. So I'll just throw a DatabaseException!"
This is lazy, sloppy, bad programming. Did the database have a problem? No? Then don't throw a DatabaseException.
I've got this package that does X, I'll use it for Y because I can nationalise that they are related in some way
I've got a user details package for my UserDetails entity. I'll put the UserEmailVerification entity there too because it has the word "User" in it.
No! Each entity gets its own package. See DDD Basics
Rule: Only the API needs Request/Response classes
The methods in the API layer tend to have any require parameters bundled into a single class. So instead of:
String someMethod(String a, String b, Long c); |
We create a request object:
MethodResponse someMethod( MethodRequest r); |
And then build a MethodRequest class with members a,b and c.
Why?
We do this purely so that we can enhance the API later without breaking backwards compatibility for JAX-RS Java clients. We can add a new member to MethodRequest without breaking any client code, but adding a new parameter means that client code wont compile.
What about Application Layer?
Don't do this in any other layer. The purpose of this parameter aggregation is purely to support backwards compabilitiy in code bases that we have no control over. The other layers (App, Repo, Infra) are completely under our control and can (and should) be refactored at will.
Method / Variables Names That Lie
We can only improve code that we understand. If we work on code that we don't understand we will introduce bugs. Its that simple.
Method names that lie are so incredibly detrimental to our understanding of code that they should be ruthlessly eliminated.
Example: What role does this code assign to the current user, ADMIN or USER?
mapSqlParameterSource.addValue("_userrole", USER_ROLE); |
Answer: ADMIN!
There is no ADMIN_ROLE. Just USER_ROLE which equals "ADMIN". There is no constant for "USER".
Action
Use domain objects for constants, such as:
public class UserRole extends StringIdValue { public static final UserRole ADMIN = new UserRole("ADMIN"); public static final UserRole USER = new UserRole("USER"); public UserRole(@Nonnull String role) { super(role); }} |
Enums can work too.
Hacking vs Software Engineering
Note to non-technical staff: "Hacking" in the software development community does not mean "breaking into other people's computers" as it does in the media or security world.
I advocate taking part in hack-a-thons and speed programming contests. My team in the 2nd San Diego Startup Weekend built iOS and Android apps and got them submitted to the app store before the weekend was done. We won 2nd place! I sometimes do speed programming exercises at weekends. There is great experience to be gained from short bursts of rapid development: making something work no matter what. Hacking is being like the McGuyver of programming:
Resourceful and possessed of an encyclopedic knowledge of the physical sciences, he solves complex problems with everyday materials he finds at hand, along with his ever-present duct tape and Swiss Army knife.
Hacking is the antithesis of over-engineering, and if I must choose between those two extremes I would choose hacking nearly every time.
However, it is extremely important that we do not deploy hacked code! Hacking is best used to generate knowledge. Spikes are hacks! Hacking can also be tolerated to jump start a project, but only if the technical debt introduced is quickly understood and refactored.
Example: SimpleJDBCCall
We started using the SImpleJDBCCall to speed up writing our Repo layer (the "DAO" layer in old-school language). This is great. Get the thing working, and get it working fast. At the beginning of the project we want to validate our design, our access patterns. The high-risk aspect of the project was:
- Is our data model correct?
- Can we access PostgreSQL using Stored Procedures in this fashion?
- How do PosgreSQL schemas change our model compared to MySQL (with which we were more familiar)?
We are not too worried about the actual java code so as long as it works. Once these technical questions have been addressed, however, I would expect that we move quickly to a point where we understand the engineering used in our Repo layer. We could do that by switching to a different technology that we fully understand already, or by getting to grips with the technology we grabbed off the shelf, in this case, SimpleJDBCCall. Within a week or two, I would expect the team to have filled in the gaps, or abandoned the tech for one they do understand.
Doesn't really initialize, does it?
Doesn't really initialize, does it?
1
2 3 4 5 6 7 8 9 10 | initializeSimpleJdbcCall();simpleJdbcCallRegisterInstrument.withoutProcedureColumnMetaDataAccess();simpleJdbcCallRegisterInstrument.withSchemaName("dhapic");simpleJdbcCallRegisterInstrument.withFunctionName("register_instrument");simpleJdbcCallRegisterInstrument.declareParameters(new SqlParameter("_serialnumber", Types.VARCHAR), new SqlParameter("_token", Types.VARCHAR), new SqlParameter("_username", Types.VARCHAR), new SqlParameter("_type", Types.VARCHAR), new SqlParameter("_userrole", Types.VARCHAR), new SqlOutParameter("result", Types.INTEGER)); |
There is a number of problems with this:
- Using a SimpleJDBCCall for the first time it is "initialized" is slow.
- Changing any of its initialization parameters (such as schema, or function) causes it to be reinitialized (and slow) again.
- The advantage of using SimpleJDBCCall over JDBCTemplate is that the former queries the database for you to get the parameters. We have turned this off at line 2!
- If you are going to declare the parameters (as we do above) then there isn't much point in using it at all.
We are using SimpleJDBCCall in a way that negates any benefit to us (because we manually do all the work it provides automatically) but are causing it to reinitialize itself every time!
There are two viable options:
- Stop using SimpleJDBCCall and use underlying JDBC system. We're doing most of the work anyway.
- Understand what exactly SimpleJDBCCall does and use it maximally.
Cargo Cult Programming
Beyond hacking is Cargo Cult Programming. Some people actually program like this. The difference between a hacker and a cargo cult programmer is that a hacker may drop in code they don't understand, but they will then try to understand it.
With the end of the war, the military abandoned the airbases and stopped dropping cargo. In response, charismatic individuals developed cults among remote Melanesian populations that promised to bestow on their followers deliveries of food, arms, Jeeps, etc. The cult leaders explained that the cargo would be gifts from their own ancestors, or other sources, as had occurred with the outsider armies. In attempts to get cargo to fall by parachute or land in planes or ships again, islanders imitated the same practices they had seen the soldiers, sailors, and airmen use. Cult behaviors usually involved mimicking the day-to-day activities and dress styles of US soldiers, such as performing parade ground drills with wooden or salvaged rifles.[14] The islanders carved headphones from wood and wore them while sitting in fabricated control towers. They waved the landing signals while standing on the runways. They lit signal fires and torches to light up runways and lighthouses.
Cargo cult programming is a style of computer programming characterized by the ritual inclusion of code or program structures that serve no real purpose. Cargo cult programming is typically symptomatic of a programmer not understanding either a bug they were attempting to solve or the apparent solution (compare shotgun debugging, deep magic).[1] The term cargo cult programmer may apply when an unskilled or novice computer programmer (or one inexperienced with the problem at hand) copies some program code from one place and pastes it into another place, with little or no understanding of how the code works, or whether it is required in its new position.
By way of example, here is some actual code. There is a requirement that an email message be sent in HTML.
private SendEmailRequest createHTMLMessage(String message, String recipientUsername, String subject) { Destination dest = new Destination().withToAddresses(recipientUsername); Content subj = new Content().withData(subject); Content content = composeHTMLEmail(message); Body body = new Body(); body.setHtml(content); Message msg = new Message(); msg.setSubject(subj); msg.setBody(body); SendEmailRequest expectedRequest = new SendEmailRequest(); expectedRequest.setDestination(dest); expectedRequest.setMessage(msg); return expectedRequest;}private Content composeHTMLEmail(String message) { return new Content().withData(String.format(HTML_CONTENT, emailCloudSuiteLogo, message, emailEulaURL));} |
Here the programmer has created AWS SES objects to structure an email request, and have created an HTML string from a constant and some parameters. The HTML content is buried down inside a Content object, inside a Body object, inside a Message, inside a SendEmailRequest. Guess what we do then: we call sendEmailRequest.getMessage().getBody().getHtml().getData() to get the HTML as a string to pass to the real object that uses SES under the cover. Someone obviously has been bewitched by the trappings of structured email messages in the AWS SES SDK.
Destroying Vital Information with throw
} catch (UnsupportedEncodingException e) { throw new InvalidTokenException(e.getMessage());} |
Aaaaaaargh!!!
Action
Always, always, always, include the exception in the new one:
} catch (UnsupportedEncodingException e) { throw new InvalidTokenException(e);} |
See how easy that was? Less typing, better results!!! How often does that happen?
Destroying Vital Information with LOGGER.error(e.getMessage());
This logs the complete stack trace so we can have some hope of figuring out what went wrong.
LOGGER.error(e); |
This logs just the message, which is usually hopelessly inadequate and no help at all:
LOGGER.error(e.getMessage()); |
Please let this be the last time an architect has to write about this.
Action
Use LOGGER.error(e), or LOGGER.error("Some useful information",e); or a variant. (Or don't log anything at all! Know where the appropriate logging points are).
Do not put the class or method name in the message: its in the stack trace!
Throwing Unnecessary Exceptions
When using Spring, Apache CXF, Servlets, anything really, then throwing any exception (without catching it yourself obviously) will result in a 500 Response from the server.
Therefore, catching any exception in the API layer and converting them to a 500 error is a waste of time and money.
Giving the exception a nicer name isn't worth the extra effort.
Action:
Just let the exception bubble up and be caught by the Servlet container.
Converting Exceptions to Throw 500 Exceptions
catch (Exception e) { throw new WebApplicationException(Response.status( Response.Status.INTERNAL_SERVER_ERROR).entity(e.getMessage()).build()); } |
Apache CXF will take any uncaught exception and convert it to an 500 error. In addition, Apache CXF will ensure that the return entity is of the correct type, which the above code does not do. So the above code makes three errors:
- It is totally unnecessary.
- It does not set the return content type correctly.
- It uses e.getMessage() to destroy information.
Its five lines of code that does nothing but add bugs!
Action:
Delete this code entirely.
Without these lines of code, cxf will generate a 500 exception all by itself.
Unnecessary Catch Blocks
If your catch blocks do exactly the same thing, then collapse them using Java 7 multi-catch. If they happen to all be the sub-classes of the same exception, then just catch the base class.
Notice how IntelliJ is actually calling out this problem!
Bonus points for noticing: JsonMappingException, JsonParseException and UnsupportedEncodingException are all instances of IOException, and the first three catch blocks could be deleted with zero impact.
Action
Just one exception is fine.
} catch (IOException e) { LOGGER.error("Error creating token for {}", e); throw new InvalidTokenException(e);} |
What about the RuntimeException?
Catching RuntimeExceptions and doing nothing different
The author here correctly treats the RuntimeException differently. The other exceptions indicate a problem with the token, while the RuntimeException could mean anything, e.g. JVM memory exhaustion, or aliens invading. So the RuntimeException isn't converted into an InvalidTokenException. Good call. A better call would be to ignore it entirely. The fact that it was related to the token, if it turns out to be relevant, will be still be discoverable from the stack trace. Logging it provides no additional benefit. Its just more code to test.
Putting logic in finally clauses
What does the following code do? Can you tell?
1
2 3 4 5 6 7 8 9 10 11 12 13 14 | try { emailAddress = accountService.getEmailAddressForUsername(recipientUsername);} catch (UsernameNotFoundException e) { emailAddress = EmailAddress.from(recipientUsername.getValue()); throw e;} finally { Recipient recipient = Recipient.from(emailAddress); MessageTitle title = MessageTitle.from(subject); MessageBody body = MessageBody.from(messageText); message = Message.from(recipient, title, body); notification = notificationFactory.createNotification(sender, message, templateId); notificationRepository.saveNotification(notification);}emailService.sendEmail(message); |
If the accountService throws an exception, it propagates the exception onward....but wait! It creates a notification and saves it to the database, then it... no it does not send the email! Instead it throws that original exception.
Action
Do not throw an exception and then do the main processing in a finally clause. This does not do what you expect, and it is a bad pattern to follow. Instead, catch that exception and deal with it properly, then create the notification and send the message.
1
2 3 4 5 6 7 8 9 10 11 12 13 | String emailAddress;try { emailAddress = accountService.getEmailAddressForUsername(recipientUsername);} catch (UsernameNotFoundException e) { emailAddress = EmailAddress.from(recipientUsername.getValue());}Recipient recipient = Recipient.from(emailAddress);MessageTitle title = MessageTitle.from(subject);MessageBody body = MessageBody.from(messageText);message = Message.from(recipient, title, body);notification = notificationFactory.createNotification(sender, message, templateId);notificationRepository.saveNotification(notification);emailService.sendEmail(message); |
Testing for all possible exceptions
Dont do that. Only test for exceptions when the method under test must behave differently, such as releasing a resource or closing a file.
Testing for Exceptions that make no Material Difference
@Overridepublic InstrumentRegistration registerInstrument(InstrumentMetadata instrumentMetadata, UserCredentials adminUserCredentials) { accountService.verifyCredentials(adminUserCredentials); InstrumentRegistration instrumentRegistration = instrumentRegistrationFactory.create(instrumentMetadata); InstrumentRegistrationId instrumentId = instrumentRegistrationRepository.createRegistrations(instrumentRegistration, adminUserCredentials.getUsername()); instrumentRegistration.setId(instrumentId); return instrumentRegistration;} |
iRF.create could throw a NullPointerException. If it does this method will exit immediately. That's how exceptions work, and we can rely on that behavior. We do not need to test it. In fact, if we were to test it, then we would be saying "It is important the proper functioning of this application that it will throw an NPE if the factory does." That's not really true. That's how it works, but its not actually a requirement. So this test is superfluous:
Superfluous test code
Superfluous test code
@Test(expected = NullPointerException.class)public void testRegisterInstrument_NullPointerException() { InstrumentMetadata instrumentMetadata = new InstrumentMetadata(serialNumber, type); UserCredentials userCredentials = new UserCredentials(username, password); Mockito.doNothing().when(accountService).verifyCredentials(userCredentials); Mockito.doThrow(NullPointerException.class).when(instrumentRegistrationFactory).create(instrumentMetadata); instrumentConnectApplicationImpl.registerInstrument(instrumentMetadata, userCredentials);} |
Its not causing any damage, but it is taking time to write, and we don't have time to write things we don't need.
Action
Don't do it.
Not testing for when exceptions do make a material difference
The first line is very important. Its important that it happens first.
public InstrumentRegistration registerInstrument(InstrumentMetadata instrumentMetadata, UserCredentials adminUserCredentials) { accountService.verifyCredentials(adminUserCredentials); InstrumentRegistration instrumentRegistration = instrumentRegistrationFactory.create(instrumentMetadata); InstrumentRegistrationId instrumentId = instrumentRegistrationRepository.createRegistrations(instrumentRegistration, adminUserCredentials.getUsername()); instrumentRegistration.setId(instrumentId); return instrumentRegistration;} |
Without a test, a cut-and-paste error could make the code look like this:
public InstrumentRegistration registerInstrument(InstrumentMetadata instrumentMetadata, UserCredentials adminUserCredentials) { InstrumentRegistration instrumentRegistration = instrumentRegistrationFactory.create(instrumentMetadata); InstrumentRegistrationId instrumentId = instrumentRegistrationRepository.createRegistrations(instrumentRegistration, adminUserCredentials.getUsername()); accountService.verifyCredentials(adminUserCredentials); instrumentRegistration.setId(instrumentId); return instrumentRegistration;} |
Yes, the gaping security flaw isn't immediately obvious. Indeed at first glance you'd be forgiven for thinking they were identical code.
Unfortunately, the tests for this class all currently pass with the above, defective code. I have seen cut-and-paste errors like this make it to production (not here thankfully!).
Action
Use verify, as in this example:
verify never() example
verify never() example
1
2 3 4 5 6 7 8 9 | when(authenticationProvider.getCurrentUsername()).thenReturn(userId);doThrow(EntityNotFoundException.class).when(assetApplication).getAssetDetail(userId, assetId);try { assetJavascriptApi.getAssetDetailInfo(assetId); fail("Should have thrown exception.");} catch (WebApplicationException e) { assertEquals(Response.Status.NOT_FOUND.getStatusCode(), e.getResponse().getStatus());}verify(assetDetailInfoConverter, never()).apply(any(AssetDetail.class)); |
First, if the call to getAssetDetailInfo doesn't throw an exception then we have a problem. So we call fail on the next line. Then in the catch block we check some value on the exception itself (not always required - depends on the exception). Finally, we use verify(... never()) to make sure that if the exception was thrown, the assetDetailInfoConverter.apply method isn't called.
When we use verify(never()) we probably want to be as accepting as possible of inputs, e.g. using any(AssetDetail.class) in this case. In contrast, when we use verify without never() we should be as specific as possible.
Testing for things that make no material difference
Example
This code passes all tests:
simpleJdbcCallRegisterInstrument.withoutProcedureColumnMetaDataAccess();simpleJdbcCallRegisterInstrument.withSchemaName("dhapic");simpleJdbcCallRegisterInstrument.withFunctionName("register_instrument"); |
This code fails.
simpleJdbcCallRegisterInstrument.withoutProcedureColumnMetaDataAccess();simpleJdbcCallRegisterInstrument.withFunctionName("register_instrument");simpleJdbcCallRegisterInstrument.withSchemaName("dhapic"); |
Ironically, this code, which runs fine, fails tests:
While this code, which passes all tests, fails at runtime:
Checking Return Values of Internal Objects for Null
instrumentRegistration = instrumentConnectApplication.registerInstrument(instrumentMetadata, userCredentials);Preconditions.checkNotNull(instrumentRegistration);instrumentToken = instrumentRegistration.getToken(); |
When calling objects that we control, then decide whether or not Null is an acceptable return value. If null is not an acceptable return value then don't bother checking for it.
We do not second guess our own code. We assert what it should do and prove it to be correct using tests.
Action: if the method should be expected to return null
- Annotate the method with @Nullable.
- Check for null with an if statement.
Action: if the method should not be expected to return null
- Annotate the method with @Nonnull
- Make sure the tests for that method actually verify this fact.
- Use the return value with no checks.
In our example above, update the interface:
@Nonnullpublic InstrumentRegistration registerInstrument(@Nonnull InstrumentMetadata instrumentMetadata, @Nonnull UserCredentials adminUserCredentials); |
Then modify the code to omit the check:
instrumentRegistration = instrumentConnectApplication.registerInstrument(instrumentMetadata, userCredentials);instrumentToken = instrumentRegistration.getToken(); |
Marking HTTP API Story as Complete, without testing it over HTTP
HTTP/1.1 415 Unsupported Media TypeServer: Apache-Coyote/1.1Date: Wed, 20 Aug 2014 22:54:43 GMTContent-Length: 0 |
Returning text but claiming it is application/json
This is what CXF does if we just throw an exception and let CXF handle it:
HTTP/1.1 500 Internal Server ErrorServer: Apache-Coyote/1.1Content-Type: text/html;charset=utf-8Content-Length: 5914Date: Wed, 20 Aug 2014 23:22:37 GMTConnection: close500 - org.apache.cxf.interceptor.Fault: error! |
"1" noshade="noshade">type Exception report
message org.apache.cxf.interceptor.Fault: error!
description The server encountered an internal error that prevented it from fulfilling this request.exception java.lang.RuntimeException: org.apache.cxf.interceptor.Fault: error!
org.apache.cxf.interceptor.AbstractFaultChainInitiatorObserver.onMessage(AbstractFaultChainInitiatorObserver.java:116) org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:333) org.apache.cxf.transport.ChainInitiationObserver.onMessage(ChainInitiationObserver.java:121) org.apache.cxf.transport.http.AbstractHTTPDestination.invoke(AbstractHTTPDestination.java:239)
This is what happens when we write several extra lines of code to catch the exception and then throw it as 500:
HTTP/1.1 500 Internal Server ErrorServer: Apache-Coyote/1.1Date: Wed, 20 Aug 2014 23:25:53 GMTContent-Type: application/jsonContent-Length: 32Connection: closeError in instrument registration |
Anyone spot the problem?
This will cause a crash in any client, because we've said "application/json", but we're sending text. My REST Client handles the error politely:
Java clients will not. Java clients will have an unexplained deserialization error.
try { ... } catch (DatabaseInstrumentRegistrationException | NullPointerException e) { validationMessage = "Error in instrument registration"; LOGGER.error(validationMessage, e); throw new InternalServerErrorException(Response.status(Response.Status.INTERNAL_SERVER_ERROR).entity(validationMessage).build());} |
These lines produce the error. Deleting these lines eliminates the error.
This problem also happens for BadRequestException:
if (isNullOrEmpty(serialNumber)) { validationMessage = "SerialNumber cannot be null or empty"; LOGGER.error("InstrumentRegistration-serialNumber ", validationMessage); throw new BadRequestException(Response.status(Response.Status.BAD_REQUEST).entity(validationMessage).build());} |
HTTP/1.1 400 Bad RequestServer: Apache-Coyote/1.1Date: Wed, 20 Aug 2014 23:43:56 GMTContent-Type: application/jsonContent-Length: 69Connection: closeUserName cannot be null or empty or should not exceed 255 characters. |
The request body is text, but the header says application/json.
Massive Spring.xml files
Spring configuration files are just like classes: they should contain related functionality. Don't throw unrelated spring-beans into the same file just because the file is there already.
Action
Create multiple applicationContext-*.xml files for related objects.
Entities with missing or inappropriate hash and equals methods
Entities (in Domain Driven Design) are things that are distinguished by a unique identifier. A PersonnelRecord is an Entity that might use a social security number as a unique identifier. If we have two PersonnelRecord instances A and B, then they are considered equal if and only if they have the same unique identifier. Consider two PersonnnelRecords, one { 225-12-4432, "Sam","Smith"} and another ( 225-12-4432, "Sam", "Brown"}. The equals method should return true when these two instances are compared, because they represent information about the same person. In this case, the same person before and after a name change. Consequently, if the equals method compares all three fields for equality, then we have an error.
BAD Code: Equals tests too much
BAD Code: Equals tests too much
public class PersonnelRecord { private SocialSecurityId socialSecurityId; private String firstName; private String lastName; @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; PersonnelRecord that = (PersonnelRecord) o; if (!firstName.equals(that.firstName)) return false; if (!lastName.equals(that.lastName)) return false; if (!socialSecurityId.equals(that.socialSecurityId)) return false; return true; } @Override public int hashCode() { int result = socialSecurityId.hashCode(); result = 31 * result + firstName.hashCode(); result = 31 * result + lastName.hashCode(); return result; }} |
Actions
Appropriate Equals and Hash: Just test ID
public class PersonnelRecord { private SocialSecurityId socialSecurityId; private String firstName; private String lastName; @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; PersonnelRecord that = (PersonnelRecord) o; if (!socialSecurityId.equals(that.socialSecurityId)) return false; return true; } @Override public int hashCode() { return socialSecurityId.hashCode(); }} |
Constant Classes
Do not do this
public class AMPConstants { private AMPConstants() { } public static final String LATEST_EULA_VERSION = "latestEulaVersion"; public static final String USER_DETAILS_TABLE_NAME = "DHA_AMP_UserDetails"; public static final String USERNAME_COLUMN_NAME = "username"; public static final String EULA_VERSION_COLUMN_NAME = "eulaversion"; public static final String VERSION_COLUMN_NAME = "version"; public static final String FAMILY_NAME_COLUMN_NAME = "familyname"; public static final String FRIENDLY_NAME_COLUMN_NAME = "friendlyname"; public static final String PRINCIPAL_ID_COLUMN_NAME = "principalid"; |
This is the very opposite of object oriented design. This design says, "These things are constants, so lets put them in the same file, even thought hey are otherwise unrelated".
What happens if we apply this idea to other parts of our code? Why should it apply just to "public static final String" constructs? Why would this rule not apply to other Java constructs, such as "for loops", or "if statements"?
According to this theory, we should put all our if statements into a single class. All our while statements into another. If our if statement needs a while loop inside it, well we just create a method on the the AMPWhiles class, and have the AMPIfs class call it. If one of those needs a constant, it can use the AMPConstants class.
Clearly, this is absurd. Doing so would make it much harder to see strongly-related information, because such strongly-related information is spread out into different files not based on relevance, but on "what kind of thing they are." This affects our reading of the AMP code. For example, it would be easy to see all the names of DynamoDBUserDetailsRepository class if they were listed at the top of the class as a set of constants. Unfortunately they have been placed in a file named AMPConstants, and nobody can look at this file and determine which of the strings are for DynamoDBUserDetailsRepository and which are completely unrelated.
Action
Place constants in the class which "owns" them.
Define constants in the classes that need them
Define constants in the classes that need them
public class DynamoDBUserDetailsRepository implements UserDetailsRepository { public static final String USER_DETAILS_TABLE_NAME = "DHA_AMP_UserDetails"; public static final String USERNAME_COLUMN_NAME = "username"; public static final String EULA_VERSION_COLUMN_NAME = "eulaversion"; public static final String VERSION_COLUMN_NAME = "version"; public static final String FAMILY_NAME_COLUMN_NAME = "familyname"; public static final String FRIENDLY_NAME_COLUMN_NAME = "friendlyname"; |
