Tuesday 4: Larry Garfield, "This code stinks!"

 

Code smells

1 The word AND

Solrphpclient addDocuments

impossible to render out Solr documents without sending them to Solr

atomic actions

 

God objects - they do more tha one thing and know too much.

Use composition

 

2 The word OR

Sometimes do one thing, sometimes do another.

_registry_check_code

type argument is sometimes a type and sometimes a flag

second argument is polymorphic too

what the hell does this function do

write to functions

make object oriented so they can share data

[doesn't solve the staticness of e.g. a REQUEST-persistent cache

3 Complexity of logic

comment node view - insane conditional structure. Cyclomatic complexity: how many different routes through the code. Depends on an unstructured object and a string. Unit test THAT if you can!

"If you need more than three levels of indentation, you're screwed anyway and should fix your program." Linus Torvalds

Run-time type identification - naive identity of some entity types. Instead a registry of entity-type callbacks - that's what core does. But it's slow. Entities should have a label() method: polymorphism.

4 Units and unit testing

DrupalWebTestCase treats 1 unit as 1 Drupal install. THIS IS NOT A UNIT TEST. It's a system test, which is nice, but bugs can cancel each other out.

DrupalUnittestCase - use this instead. 1000 times faster.

More testable

avoid globals and statics

Dependency injection - pass in what all your function needs

Minimize singletons - hard dependencies like function calls

5 Documentation

You can't teach what you don't know, and if you can't document it, neither you nor I know what you're doing.

Lack of comments - laziness, lack of comprehension, indifference, embarrassment

What to document - function, method, class, object property, constant, parameter. No exceptions

6 inappropriate intimacy / tight coupling

Can't change code x without coding y

What are the knock-on effects when implementation details change? Probably a 400k patch.

If you expose implementation details, this breaks the API. People will rely on it

Form API

render API / hook page alter

Field / language API

Node API

Implementation details hidden by query objects

Solution

interfaces - document them - well documented interfaces

[Is there's any way of doing AOP in PHP without strong coupling?]

7 impurity

Pure function - deterministic with no side effects

If you sant impure functions, then do so, but separate out as much pure as possible.

Drupal theme initialise

solution is to refactor as OO

 

"Can't I do anything right?"

Good smells

single purpose

self contained

predictable

repeatable

unit testable

documented