Saturday, December 5, 2009

Three heresies

I encourage people to think for themselves rather than following cargo cults. You might or might not agree with the three heresies I've written about here, but do at least think about them.

Public fields

In Java code, instead of having a public getter and public setter for a field, why not just make the field public? It's much simpler and less code. If you later need a getter and setter for some reason you can always refactor to that (and many IDEs will give you help doing it). There is a comment by Richard Gomes at the end of this previous article on the subject of public fields for data objects. I think public fields make most sense for NOJOs (data objects) (in the rare case where a NOJO is useful - not very often) but maybe it would sometimes make sense for other sorts of classes too?

Note that having public access to a field is not what I'm trying to encourage. The point I'm making is that if you do have public access to a field then it doesn't matter much whether it is by getter/setter or making the field public, so you might as well use a public field as it is simpler. (But please, tell don't ask instead.)

Magic values instead of constants (in build files)

Instead of always factoring out magic values as properties in your build file, consider just using the magic value where it makes sense. For example, maybe instead of "${src}" just use "src" (and get rid of the property) - this was suggested by Jeffrey Fredrick at CITCON Paris 2009. I think there is a lot of merit in this approach. What are the chances that you'd be able to just modify the value of the "src" property and everything would still work? Probably quite low - you'd probably do a text file search for "src" anyway. What are the chances that you'll want to change it anyway? I think it's worth thinking about whether it's better or worse to factor out constants in some cases.

Make the CI build fail fast rather than run all the tests

Rather than running all the tests in your CI build, how about have the build fail as soon as any test fails? That way, a failing build uses less of your build farm's capacity. If your build farm capacity is limited, then this approach may result in getting a passing build sooner (as when the fix is committed there may be a build agent available for running the build with that commit sooner because it's time isn't being taken running a build which will eventually fail anyway). I think it's often more important to know which commit broke the build than which tests failed in order to know both who should fix the build and what caused the build breakage. This approach might not be so good if you have a flickering build (i.e. randomly failing tests) - however, making the build reliable can be achieved and is worthwhile anyway.

More heresies to follow

I have other heresies to write about. Please suggest your own in the comments.

Copyright © 2009 Ivan Moore


Philip Schwarz said...

In Effective Java 2nd Edition (Item 14 - p71), Joshua Bloch says that when you dispense with accessor methods, and make a member field directly accessible to reduce visual clutter, you give up the benefits of encapsulation: you can't change the representation of the field without changing the API, you can't enforce invariants on the field, and you can't take auxiliary action when a field is accessed.

He says that this trade-off is:

1. Never acceptable for mutable fields of public classes

2. Less harmful, though still questionable, for immutable fields of public classes

3. Sometimes desirable for mutable and immutable fields of package-private or private classes

You ask: public fields make most sense for NOJOs, but maybe it would sometimes make sense for other sorts of classes too?

Is there any value in re-framing your question factoring in aspects like whether the classes in question are public or private, whether the fields in question are mutable or immutable, and whether a future need to change the representation of the fields is possible/likely?

Ivan Moore said...

Hi Philip, many thanks for the comment. I probably should have been more specific - I was meaning the most heretical case - i.e. mutable fields of a public class.
I understand Bloch's orthodoxy. Nevertheless, I will not denounce my heresy. If you need a getter/setter to do more than just get/set then you can always refactor to replace the field with getter/setter, so why bother with the extra code unless you need it?
I don't feel compelled to follow Bloch or anyone else's orthodox edicts. If the team don't like it then that's a different matter. My current team are happy to allow freethinking so we discuss things like this and I'm happy to go with the collective view - what matters is that we have thought about it.

keithb said...

In situations where I am forced to use arrays (rather than a less powerful aggregate, which I prefer) I'll sometimes bind the array to a singular name, like this:

Foo[] foo

and not a plural ("foos") so that when I dereference through the array to get a single element the use looks most natural

aFoo = foo[3]

this is a specific case of a general principle of writing natural looking code and then working backwards to define things (variables, classes, methods, functions, whatever) so that what I wrote works. This as opposed to thinking up classes, methods, functions and what have you based on what they "are" and then using them as best I can to do what I need.

dafydd said...

I propose using the old-fashioned JUnit 4 form for writing tests instead of the JUnit 5 @Test annotation in cases where you're writing lots of little tests.

Adding @Test adds an extra line to each test - which can mean much more scrolling and verbosity (as though Java wasn't verbose enough already).

I've never had a problem prefixing my test methods with the word "test" - but I do notice the extra scrolling and typing with @Test... ;-)