2018/10/31

Peering Under the Hood: Awful Code Practices

by Lucido Group

Code review is an essential part of any organization’s change control practice. We notice far too often that Openlink’s clients trust their developers, and 3rd party partners to deliver code of high quality. Faith in these service providers can be misplaced. All developers’ code should be reviewed; including ours.

Let’s look under the hood at snippets of some awkward solutions that are in production around the world. We’re sure you’ll agree that the standard needs to improve. We can help!

Meaningless Names

Arguably superficial, but when a developer names a class, method and variable, they communicate to a subsequent developer how the solution was designed and the business requirements resolved by the solution, or method, or line of code.

Meaningless names: what is in tbl_temp and what takes place here?

In the preceding snippet, it is not obvious what iSuccess represents; a value of 1 is often used to represent true, but the developer would be better off using a Boolean data type. It’s difficult to know what tbl_temp contains, although it is reasonably clear without a lot of effort, that it is a table and it has some kind of accounting data. Other variables that are confusing include tbl_row_holder, and fields in these tables that are labeled curr, order, cnt_at_pos.

Renaming the variables, and creating shorter private methods that describe the processing, makes code easier to understand for subsequent developers that need to debug and enhance the solution.

We always pay careful attention to variable, method and class names to make it easier for later developers to manage the code. We also use object-oriented programming techniques to dramatically improve the clarity of the solution, and make it more closely aligned with the business behavior being modeled.

Explain Yourself in Code

Some developers use no comments, and leave a tangled web of mystery.

A more experienced developer, aware that code is read more often than it is written, might include helpful comments, but retain many of the tangles in the code. A subsequent developer might make a bug fix or enhancement, and unfortunately retain outdated comments, that are no longer relevant or even accurate.

The best ways to explain to subsequent readers of the code what it is doing is by using meaningful variables, methods and classes. Describing behavior in classes can be the most intuitive approach, and make it easier for following developers.

Comments can be used to help a reader understand the intent of some code, perhaps explain the reason a solution was designed a certain way (for example, bug in the Openlink API, core code deficiency, non-performant alternatives). Javadocs are helpful for reusable classes, to encourage their re-use as though they were a public API.

Avoid Magic Numbers

The following snippet uses a hard-coded transaction type, a native feature of the application, and a hard-coded unique identifier (20002) for a user defined field. The magic happens when the user defined field’s value is ‘AAA’.

Magic numbers: What is type_id 20002 and why AAA?

Hard-coded solutions like this are brittle, and part of the reason that many organizations report that it is difficult and risky to change their configuration, no matter how innocuous it might seem.

Did you think that maybe this solution behaves differently in a Development, Test and Production? This data is environment-dependent because type_id 20002 might be a credit rating in Development and Test, but a reference ID field in Production.

Depending on the rest of the solution’s requirements, there are several design improvements available, and the solution might involve more than one of these enhancements –

  • Change the design to be object-oriented, with a system adapter that separates concerns with Findur’s data model from the specific business rules (here, somehow related to credit ratings).
  • Instead of using the hard-coded integer 20002, use the String representation of the transaction field: perhaps it’s ‘Credit Rating’.
  • Think about what the value ‘AAA’ means and whether this could change in future. If it represents one rating agency’s top rating, what about the other agencies? What if the threshold changes, and the 2nd top rating becomes acceptable for this solution? Now we need to look at ‘AAA’ and ‘AA’. It quickly gets awkward.
  • Retrieving the trade’s field value, and instantiating it as a Rating object could lead to a solution that is more easily understood, and maintained. Rating could have a Rank property. In fact, we have done something just like this previously to facilitate requirements related to missing ratings for an agency, rating equivalent comparisons, composite internal ratings, and similar requirements. Ask us for more information and we’ll publish the details.
  • If this solution is expected to operate on only one transaction, why not design it to use a Transaction object, instead of querying the database. Using an object, instead of the database, makes the solution much more flexible to new requirements. Querying the database can be a brittle solution, and is tightly coupled to the Findur data model. Often, minor changes in deal modeling can be a real headache when you couple a solution to the database (e.g. compare the difference between a vanilla swap and a daily-compounding OIS swap). Finally, for another justification for using a transaction object, see our paper about Trade Snapshots, and why you should design user-defined sim results to use transaction objects whenever it’s possible.

