Friday, 27 November 2015

Unit testing (PHP): keeping sight of the intent of the tests

An article with some actual code in it! That'd become a rarity. This is actuallya real-world challenge I'm faced with in backfilling some unit tests. Yeah, I know I'm an advocate of TDD so that should mean there's no "backfilling" of tests: they all get done up front. But for reasons I won't go into... I'm backfilling testing for this function.

Here's the function:

public static function loadValidatorMetadata(ClassMetadata $metadata)
    $metadata->addPropertyConstraint('languageId', new Assert\Range(
        ['min' => 1, 'max' => self::$maxLegacyLanguageId]

    $metadata->addPropertyConstraint('destinationId', new Assert\Range(['min' => 1]));

    $metadata->addConstraint(new Assert\Callback('validateArrivalDate'));
    $metadata->addConstraint(new Assert\Callback('validateNights'));

    $metadata->addPropertyConstraint('firstName', new Assert\NotBlank());
    $metadata->addPropertyConstraint('firstName', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    $metadata->addPropertyConstraint('surName', new Assert\NotBlank());
    $metadata->addPropertyConstraint('surName', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]

    $metadata->addPropertyConstraint('email', new Assert\NotBlank());
    $metadata->addPropertyConstraint('email', new Assert\Email());

    $metadata->addPropertyConstraint('groupTypeId', new Assert\Range(['min' => 1]));
    $metadata->addPropertyConstraint('groupSize', new Assert\NotNull());
    $metadata->addPropertyConstraint('groupSize', new Assert\Range(
        ['min' => GroupEnquiryService::MIN_GROUPS_THRESHOLD, 'max' => BookingService::MAXIMUM_PEOPLE]
    $metadata->addPropertyConstraint('youngestAge', new Assert\Range(
        ['min' => 1, 'max' => GroupEnquiryService::MAX_AGE]

    $metadata->addPropertyConstraint('telephoneCountryCode', new Assert\NotBlank());
    $metadata->addPropertyConstraint('telephoneCountryCode', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    $metadata->addPropertyConstraint('phone', new Assert\NotBlank());
    $metadata->addPropertyConstraint('phone', new Assert\Length(
        ['min' => GroupEnquiryService::MIN_TELEPHONE_NUMBER_LENGTH]
    $metadata->addConstraint(new Assert\Callback('validateMobileCountryCode'));
    $metadata->addConstraint(new Assert\Callback('validateMobile'));

    $metadata->addConstraint(new Assert\Callback('validateAdditionalInfo'));

First things first: yeah, normally I'd not write a function that's 40-odd lines long, but in this case there's very little actual logic, plus it's only setting some validation config. BTW this is using the Symfony 2 validation framework (Symfony: validation), as implemented by the Silex microframework (ValidatorServiceProvider: validating objects).

To provide context, we have a class - basically a validation bean - thus:

class GroupEnquiry
    public $languageId;
    public $destinationId;
    public $arrivalDate;
    public $nights;
    public $firstName;
    public $surName;
    public $email;
    public $telephoneCountryCode;
    public $phone;
    public $mobileCountryCode;
    public $mobile;
    public $groupTypeId;
    public $groupSize;
    public $youngestAge;
    public $additionalInfo;

And that loadValidatorMetadata() function provides the constraints for Symfony to enforce. As you can see: there's range validation, existence validation, length validation, date validation, and a coupla compound constraints implemented via callbacks.

So, anyway, I need to write the testing for loadValidatorMetadata(). Initially I was staring at the code going and going "how the hell?".

Then I thought: I'll assert the addConstraint() and addPropertyConstraint() methods are called for the correct properties, and the correct number of times. But that seemed unhelpful. What's the point of checking that some constraint is applied to a property, but not checking what sort of constraint. IT all seemed too contrived.

Then I thought that the constraints themselves don't need testing here because I'm either testing our callbacks elsewhere, or Symfony have done their own testing. In theory this is a fine notion, but in reality it does not actually help me test our validation code works.

I was stumped as to how to test this stuff, without basically replicating the function in parallel for test. And that clearly sucks as far as effort and common sense goes. It was the wrong approach.

Thursday, 26 November 2015

Guest author: a response to "Why good developers write bad code"

I could get used to this. Another one of my readers has provided my copy for today, in response to my earlier guest-authored article "Why good developers write bad code". This originally was a comment on that article (well: it still is) but it stands on its own merit, and Brian's cleared me to reproduce it as an article. Brian's a mate, one of my colleagues at The quote below is unabridged. I have corrected one typo, and the links are my own.

Brian says:

Really thought provoking post, Adam, and my immediate impression is that your mate seems to be describing an awful lot of dysfunctionality here. I'll get my declaration of interest out in the open straightaway and own up to being a massive Agile/XP fanboy, and consequently I think much of what your mate describes could be cured by the adoption of Agile/XP practices.

The "...but then he gets a chunky piece of work and, more often than you'd expect, it goes astray" quote is very revealing, because it would appear that there's no effort made to break large requirements down into smaller ones and we're on a slippery slope from here on in.

Developers often receive very coarse grained requirements and irrespective of our natural coding ability, we're not naturally adept at managing large pieces of work. That's not to say we can't manage large requirements, but without training, mentoring and support developers who might be first rate coders, can often struggle. In this case, it sounds like the developer in question is trying to eat an elephant in one sitting, and then being left alone to go dark and "fail". Reading between the lines it seems that this is accompanied by a seat of the pants estimate, which becomes a commitment as soon as he's uttered it, and before long we're into death march territory.

Also I wouldn't want to blame the developer in question, because as your mate points out, the same thing keeps happening time and again but there's no intervention or help from the guy's colleagues or his manager. Clearly they're at a loss as to how to fix the problem but unless someone can show him a better way, how is he going to improve?...

Anyway... if I was working with him I'd strongly encourage him to forget about eating the whole elephant, and just start by trying to nibble its trunk. Use TDD religiously from the outset, stick to the red-green-refactor cycle and You Ain't Gonna Need It (even more religiously), avoid Big Design Up Front, and let the application architecture evolve as he uncovers smells in the code that he's writing.

See how far he gets nibbling the elephant's trunk in one day, and if all goes then we'll reconvene and decide on the next bit of the elephant to consume, and carry on in that fashion. If he takes longer than a day - and we'll know that because we have daily stand-ups right? - then we'll pair up and see what the problem is. Invariably if the guy's a first rate coder, it will only be because he's bitten off more than he can chew, so get him to stop, re-plan the work, and decrease the size of the immediate task in front of him so he can actually finish it. In short, KISS, keep tasks small and try to conquer complexity. Writing code is intrinsically hard, we should actively avoid anything that makes it harder.

I think it's also worth bearing in mind that many, if not most developers, are naturally quite reluctant to provide honest progress reports if things aren't going well. Combine that with our tendency to give hopelessly optimistic estimates and we're already in a very bad place. I've found that it's almost endemic in our industry that developers are scared of giving bad news to anyone, let alone their line manager or project manager. Developers are usually clever people who spent much of their academic careers being praised for their intellectual prowess, and to be put in a position where they are suddenly the bad guys because they didn't hit an arbitrary deadline can be very distressing and something to avoid at all costs.

If there was just one thing I would wish for in the industry it would be to reform the attitude that project managers, team leads and developers have to estimation. Estimates should be treated by all stakeholders as just that, they are not cast iron commitments written in blood. They should be based on evidence (dude we go through six stages of peer review on every task, why did you give an estimate of 10 minutes?) and when the evidence is demonstrating that our estimates are wrong, then remaining estimates should be re-calibrated. All this is only possible if managers and developers are prepared to create a culture where realistic and responsible estimation, combined with predictable delivery is championed over masochistic estimation and constant failures to hit deadlines.

One final point, when your mate writes "it's a good idea to keep new features *out* of QA for as long as possible", I totally disagree! :-) I'd want feedback from the QA team as soon as possible. The shorter the feedback cycle the better and in an ideal world the QA team would be writing automated acceptance tests in parallel with my development efforts. I'm a firm believer in the agile principle that "Working software is the primary measure of progress" and feedback from QA I'm just guessing that the software I'm writing is actually working.

Sunday, 22 November 2015

ColdFusion: request for transparency and inclusion from Adobe

A few days back I indicated some ire that a (IMO) wayward ER had been implemented for ColdFusion 12: "ColdFusion: a piece of functionality should do one thing, and do it well". Poor old David Epler who raised it seems to feel a bit put upon cos it was something he raised three years ago, and with no further consultation Adobe have now implemented a solution. Without indication it was going to happen, or any communication with David about it, who now think it's perhaps not as good an idea as he originally might have thought.

This also got me thinking about another new feature in ColdFusion 12: this whole ordered / sorted structs carry on (see "ColdFusion 12: ordered/sorted structs"). Where's the ER for this one? I can't find one. So where did the impetus to do this work come from? Did Adobe just decide to do it off their own bat? If so why? Why the heck did they decide to do that work? Instead of any of a number of other features the community have actually asked for. Obviously there are probably some back channels via which people can ask for ERs - although there shouldn't be - but even if this came from some PHB or one of his minions at one of Adobe's special enterprise clients... why does this mean this is what gets implemented? All Adobe ought to say to the PHB et al in this case is "raise the ticket, engender some interest from the community, and let's see what people think".

Conversely the ColdFusion Team might have come up with this themselves. They really shouldn't do that, as - as far as I can tell - they are completely ignorant of CFML usage (I judge this based on everything they do and say about CFML, basically), so their decisions are not informed ones. But by accident they might have a decent idea... in which case they should... raise the ticket, engender some interest from the community, and let's see what people think. They definitely are not qualified by themselves to make decisions as to the direct on CFML.

I'm on the ColdFusion Pre-release Programme, and am under NDA to not divulge or discuss anything that gets mentioned on said programme (some people claim the PR has some sort of Fight-Club-esque rules about even mentioning it, or participation therein, but this is not true. They just want to make themselves come across as being "special". They do... but not in the way they think). So I won't. However I can speak freely on topics that have not had any discussion at all. And two of these are:

  1. Dave's issue about adding encoding functionality to <cfoutput>. This has been apparently implemented without any mention whatsoever on the pre-release.
  2. This sorted / ordered struct stuff. All I can say about that is that it exists in ColdFusion 12. And there has been no discussion about its suitability or necessity for ColdFusion 12 at all. Make of that what you will.
This is absolutely the wrong way of going about things. The ColdFusion Team might be fine Java developers (hey, they might be), but they seem woefully uninformed about CFML, and - as I have said already - don't use it, and don't really seem to have much of a handle on how we use it. They should not be making opaque decisions as to what goes into it. They have a CAB and a Pre-Realease Programme for this, and these decisions should be made there. And any work that is planned to be undertaken should be discussed out in the open, on the public bug tracker. An ER should be raised, public consultation should be allowed, and then the plan of how (or "if") the functionality will be undertaken should be proposed in public for further discussion. Not all opinions should necessarily be given equal weight (there's some bloody stupid opinions out there; some of them are my own), but they should be at least heard.

Here's an example of how it should be done, Here's the proposed schedule and feature-set for Java 9. There's no reason Adobe could not do that, at least at this high level. They could caveat the timeline with "all things being equal, and subject to change due to unforeseen circumstances", and they could caveat the feature list with "this is not a promise, but it's a guideline, and is also subject to change". We're all grown-ups and we understand things might prove tricky or not worth it, or something else might come along that's more important and needs to bump one of the "nice to have" features. And timeframes can change, etc. There's not even any commercial sensitivity to this, as ColdFusion isn't competing with anything. Well, other than "itself and its community" at times, I think.

PHP has similar timelines and have pretty organised RFC documentation these days for new features. I presume other language projects - well successful language projects - are similar.

Even LAS (those behind the Lucee CFML project, and the .lucee language) are heading in that direction, albeit in a more casual sort of way... but they're still finding their feet so it will probably round out as time passes.

I think the chief reason the ColdFusion Team don't take an inclusive communicative approach is that "inclusion" and "communication" are just devoid from their collective psyche, for whatever reason. They seem to treat their community with a degree of contempt that they seem to be "handing down manna from heaven" when they give us a ColdFusion release.

I think it's fine that Adobe marshal suggestions from their own team and from the "dark" part of their user base. But then they should open that up to the rest of the community so we can put our oar in. We have a great wealth of expertise in CFML usage to gauge how a feature might fit, and we've also got a great resource in the form of Sean Corfield who actually has designed languages in the past, so kinda knows how these things work (others in the CFML community might have experience here too, but I don't know about it). There's a few really old hands from the CAB and PR programme still around too, and they/we are kinda used to discussing how CFML should come together too.

Adobe have done a chunk of fine work in CFML, this is for sure. But they've also done some woeful shite, which I can't help but think never would have happened if they actually engaged their user base. As far as I can see there's no published plan for ColdFusion 12, so I suspect there isn't really one. So it's not too late for them to kinda organise themselves a bit more, and include the rest of us in the language planning process. This would be bloody easy. For any public ER out there, simply tell us on Twitter or on Slack or on their own blog that they're looking at implementing it. And then encouraging people to have input. If there's not a public ER... create one, then do the same. Adobe have a vast pool of free planning resource. They should be using it.

Start using it.


Friday, 20 November 2015

ColdFusion: a piece of functionality should do one thing, and do it well

One of my pet hates is having a function called doX(), and doX() does X, but it also does Y.

An example of this is CFML's isValid() function which not only checks validity - as one might expect - but also trims any string values its validating first. The function is not called isValidAfterTrim(), so it should not do the trimming. If one wants to trim a value before validating it... call trim() then isValid(). Simple. Another example is <cfdump> which picked up and abort attribute along the way. This was because some mungbean said "well usually I want to dump and abort, so why can't dump just abort too". And for some stupid reason Adobe went "oh yeah... let's do that". No. If you want to abort after you dump... do an abort after your dump. Don't change <cfdump> to be <cfdumpandabortifyoulike>. That's dumb. At least the Lucee ER to have <cfinclude> also do an abort got rejected ("Add optional abort Attribute to cfinclude"). FFS.

The reason to this is to keep units of functionality small, for starters. It makes them easier and more coherent to test. Also the more functionality one piles into a function (and for this purpose, a tag is just a function that looks funny ;-), the more potential for it being broken as it's maintained. Plus it makes code less clear and less clean.

A function should do one thing, and do it well.

Anyway, today I got wind of this ER: "encodeFor attribute for cfoutput, writeOutput". The intent is honourable, but it suffers from trying to do too much. The gist of it is:

While ColdFusion 10 added the various ESAPI encodeFor* functions, it is dependent upon the developer to properly wrap location where used with the appropriate function (e.g. <cfoutput>#EncodeForHTML(</cfoutput> ). Adding an attribute encodeFor negates the need for wrapping individual variables and would process the entire block contained within <cfoutput>cfoutput>cfoutput> for anything within #'s with the appropriate ESAPI EncodeFor* function specified.

On the face of it this sounds reasonable. It still smacks of "doing too much", so I echoed my position as much.

I sometimes doubt I'm being too dogmatic about these things, so I asked on the #CFML Slack channel, and John Whish came up with a great counter example:

<cfoutput encodeFor="HTML">
    <a href="whatever.cfm?" onclick="get(">Go to whatever.cfm?</a>

What's this gonna do? Encode all the expressions within it for HTML. Which is only correct for should be encoded for URL, JavaScript and URL for each of its three respective uses. However a less than discerning dev might not "get" that, and now their output is encoded incorrectly.

So what we'd need to then do is this:

<cfoutput encodeFor="HTML"></cfoutput>
    <a href="whatever.cfm?id=<cfoutput encodeFor="URL"></cfoutput>" onclick="get(<cfoutput encodeFor="JavaScript"></cfoutput>)">Go to whatever.cfm?id=<cfoutput encodeFor="URL"></cfoutput></a>

What a mess. What we could do is this:

    <a href="whatever.cfm?id=#encodeForUrl(" onclick="get(#encodeForJavaScript(">Go to whatever.cfm?id=#encodeForUrl(</a>

Look familiar? Yeah... we already have the correct (and safe) implementation in place.

We don't need to mess with something that's supposed to just output, and make it do something different as well. It's a bad approach to designing the language.

I reckon this ER should be pulled. It's a pity the work has already been done...



ColdFusion: help one of our community members with a PDF problem

One of our community members, Prabha (I dunno the rest of their name), is having some problems with PDFs. They came to me for help, but I don't know the first thing about PDFs nor how to manipulate them with ColdFusion. And to be completely honest, I am completely content maintaining that ignorance. But I know a bunch of people out there do know how to do this stuff, so they might be able to help.

The question is this one, on StackOverflow: "When exporting PDF with CFChart images in ColdFusion It shows embedded font error". I can't actually make head or tail of it, but there seems to be two things:

They're getting this error:

"cannot extract the embedded font 'PCBOHZ + TimesNewRomanPS-BoldMT. some characters may not display or print correctly ( OR ) cannot extract TimesNewRomanPS-BoldMT"

And clearly they'd rather not.

Secondly there's something about images missing:

In the generated PDF the images are showing as red cross marks, while creating PDF, CFDOCUMENT makes HTTP URL calls to coldfusion server to get the images from the CF virtual folder CFIDE/CFSERVLET (because these images are saved in this folder by cfchart tags based on the charting settings in CF administrator) [...]
It's all greek to me.

Now these should be two different questions on StackOverflow, but... err... shrug.

If there are any ColdFusion/PDF experts out there, can you have a look-see and try to help? Cheers.



Thursday, 19 November 2015

... and... Adobe hot fixes it

Just a quick follow-up to this morning's article: "CAUTION: Latest ColdFusion 11 patch breaks (at least some) code using "/>" in CFML tags"

Well knock me over with a feather... they're released a hot-fix for it! How good is that?

It's attached to the ticket: "CFPOP doesn't create the query given by name="" with updater 7 installed".

I still think they need to withdraw update 7, bake that hot fix into it, and release it again (as update 8), but at least this should get Tom going.

I haven't tested it as I'm behind a firewall here and cannot get to a POP server, but I'll report back when I have confirmation it actually fixes the problem.

Good snappy work there Adobe. And good to see it's possible for you to release individual hot fixes.



Guest author: Why good developers write bad code

A while back one of my mates said there was a topic they had some thoughts on, and figured they should write a blog article about it. Their problem being they didn't have a blog. This seemed like a good way for me to get someone else to do my work for me, so I raised my hand to host it for them here. I've been waiting for... god... oh about three months for this. But 'ere' 'tis:

So there's a guy on your team, and he's a good developer. He knows the language inside out, he's up-to-date with all the latest technologies, he spots design patterns and code smells, and when you send him your code to review he cuts you a new asshole (and it turns out he's bloody right about everything)

... but then he gets a chunky piece of work and, more often than you'd expect, it goes astray. He's swearing at his screen, he's "almost done" for ages. Because it's late the code reviews are lighter than they should be, and QA raise a ton of bugs. He says they're all small but each one takes longer than the next to fix. When you try and spread out the bugfixing among the team it takes everyone else even longer so he ends up slogging through everything on his own. Eventually it's done and it works, but it's brittle and everyone is afraid to touch it. He blames it on over-complicated requirements and over-ambitious timelines and we all move on.

Maybe it was The Business's fault for coming up with stupid requirements, or just the fact that smart people can make mistakes in ingenious ways. Everyone gets unlucky now and again, but if a competent developer gets bogged down regularly then there's something else going on. What is it?

Good devs are often tinkerers, and are very good at figuring out ways to work around obstacles. The dark side of this is that obstacles can be seen as things-to-be-worked-around rather than problems to be solved.

At the code level this manifests as a reluctance to refactor. Tests make refactoring safer and easier, but even with good tests developers often see existing code as fixed, something to be built upon rather than restructured. The upshot of this is you get layers and layers of code, each layer adding a missing piece or working around a bad design decision in the one below, and the complexity quickly gets out of hand. This gets especially bad when time is tight, and guys don't want to go back to the start, and so treat their own almost-but-not-quite working code from a few days ago as immutable (especially after their feature has been through QA) - essentially they get into trial-and-error mode, and nothing gets re-thought, so underlying issues with their code design don't get fixed.

You also see it one level up from the code - when confronted with an over-complicated set of requirements, a capable developer will often just implement them as they are, logical contradictions and all. He'll bitch and moan, but will very seldom chase down a bad requirement and get it (or his interpretation of it) fixed so that it makes sense. We all bang on about elegance, but the requirements are the soil the code grows in, and without taking some responsibility for them your code cannot ever be consistently good.

There's no easy way to fix this. It's hard enough to find programmers capable of writing good code, harder still to find ones that consistently do so. If this is you, write tests you can trust then refactor refactor refactor. If for some reason you're not doing TDD then refactor anyway. If you've coded yourself into a corner create a new branch and work out a new approach in that. Be bold. It's just code, it's not set in stone, and neither are your requirements, so stop your moaning and take responsibility for them. And stay out of trial-and-error mode as much as you can.

If this is someone on a team you lead, TDD and code reviews (especially if they're done early enough to prevent a dev going too far down the wrong path) will help, also because bugs are often fixed by trial and error it's a good idea to keep new features *out* of QA for as long as possible. Really though there's no magic bullet - you'll just have to know who's susceptible to this kind of thing, and keep an eye out for it.

Yep, very true. I encounter this challenge all the time, and indeed sometimes I self-identify as having this problem too. I suppose we all do/did at some point.