0

我有一个方法叫做UpdateUserDevices (UserModel). UserModel包含将设备List<DeviceModel>列表与特定用户相关联的 。（一对多）。

当我调用该方法时，一切都按预期工作，但是嵌套循环和 if 语句非常复杂。

什么是减少圈复杂度的好模式？我想到了“CoR”，但这可能有点矫枉过正。

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 );
        }
    }
}
4

3 回答 3

1

可能是我不明白到底发生了什么，但在我看来，您可以通过删除所有最外层的 if 语句并只执行最顶层的代码块来简化它。

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 );
        }
    }

}

实际上，foreach 可以分成三个独立的，没有嵌套的 if。

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 );

}
于 2013-10-14T20:27:25.960 回答
0

continue 语句是高圈复杂度的原因

代替

if( devicesToAdd.Contains( device ) )
{
   _deviceRepository.Create( device );
   continue;
}

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

有类似的东西

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

然后你也可以删除 continue ，这有点代码味道。

使用 else-if 可以通过您的方法进行的组合可能更少（路径更少），至少从圈复杂度分析器 sw 的角度来看。

一个更简单的例子：

if (debugLevel.equals("1") {
    debugDetail = 1;
}
if (debugLevel.equals("2") {
    debugDetail = 2;
}

这段代码有 4 条路径，每条路径都将复杂度乘以 2。从人类智能来看，这个例子看起来很简单。

对于复杂度分析器，这更好：

if (debugLevel.equals("1") {
    debugDetail = 1;
} else if (debugLevel.equals("2") {
    debugDetail = 2;
}

这现在只有 2 条可能的路径！您使用人类智能看到这两个条件是互斥的，但复杂性分析器仅在使用 else-if 子句的第二个示例中看到这一点。

于 2013-10-14T20:22:08.290 回答
0

我要做的就是分解方法。您正在尝试做一些事情，以便我们可以将它们分开。将循环条件移动到变量中（可读性）。检查边缘情况，无论是空的，首先。将删除移至其自己的方法（可读性/可维护性）。将 main（复杂逻辑）移至其自己的方法（可读性/可维护性）。UpdateUserDevices 现在看起来很干净。

主要方法：

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

    $isUserDevicesEmpty = !model.Devices.Any();
    $isRepositoryEmpty = !currentDevicesFromDatabase.Any();

    if($isRepositoryEmpty || $isUserEmpty){
        // Missing One
        if($isRepositoryEmpty){
            this.deleteEmpty(currentDevicesFromDatabase);
        }

        if($isUserDevicesEmpty){
            this.deleteEmpty(model.Devices);
        }
    }
    else{
        this.mergeRepositories(currentDevicesFromDatabase, model);
    }

    return;
}

Merge Repos Method：原来方法的肉土豆现在有了自己的方法。这里发生的事情。删除workingDevice以添加devicesToUpdate（直接与间接逻辑 => 采用联合然后对所有元素执行包含与执行一次相交相同）。现在我们可以有 3 个单独的 foreach 循环来处理更改。（您正在避免contains为每台设备执行此操作，也许还有其他效率提升）。

private void mergeRepositories(UserModel model, List currentDevicesFromDatabase)
{
    // if both the model devices and the datbase devices have records
    // compare them and run creates, deletes, and updates

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

    foreach( device in devicesToAdd ){
        _deviceRepository.Create( device );
    }

    foreach( device in devicesToDelete ){
        _deviceRepository.Delete( device );
    }

    foreach( device in devicesToUpdate){
        _deviceRepository.Update( device );
    }

    return;
}

删除空方法：这里没什么可看的

private void deleteEmpty(List devices){
    foreach( var device in devices )
    {
        _deviceRepository.Delete( device );
    }

    return
}

现在它又好又简单。反正我是这么想的。（为了在 Ebay 上列出项目，我不得不做类似的事情。我实际上写的是一个稍微不同的版本，没有在整个集合中使用 Intersect，但这既不是这里也不是那里）

于 2013-10-14T21:09:10.930 回答