Pay Attention to Exception Handling

Silent Exceptions

This snippet highlights a few problems, but the primary problem we want to highlight here is that the code exists in order to report an exception, and yet if there is an exception encountered while sending the email, the catch block is empty. When there’s a problem, and there’s an email email failure, nothing at all will be reported.

A second major problem here is that the solution handles the email notification in a private method shared by no other solution. That’s a mistake because it increases the maintenance effort if the logging strategy needs to change.

We use a common logging framework that logs to files and has support for email. It is easy to change the log file location, file naming convention, email addresses, or any other part of the solution in only one place.

Silent exception: the catch block is empty
Log to a Persistent Location

Sometimes the code will perform some checks and, if there is a problem, log the details to the console. The console information is not necessarily retained. There is a Findur environment variable available to control whether the output to the console is printed to a log file. Logging to a file is not usually a significant performance drain, and at least a minimal level of logging should be written to a log file.

In our logging strategy, we usually log to a solution-specific file in order to be able to analyze a problem that occurred in a that solution. The log file will be fairly well-contained and typically small enough that it can be shared as an email attachment.

We usually also log to a single error log file shared by the native application. The benefit of logging to the shared log file is the ability to see in a single location all of the processing taking place on all processes and threads. When it is necessary to debug a production issue, having native application exceptions logged in the same file as custom code logging, it is easier to understand the root cause of a problem. When sharing the details with the Openlink Support Desk, it is easier to point to a particular native component as the origin of a problem, rather than a custom component.

Handle Exceptions at the Origin of a Problem

In the following snippet, the solution will continue and lead in an uncertain state with results that are difficult to predict. If the database query failed, the account object, which was initialized, will contain no columns or rows.

The native application behavior can change from one version to the next so perhaps the next version will have a null table, or a default table, it’s difficult to predict. Then, how will later processing handle a null or an empty account table? How much later in the code will an exception be encountered as a result of the malformed query here? Will it be obvious to the production support team that the root cause of the problem was in this account table?

Console logging of an exception

If the code had never caught this exception, at least the solution would have failed, and the stack would have reported which line of code threw an exception. The production support team will find it easier to identify the cause.

An alternative improvement could include better logging of the problem, including logging to a file. The developer could add context to the query execution such as the statement, the user, perhaps other relevant business conditions.

Poor Exception Handling

The following snippet relies on legacy Openlink behavior that has been progressively decommissioned in recent versions: methods returning an integer to represent success or failure of an operation.

Prefer throwing exceptions to return values

More recent versions of Findur have tended to throw an OException if a method fails. This has been part of a multi-year effort to move towards more modern development behavior. The enhancements have been rolled out to an increasing number of classes and methods with each release.

The change in behavior has generated negativity from some clients and some implementation consultants, who became accustomed to returning integer values to represent the result of an operation. The API change has affected the behavior of the solution when it encounters exceptions.

The solution in this illustration is responsible for saving interest rate, and other index data, and the failure might occur because of one or more security privileges or secured indices, or another problem. Logging the failure to a file, and continuing might lead to additional problems downstream, that are more difficult to debug.

However, if the behavior of the API changes in a new version, this solution might throw an exception instead of returning int_RetVal = 0. The behavioral change needs to be tested to ensure the expected behavior of the solution stands up in the new version.

Generally, it is better to throw an exception, and handle the exception gracefully higher in the stack.

One context in which an exception could be encountered running this plugin is a missing security privilege. In a production environment, users quickly become frustrated if they repeatedly encounter missing security privileges. It is best, from the user’s perspective, to report all of the missing security privileges the first time the solution is run, so the end-user may resolve all of the missing privileges with the system admin staff in one shot. That can sometimes mean attempting to continue processing, even if processing fails here.

