Need help reducing cyclomatic complexity

Question

I've got a method called UpdateUserDevices (UserModel). The UserModel contains a List<DeviceModel> which associates a list of devices with a specific user. (One-to-many).

When I call the method, everything is working as excpected, however it's quite complex with nested loops and if statements.

What would be a good pattern to reduce the cyclomatic complexity on this? I thought about "CoR" but that might be overkill.

private void UpdateUserDevices( UserModel model )
{
    // Get users current devices from database
    var currentDevicesFromDatabase = _deviceRepository.FindByUserId( model.Id ).ToList();

    // if both the model devices and the datbase devices have records
    // compare them and run creates, deletes, and updates
    if( model.Devices.Any() && currentDevicesFromDatabase.Any() )
    {
        var devicesToAdd = model.Devices.Exclude( currentDevicesFromDatabase, d => d.Id ).ToList();
        var devicesToDelete = currentDevicesFromDatabase.Exclude( model.Devices, d => d.Id ).ToList();
        var workingDevices = model.Devices.Union( currentDevicesFromDatabase );

        foreach( var device in workingDevices )
        {
            // Add devices
            if( devicesToAdd.Contains( device ) )
            {
                _deviceRepository.Create( device );
                continue;
            }

            // delete devices
            if( devicesToDelete.Contains( device ) )
            {
                _deviceRepository.Delete( device );
                continue;
            }

            // update the rest
            _deviceRepository.Update( device );
        }
        return;
    }

    // model.Devices doesn't have any records in it.
    // delete all records from the database
    if( !model.Devices.Any() )
    {
        foreach( var device in currentDevicesFromDatabase )
        {
            _deviceRepository.Delete( device );
        }
    }

    // database doesn't have any records in it
    // create all new records
    if( !currentDevicesFromDatabase.Any() )
    {
        foreach( var device in currentDevicesFromDatabase )
        {
            _deviceRepository.Create( device );
        }
    }
}

Answer 1

It might be that I don't understand exactly what happens, but it seems to me like you could simplify it a lot by removing all your outermost if statements and just performing the topmost codeblock.

private void UpdateUserDevices ( UserModel model )
{
    // Get users current devices from database
    var currentDevicesFromDatabase = _deviceRepository.FindByUserId( model.Id );

    var devicesToAdd = model.Devices.Exclude( currentDevicesFromDatabase, d => d.Id ).ToList();
    var devicesToDelete = currentDevicesFromDatabase.Exclude( model.Devices, d => d.Id ).ToList();
    var workingDevices = model.Devices.Union( currentDevicesFromDatabase ).ToList();

    foreach ( var device in workingDevices )
    {
        if ( devicesToAdd.Contains( device ) )
        {
            // Add devices
            _deviceRepository.Create( device );

        }
        else if ( devicesToDelete.Contains( device ) )
        {
            // Delete devices
            _deviceRepository.Delete( device );

        }
        else
        {
            // Update the rest
            _deviceRepository.Update( device );
        }
    }

}

Actually the foreach could be split into three separate with no nested ifs.

private void UpdateUserDevices ( UserModel model )
{

    var currentDevicesFromDatabase = _deviceRepository.FindByUserId( model.Id );

    // Take the current model and remove all items from the database... This leaves us with only records that need to be added.
    var devicesToAdd = model.Devices.Exclude( currentDevicesFromDatabase, d => d.Id ).ToList();

    // Take the database and remove all items from the model... this leaves us with only records that need to be deleted
    var devicesToDelete = currentDevicesFromDatabase.Exclude( model.Devices, d => d.Id ).ToList();

    // Take the current model and remove all of the items that needed to be added... this leaves us with only updateable recoreds
    var devicesToUpdate = model.Devices.Exclude(devicesToAdd, d => d.Id).ToList();

    foreach ( var device in devicesToAdd )
        _deviceRepository.Create( device );

    foreach ( var device in devicesToDelete )
        _deviceRepository.Delete( device );

    foreach ( var device in devicesToUpdate )
        _deviceRepository.Update( device );

}