Why We Should Avoid Side Effects

I have recently come across a very bad piece of code. Here is the idea. It is a service that basically takes camera stream configuration, takes snapshot and saves the stream + snapshot + some additional info for further use. This service is called every time a new stream is added or when the stream needs to be updated.

Here is the roughly sketched idea.

CameraService

@Service
public class CameraService() {

    @Autowired
    private CameraRepository cameraRepository;

    @Autowired
    private SnapshotService snapshotService;

    public Camera updateCamera(CameraConfiguration configuration) {

        Camera updatedCamera = cameraRepository.findByUuid(configuration.getUuid());
        String newSnapshot = snapshotService.takeSnapshot(configuration);

        updatedCamera.setSnapshot(newSnapshot);

        // ... Some more updates based on the configuration

        return cameraRepository.save(updatedCamera);
    }
}

Taking snapshot took quite a while so we decided to take the snapshot only in case the stream url has changed to spare some time.

CameraService

@Service
public class CameraService() {

    @Autowired
    private CameraRepository cameraRepository;

    @Autowired
    private SnapshotService snapshotService;

    public Camera updateCamera(CameraConfiguration configuration) {

        Camera updatedCamera = cameraRepository.findByUuid(configuration.getUuid());

        if(needsNewsnapshot(updatedCamera, configuration)) {
            String newSnapshot = snapshotService.takeSnapshot(updatedCamera,configuration);
            updatedCamera.setSnapshot(newSnapshot);
        }

        // ... Some more updates based on the configuration

        return cameraRepository.save(updatedCamera);
    }

    private boolean needsNewSnapshot(Camera camera, CameraConfiguration configuration) {
        // Check if the camera needs new Snapshot based on updated configuration
    }
}

Everything should have worked perfectly but, alas, it did not. The updated camera started to fail in case one specific configuration part changed. It was due to unexpected behaviour of the SnapshotService. One would say that this service should take a snapshot of a camera stream. Well, one of my predecessors had probably thought otherwise.

SnapshotService

@Service
public class SnapshotService {

    @Autowired
    private MetadataService metadataService;

    public void takeSnapshot(Camera camera, CameraConfiguration configuration) {

        String snapshot = this.base64Snapshot(configuration.getStreamUrl());

        camera.setmetadata(metadataService.getMetadata(configuration)); 1

        return snapshot;
    }

    private String base64Snapshot(Url url) {
        // ... Do some magic to get the base64 encoded picture.
    }
}

Silently changing state of the camera instance. Who would have thought that the takeSnapshot method would not only take a snapshot and return it as a String but also add a bonus of silently changing the state of a referenced object?!

A slightly better design might look like this:

CameraService

@Service
public class CameraService() {

    @Autowired
    private CameraRepository cameraRepository;

    @Autowired
    private MetadataService metadataService;

    @Autowired
    private SnapshotService snapshotService;

    public Camera updateCamera(CameraConfiguration configuration) {

        Camera updatedCamera = cameraRepository.findByUuid(configuration.getUuid());

        if(needsNewsnapshot(updatedCamera, configuration)) {
            String newSnapshot = snapshotService.takeSnapshot(updatedCamera,configuration);
            updatedCamera.setSnapshot(newSnapshot);
        }

        updatedCamera.setMetadata(metadataService.getMetadata(configuration));

        // ... Some more updates based on the configuration

        return cameraRepository.save(updatedCamera);
    }
}

SnapshotService

@Service
public class SnapshotService {

    public void takeSnapshot(Camera camera, CameraConfiguration configuration) {
        return this.base64Snapshot(configuration.getStreamUrl());
    }
}