For example, if the user is missing 3 required security privileges (save index data, save volatilities, save correlations) and missing access to 2 indices (exchanges rates, and USD LIBOR), then it would be most convenient to try to process all index data types and indices, reporting all 5 exception conditions, and failing the job.

The more significant deficiency (not displayed here) in this solution is that if there is any failure to save one index, the plugin result will not be reported to the dispatcher. This is part of a client’s end of day workflow. If there is an exception saving one or more indices, then it is important for the failure to be reported all the way up the stack to the EOD workflow so that the system administrator may investigate the failure. The exception handling in this solution demonstrates a different manner of silent failure.

Retaining Commented Code

About two-thirds of the following class file is commented out. Code repositories retain the history of source code so there is no need to retain commented code.

We don’t retain commented code.

Commented code: More comments than source code

Hard-Coded Lists of Business Objects

Sometimes code contains a list of business objects, such as the snippet below.

Hard-coded list of business objects

This snippet actually includes only three indices because the screenshot is dominated by commented code.

This list of business objects is fixed in one solution. Perhaps the behavior should be part of a broader list of features, and apply to several solutions. When the list is hard-coded in this simulation result, it might be needed in another as well. We often see the same list of objects copied and pasted from one plugin to another.

Perhaps there is a property of these indices that is the true reason custom logic applies: e.g. it’s all USD-denominated interest rate indices that should behave this way.

Some organizations prefer lists of business objects, such as these, to be managed in the code because of the benefits extended by the code repository, including retention of the history of changes, and ease of deployment.

Other organizations would prefer that the list of business objects be maintained, and controlled by business endusers. The code snippet here could be changed to retrieve the list of indices from a database user table that is under the control of a narrow set of users. If business users are responsible and accountable for the list of business objects, then it might be best to delegate maintenance of the solution to them.

Another alternative would be to extend the data model on the index definition to include a field that controls the the underlying behavior and why these three indices behave this way. For example, the field might allow the plugin to identify these three indices as inputs to a bootstrapped USD yield curve. The new field could be called ‘USD Yield Curve’ and take a Boolean value of Yes/No. The code could then dynamically retrieve the indices, so that if an index was ever added (OIS.USD) or perhaps disabled (LIBOR.USD) then the operation would be more easily updated.

It is important to understand the origin of the business requirement in order to recommend the most appropriate solution. And perhaps in this case, the client is comfortable with this solution as it is shown. But don’t retain commented code!

Complex If/Then/Else Statements

The next snippet is hard to read, and it’s part of a solution that is currently in production. This solution handles a client’s internal compliance to control trade entry and lifecycle events.

This is the most complex series of if/then/else statements we have ever seen. The people who contributed to the solution include some very talented developers with a lot of experience. Don’t make the mistake of immediately blaming the developers.

