1

A team created a structure like this (pseudo-code):

struct Rectangle {
    int left, right, top, bottom;
}

Half a project later, the team realized that two thirds of the code was filling this structure with coordinates where top < bottom and the other third used top > bottom.

A requirement is that the coordinates will be coming into the system from different places (UI, imported files...) and are in different coordinate systems. Everyone knew this, but so far, people did ad-hoc swapping of top and bottom in their code when needed. This has become unmaintainable and led to bugs, because the Rectangle class can only implement common operations like intersect if a coordinate system is chosen.

  1. How could they have prevented this? (Is this calling for a technical solution or is it a question of people and best practices?)
  2. What would be the best steps to fix the codebase, so that top > bottom always, and the myriad places where rectangles are emitted and consumed don't break?

A library enforcing contracts/invariants? Not a widely used approach AFAIK, I only learnt about it in CS classes. Unit tests? The unit is fine, the consuming code is wrong. Documentation? The people that didn't think before coding this will not read it either.

  • 1
    You don't say what language the team was using. The language may or may not provide capabilities that will help with this problem. – John R. Strohm Sep 20 '18 at 21:38
  • While this can be language agnostic, is this an object-oriented language, procedural? That can make a difference. – Greg Burghardt Sep 20 '18 at 21:41
  • @GregBurghardt C++ – marczellm Sep 21 '18 at 7:39
7

How could they have prevented this? (Is this calling for a technical solution or is it a question of people and best practices?)

First things first, the ambiguity could possibly have been avoided with a better model. A rectangle can be represented as a pair (x, y) (or (top, left) to further reduce the ambiguity), a width and a height.

If one sticks with the structure where a rectangle has a top and a bottom, there is still no need for contracts here. You can easily guarantee that top < bottom (or the other way around) by using exceptions or assertions within the class. In other words:

  • Don't let anyone access class internals. getBottom and setBottom is OK (or if your language has properties, it's even better). Having a public field bottom is wrong.

    This is why, for instance, C#'s guidelines forbid public fields: if you want to expose a value to other classes, use a property.

  • Make sure setBottom have specific controls which don't let bad data go in. If bad data tries to go in, throwing an exception is an appropriate reaction.

    Note that you might have to handle a case where top and bottom are changed one after another; for instance, a rectangle with top: 3, bottom: 7 is moved ten points to the bottom; setting top to 3+10 would throw an exception, because 13 > 7. Depending on the usage of the class, you may have a shiftBy(x, y) method or something similar.

The rule such as top < bottom is an ordinary business rule. In the same way, a class Product should encompass a rule telling that the price cannot be zero or negative (unless in a specific domain, zero or negative prices make sense), and a class Temperature should have a rule that tells that values inferior to −273.15°C are not allowed.

All those rules belong to the classes themselves. This also means that if the rule should be changed later, you have only one location where you need to change it—the class.

Talking about contracts, they are not limited to CS classes. For instance, .NET ecosystem makes it not only possible to declare code contracts (including invariants) very easily, but also contains very powerful tools for static checking of contracts.

Some other languages, such as Eiffel, do have contracts as well, and the entire list of languages is quite impressive.

The benefit of the contracts, like they are implemented in .NET, is their propagation. If you're initializing a temperature class with a value of −300°C, you won't have to wait until runtime to get an exception: you get a error during the static checking (which, in some cases, means getting it nearly immediately in your IDE as you type the invalid value).

While convenient, you don't need it in order to avoid the situation you're describing. A simple exception/assertion would be enough.

What would be the best steps to fix the codebase, so that top > bottom always, and the myriad places where rectangles are emitted and consumed don't break?

That's a hard one. I'm surprised you haven't found the issue earlier (well, maybe not that surprised). I would expect the code start crashing from time to time because of the mismatch. For instance, if the code which computes an area of a rectangle assumes that top < bottom and if it's fed with a rectangle where top > bottom, the caller may cause an exception to be thrown when receiving a negative area while expecting it to be strictly positive.

Anyway:

  • If one part of the application assumes that top < bottom, and the other part does the inverse, and both don't interact, you can either keep it this way (while adding appropriate exceptions, assertions or code contracts), or just switch one of the parts to use the other convention (which may be quite difficult in practice).

  • Otherwise, put the exceptions in place and run the tests (if you don't have tests, good luck). If tests are correctly done, they will show you most of the occurrences of the mismatches. Of course, mismatches can also happen at the level of the interfaces between components, which won't be detected by unit tests. Checking manually your interfaces and adding exceptions there could be helpful (and make your interfaces less ambiguous).

Another technique which is worth trying is to create a new rectangle representation with a position, a width and a height. Once done, you can migrate code step by step to the new representation, while making sure to understand whether you should use a top or a bottom for the position. This way, you don't break existent code and don't slow down the team during the migration. Once done, the old rectangle structure can be removed.

  • Do you have suggestions regarding my question 2 above? that is, how to proceed with fixing all the code that uses (reads and writes) it wrong? – marczellm Sep 21 '18 at 7:41
  • Really the solution to this problem is two-fold: A) Utilize encapsulation; B) change the model to better fit the problem. +1 for identifying that a rectangle is really composed to two pieces of data: A position and dimensions. The "position" could be its own struct with a handle on a coordinate system, which would assist in some of the "common operations" the OP talks about. – Greg Burghardt Sep 21 '18 at 11:24
  • If one can modify left and right resp. top and bottom individually, it might be better to simply modify both if necessary to keep the invariant, instead of throwing an exception. It's certainly easier to use. That would break at the extremes however, unless zero width resp. height are allowed. – Deduplicator Sep 21 '18 at 21:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.