gosingle-responsibility-principle

Extending an enum by adding a custom case without messing up the external package


Original code

The Go image package defines a method for its Gray image:

func (p *Gray) ColorModel() color.Model { return color.GrayModel }

https://github.com/golang/go/blob/96d8ff00c2d6a88384863a656fb5e53716b614d3/src/image/image.go#L853

Then the ColorModel() is used inside a switch to set the cb:

        switch m.ColorModel() {
        case color.GrayModel:
            e.cb = cbG8 // <-- cb is set **
        case color.Gray16Model:
            e.cb = cbG16
        // ...
        }

https://github.com/golang/go/blob/96d8ff00c2d6a88384863a656fb5e53716b614d3/src/image/png/writer.go#L633

Then, eventually, the cb is used to set the bitsPerPixel variable:

    bitsPerPixel := 0

    switch cb {
    case cbG8:
        bitsPerPixel = 8
    case cbTC8:
        bitsPerPixel = 24
    case cbP8:
        bitsPerPixel = 8
    case cbP4:
        bitsPerPixel = 4
    case cbP2:
        bitsPerPixel = 2
    case cbP1:
        bitsPerPixel = 1
    case cbTCA8:
        bitsPerPixel = 32
    case cbTC16:
        bitsPerPixel = 48
    case cbTCA16:
        bitsPerPixel = 64
    case cbG16:
        bitsPerPixel = 16
    }

https://github.com/golang/go/blob/96d8ff00c2d6a88384863a656fb5e53716b614d3/src/image/png/writer.go#L313

Question

How to add a new case for 3 bits per pixel to the Go source code of the image package while keeping it simple and following the single responsibility principle. While avoiding messing up the whole image package?

Maybe this question is too broad, but I'm looking for innovative ideas to make the process less painful.


Solution

  • I think you don't have more options than changing all the related files in the Go image package in a single commit, so you don't break anything between commits: that is, a commit that includes your whole improvement (3 bits per pixel image) and ensures nothing else breaks (other codecs besides png, for example).

    The single responsibility principle here should be taken as "single feature", "single improvement", and not "single method", "single file" changes. So in this case, adding "3 bits per pixel image" support is a single responsibility regardless of whether you have to change one or multiple files.

    Gather together the things that change for the same reasons. Separate those things that change for different reasons.

    All switches consuming ColorModel and cb should have a safe default case, so introducing a new ColorModel doesn't break anything. You should make sure that's the case in all uses inside the standard library. On the other hand, cb is a private member so its use is reduced to the package itself.