if(isExerciseOption()) {
//do something
}
if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_TEMPLATE.toInt()) {
if (!("None".equalsIgnoreCase(Ref.getName(SHM_USR_TABLES_ENUM.PARTY_TABLE, getExternalBunit())))) {
//do something
}
} else if (getFromStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_PENDING.toInt()) {
if (getToStatus() == PartyLockUtil.TRAN_STATUS_LOCKED) {
if ("None".equalsIgnoreCase(Ref.getName(SHM_USR_TABLES_ENUM.PARTY_TABLE, getExternalBunit())) || Str.isEmpty(Ref.getName(SHM_USR_TABLES_ENUM.PARTY_TABLE, getExternalBunit())) == 1) {
//do something
} else if((PartyLockUtil.partyGroupLock(getExternalBunit(), Ref.getUserId(), 0)) == 0) {
//do something
} else if(isExerciseOption()) {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_TEMPLATE.toInt()) {
if (!("None".equalsIgnoreCase(Ref.getName(SHM_USR_TABLES_ENUM.PARTY_TABLE, getExternalBunit())))) {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_NEW.toInt()) {
if(isExerciseOption()) {
if(getTran().getFieldInt(TRANF_FIELD.TRANF_INS_CLASS.toInt()) == INS_CLASS_ENUM.INS_SHARED.toInt() &&
getTran().getFieldInt(TRANF_FIELD.TRANF_TRAN_TYPE.toInt()) == TRAN_TYPE_ENUM.TRAN_TYPE_TRADING.toInt()) {
//do something
}
} else {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_VALIDATED.toInt()) {
if(isExerciseOption()) {
//do something
} else {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_PENDING.toInt()) {
if(isExerciseOption()) {
//do something
}
}
} else if (getFromStatus() == PartyLockUtil.TRAN_STATUS_LOCKED) {
//do something
if (getToStatus() == PartyLockUtil.TRAN_STATUS_LOCKED) {
if ("None".equalsIgnoreCase(Ref.getName(SHM_USR_TABLES_ENUM.PARTY_TABLE, getExternalBunit())) || Str.isEmpty(Ref.getName(SHM_USR_TABLES_ENUM.PARTY_TABLE, getExternalBunit())) == 1) {
//do something
} else if (orig_user != Ref.getUserId()) {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_NEW.toInt()) {
if (orig_user != Ref.getUserId()) {
//do something
} else {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_DELETED.toInt()) {
if(orig_user == -1 && getTran().getFieldInt(TRANF_FIELD.TRANF_PERSONNEL_ID.toInt()) == Ref.getUserId()) {
/
In an ideal world, this should never ever happen!
*/
//do something
} else if (orig_user != Ref.getUserId() && SecurityObject.userCanAccess(SecurityIDs.DeleteAllLockedTrades) != 1) {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_VALIDATED.toInt()) { if (retval != OLF_RETURN_CODE.OLF_RETURN_SUCCEED.toInt()) {
if(isExerciseOption()) {
//do something
} else {
//do something
}
}
if (orig_user != Ref.getUserId()) {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_TEMPLATE.toInt()) {
if (orig_user != Ref.getUserId()) {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_PENDING.toInt()) {
if(isExerciseOption()) {
//do something
} else {
//do something
}
}
} else if (getFromStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_NEW.toInt()) {
//do something
if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_PENDING.toInt() || getToStatus() == PartyLockUtil.TRAN_STATUS_LOCKED || getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_TEMPLATE.toInt()) {
//do something
} else if(getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_NEW.toInt()) {
if((PartyLockUtil.partyGroupLock(getExternalBunit(), Ref.getUserId(), 0)) == 0) {
//do something
} else {
//do something
}
} else if (getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_VALIDATED.toInt()) {
if((iNotnl != iNotnlOrig)) {
//do something
} else {
//do something
}
}
} else if(getToStatus() == TRAN_STATUS_ENUM.TRAN_STATUS_AMENDED_NEW.toInt()) {
if(getTran().getFieldInt(TRANF_FIELD.TRANF_TOOLSET_ID.toInt()) == TOOLSET_ENUM.LOANDEP_TOOLSET.toInt() && SecurityObject.userCanAccess(SecurityIDs.UpsizeModificationofLoanDeps) != 1) {
//do something
} else {
//do something
}
} else {
//do something
}
}

Can anybody understand the intention of this long block of if/then/else statements?

If something needs to change, can it be changed without breaking existing controls? Are there automated unit tests performed on the solution to identify bugs when the code needs to be changed?

Is there a better way to design the solution to remove the if/then/else statements to get to the origin of the complicated behavior implied by the code?

If this is a plugin required to customize a commercial trading system to conform to proprietary internal controls, can any of this logic be migrated to another application without completely redesigning the solution?

We would likely redesign this solution in an object-oriented manner that would facilitate automated tests. Additionally, we would adopt a system-agnostic adapter pattern so that the solution may connect to Findur today, and a different application in the future.

The Worst: Your Opinions

What’s the worst thing you’ve seen delivered by a Findur developer?

Keep the comments clean and the parties (victims and perpetrators) anonymous!