OnStartApplicationsCommand function

Question

The foreach loop seems ridiculously lengthy and I am trying to get rid of it altogether is possible. But there are a lot of things happening in there which I need to know in order to update the UI and create other values. For examples, variables mappedRuntimeName, mappedDeviceName are used for updating the UI and also for creating the RuntimeDetailsobject. The DownloadApplications function also seems to have a lot of parameters.

Does anyone have any suggestions on how to improve the shown OnStartApplicationsCommand function?

public bool? OnStartApplicationsCommand()
{
    var runCommandSuccessful = true;

    //get all the services for download begin
    ResolveAndUpdateDependencies();

    CancelOperationCommand = new AsyncDelegateCommand(StartApplicationsCommand.Cancel, () => !StartApplicationsCommand.CanExecute(null));
    ProjectCommands.CancelOperation.RegisterCommand(CancelOperationCommand);

    _eventAggregator.GetEvent<OutputWindowSourceChangedEvent>().Publish(OutputWindowSourceType.Run);

    var activeConfiguration = _configurationService.GetActiveConfiguration();

    // Message: >>>>>> Downloading and running applications...
    PostOutputMessage(string.Format(ForteDownloadMessages.Default.download_level1_info,
                                        _projectManagerService.GetProjectName(),
                                            _configurationService.GetActiveConfiguration().Name), 
                        OutputWindowMessageType.Info);

    var count = 1; //application count
    RuntimeDetails lastRuntimeDetails = null; //remember (for disconnecting) where the last application was downloaded to
    DownloadedApplications.RemoveAll(application => true); //just remove all 

    foreach (var application in activeConfiguration.Applications)
    {
        if (StartApplicationsCommand.CancellationRequested)
        {
            ProjectCommands.CancelOperation.UnregisterCommand(CancelOperationCommand);
            return null;
        }

        //retreive the fully qualified runtime name of the form deviceName.runtimeName from the application
        var fullRuntimeName = application.MappedRuntimeName;
        Debug.Assert(fullRuntimeName != string.Empty, "fullRuntimeName != string.Empty");

        var mappedDeviceName = fullRuntimeName.Split('.').First();
        var mappedRuntimeName = fullRuntimeName.Split('.').Last();

        var matchedConfigurationDeviceModel = _configurationModelProvider.
            GetConfigurationDeviceModelFromFullRuntimeName(fullRuntimeName);

        var ipAddress = matchedConfigurationDeviceModel.IPAddress;

        //retrieve the device model from the device service
        var mappedDeviceModel = 
            _componentModelProvider.GetDeviceModelFromDeviceType(matchedConfigurationDeviceModel.Type);

        //retrieve the mapped runtime from the device model
        var matchedConfigurationRuntimeModel =
            _configurationModelProvider.GetConfigurationRuntimeModelFromFullRuntimeName(fullRuntimeName);

        var port = Convert.ToInt32(matchedConfigurationRuntimeModel.Port);

        var runtimeDetails = new RuntimeDetails()
        {
            DeviceName = matchedConfigurationDeviceModel.Name,
            Platform = mappedDeviceModel.Platform,
            RuntimeType = matchedConfigurationRuntimeModel.Type,
            RuntimeName = matchedConfigurationRuntimeModel.Name,
            Ip = ipAddress, 
            Port = port
        };

        var applicationInstance = _componentModelProvider.GetApplicationModelFromApplicationType(application.Type);

        //need to close connection when switching runtimes
        //Do this before assigning the request executor so that disconnect is not skipped when runtimes
        //are switched
        if (lastRuntimeDetails != null) //at least one application was already downloaded
            if (lastRuntimeDetails.Ip != runtimeDetails.Ip || 
                    lastRuntimeDetails.Port != runtimeDetails.Port)
                        _requestExecutor?.Disconnect();

        Resource resourceInstance = null;

        switch (matchedConfigurationRuntimeModel.Type)
        {
            case RuntimeType.FORTE:
                resourceInstance = _resourceProvider.GetEmbeddedResource(application.Name + "_EMB_RES");
                _requestExecutor = _forteRequestExecutor;
                break;
            case RuntimeType.FBRT:
                resourceInstance = _resourceProvider.GetPanelResource(application.Name + "_PANEL_RESOURCE");
                _requestExecutor = _fbrtRequestExecutor;
                break;
            case RuntimeType.FZRTE:
                break;
            default:
                break;
        }

        var appSettings = application.Settings;

        //Message: >>> Downloading application: ...
        PostOutputMessage(string.Format(ForteDownloadMessages.Default.download_level2_info,
                                        application.Name,
                                        mappedDeviceName,
                                        mappedRuntimeName,
                                        resourceInstance?.Name ?? "default"),
                          OutputWindowMessageType.Info);

        //first stop monitoring
        Dependency.Resolve<IForteMonitoringService>().MonitoringOn = false;
        //try { _requestExecutor.Disconnect(); } //without disconnect doesn't seem to work on rpi
        //catch { /*swallow: no commhanlder present yet*/ };
        var downloadSuccessful = DownloadApplication(application.Name, applicationInstance, appSettings, 
            matchedConfigurationRuntimeModel, resourceInstance, runtimeDetails);
        //_requestExecutor.Disconnect();

        lastRuntimeDetails = runtimeDetails;

        if (downloadSuccessful == false)
            runCommandSuccessful = false;

        if (downloadSuccessful == null) //if cancellation requested return null and end
            return null;

        DownloadedApplications.Add(new DownloadedApplication()
        {
            Application = application,
            Device = matchedConfigurationDeviceModel,
            RuntimeDetails = runtimeDetails,
            Resource = resourceInstance,
            Running = (bool) downloadSuccessful
        });
        //publish application running event with applications that are running
        _eventAggregator.GetEvent<RunningApplicationStatusChangedEvent>().Publish(DownloadedApplications);
        count += 1; //application count
    }

    _requestExecutor?.Disconnect(); //disconnect from the last runtime
    return runCommandSuccessful;
}

Answer 1

Unused Variables

As it appears, your var count = 1; is not being used anywhere other than at count += 1; but the value is never used for anything. That's two pointless lines if this truly is the case.

Comments

Your comments aren't the most readable (this could just be me). Try adding a space and starting with a capital letter.

    //this is less appealing
    // This is more appealing.

You also have a few comments that just seem pointless. For example,

    DownloadedApplications.RemoveAll(application => true); // Just remove all 

This is no less readable than

   DownloadedApplications.RemoveAll(application => true);

From this line we quickly see "Downloaded Applications Remove all". So does this really add any value to your code? It seems like that comment just adds clutter.

Clutter & Variables used only Once

You want to reduce your for-loop there's a few things you can do aside from the typical suggestions you're going to find (i.e. Modularize into functions).

One of them being reducing the number of declarations you have (if conditions allow). For example, both mappedDeviceName and mappedRuntimeName are only used once. If you know you are only ever going to use them once in this scope/context just remove the declaration and use the value where needed.

Another quick note, I noticed you're declaring applicationInstance, mappedDeviceName, and mappedRuntimeName towards the top but not using them until the bottom half of the for-loop. Why not just declare them right before you use them? That way it's obvious where they are being used.

RuntimeDetails

A key place you can reduce the amount of code in your for-loop is your instance of RuntimeDetails. RuntimeDetails appears to be getting its data from 3 core places: fullRuntimeName, _componentModelProvider, and _configurationModelProvider.

public RuntimeDetails GetRuntimeDetails(string fullRuntimeName, WhateverTypeThisIs configurationModelProvider, WhateverTypeThisIs componentModelProvider)
{
    var matchedConfigurationDeviceModel = configurationModelProvider.GetConfigurationDeviceModelFromFullRuntimeName(fullRuntimeName);
    var matchedConfigurationRuntimeModel = configurationModelProvider.GetConfigurationRuntimeModelFromFullRuntimeName(fullRuntimeName);

    return new RuntimeDetails()
    {
        DeviceName = matchedConfigurationDeviceModel.Name,
        // Retrieve the device model from the device service
        Platform = componentModelProvider.GetDeviceModelFromDeviceType(matchedConfigurationDeviceModel.Type).Platform,
        RuntimeType = matchedConfigurationRuntimeModel.Type,
        RuntimeName = matchedConfigurationRuntimeModel.Name,
        Ip = matchedConfigurationDeviceModel.IPAddress, 
        Port = Convert.ToInt32(matchedConfigurationRuntimeModel.Port)
    };
}

One Problem

Your for-loop wants to use the matchedConfigurationDeviceModel variable later in the DownloadedApplications.Add(...) call. That call specifically wants the whole model, so if it's possible why not just have RuntimeDetails store the whole model? As it stands its purpose is to store data, so have is store all the data it could need then that should reduce the amount of code directly inside your for-loop.

I hope you find these suggestions helpful.

Answer 2

        //need to close connection when switching runtimes
        //Do this before assigning the request executor so that disconnect is not skipped when runtimes
        //are switched
        if (lastRuntimeDetails != null) //at least one application was already downloaded
            if (lastRuntimeDetails.Ip != runtimeDetails.Ip ||
                lastRuntimeDetails.Port != runtimeDetails.Port)
                _requestExecutor?.Disconnect();

When you nest if statements please use brackets.

You should use what is called a null-safe dereference operator like you suggested in the comments,

if (lastRuntimeDetails != null)
{
    if (lastRuntimeDetails?.Ip != runtimeDetails?.Ip ||
        lastRuntimeDetails?.Port != runtimeDetails?.Port)
    {
        _requestExecutor?.Disconnect();
    }
}

A positive to doing it this way, if for some reason the lastRuntimeDetails.Port property is null but the object is not (or the Ip property) this will not throw a Null Reference Exception, I think that is the biggest positive to this.

---

        if (downloadSuccessful == false)
            runCommandSuccessful = false;

        if (downloadSuccessful == null) //if cancellation requested return null and end
            return null;

you could probably turn this into an else/if statement, because if downloadSuccessful is false, then it is not null, no reason to check for something you already know.

if (downloadSuccessful == false)
{
    runCommandSuccessful = false;
}   
else if (downloadSuccessful == null) //if cancellation requested return null and end
{
    return null;
}

on further though you could just assign runCommandSuccessful directly from downloadSuccessful like this

runCommandSuccessful = downloadSuccessful ?? return null;

using the null-coalescing operator

---

Naming here could be better,

        DownloadedApplications.Add(new DownloadedApplication()
        {
            Application = application,
            Device = matchedConfigurationDeviceModel,
            RuntimeDetails = runtimeDetails,
            Resource = resourceInstance,
            Running = (bool) downloadSuccessful
        });

I assume that DownloadedApplications is a list of DownloadedApplication objects. I think that you change the object name to Application then your DownloadedApplications can be a list of Application objects that have been downloaded

Otherwise the term DownloadedApplication gets rather monotonous, and loses its meaning.

---

your port variable here isn't very helpful, it just takes up more memory

        var port = Convert.ToInt32(matchedConfigurationRuntimeModel.Port);

        var runtimeDetails = new RuntimeDetails()
        {
            DeviceName = matchedConfigurationDeviceModel.Name,
            Platform = mappedDeviceModel.Platform,
            RuntimeType = matchedConfigurationRuntimeModel.Type,
            RuntimeName = matchedConfigurationRuntimeModel.Name,
            Ip = ipAddress, 
            Port = port
        };

you should just assign the Port property directly, you might even be able to get away without converting to an integer, but you will have to test that theory yourself. I don't immediately see any reason that you would need to convert that to an integer, it may already be an integer, you should definitely look into this if you are trying to make your code more efficient.

var runtimeDetails = new RuntimeDetails()
{
    DeviceName = matchedConfigurationDeviceModel.Name,
    Platform = mappedDeviceModel.Platform,
    RuntimeType = matchedConfigurationRuntimeModel.Type,
    RuntimeName = matchedConfigurationRuntimeModel.Name,
    Ip = ipAddress, 
    Port = Convert.ToInt32(matchedConfigurationRuntimeModel.Port)
};

---

What I would do to shorten the for loop is to consolidate the variables that you are using intermediately to create your RuntimeDetails object. I touched on this briefly and would like to expand the scope of it

You are also using some variables to assign data to a PostOutputMessage I would get rid of those variables, they are several lines away from where they are used as well, that made it hard to tell what they were for.

You should also get rid of the try catch that is commented out, dead code smells

There are a lot of comments, I removed some of the ones I thought were extraneous.

I also removed the count variable, I didn't see it doing anything.

In your switch statement you have a case that doesn't do anything, I would drop this off and let the default grab that case.

    switch (matchedConfigurationRuntimeModel.Type)
    {
        case RuntimeType.FORTE:
            resourceInstance = _resourceProvider.GetEmbeddedResource(application.Name + "_EMB_RES");
            _requestExecutor = _forteRequestExecutor;
            break;
        case RuntimeType.FBRT:
            resourceInstance = _resourceProvider.GetPanelResource(application.Name + "_PANEL_RESOURCE");
            _requestExecutor = _fbrtRequestExecutor;
            break;
        case RuntimeType.FZRTE:
            break;
        default:
            break;
    }

maybe something like this

foreach (var application in activeConfiguration.Applications)
{
    if (StartApplicationsCommand.CancellationRequested)
    {
        ProjectCommands.CancelOperation.UnregisterCommand(CancelOperationCommand);
        return null;
    }

    //retreive the fully qualified runtime name of the form deviceName.runtimeName from the application
    var fullRuntimeName = application.MappedRuntimeName;
    Debug.Assert(fullRuntimeName != string.Empty, "fullRuntimeName != string.Empty");

    var matchedConfigurationDeviceModel = _configurationModelProvider.
        GetConfigurationDeviceModelFromFullRuntimeName(fullRuntimeName);            

    var matchedConfigurationRuntimeModel =
        _configurationModelProvider.GetConfigurationRuntimeModelFromFullRuntimeName(fullRuntimeName);

    var runtimeDetails = new RuntimeDetails()
    {
        DeviceName = matchedConfigurationDeviceModel.Name,
        Platform = _componentModelProvider.GetDeviceModelFromDeviceType(matchedConfigurationDeviceModel.Type).Platform,
        RuntimeType = matchedConfigurationRuntimeModel.Type,
        RuntimeName = matchedConfigurationRuntimeModel.Name,
        Ip = matchedConfigurationDeviceModel.IPAddress, 
        Port = Convert.ToInt32(matchedConfigurationRuntimeModel.Port)
    };

    var applicationInstance = _componentModelProvider.GetApplicationModelFromApplicationType(application.Type);

    //need to close connection when switching runtimes
    //Do this before assigning the request executor so that disconnect is not skipped when runtimes
    //are switched
    if (lastRuntimeDetails != null)
    {
        if (lastRuntimeDetails?.Ip != runtimeDetails?.Ip ||
            lastRuntimeDetails?.Port != runtimeDetails?.Port)
        {
            _requestExecutor?.Disconnect();
        }
    }

    Resource resourceInstance = null;

    switch (matchedConfigurationRuntimeModel.Type)
    {
        case RuntimeType.FORTE:
            resourceInstance = _resourceProvider.GetEmbeddedResource(application.Name + "_EMB_RES");
            _requestExecutor = _forteRequestExecutor;
            break;
        case RuntimeType.FBRT:
            resourceInstance = _resourceProvider.GetPanelResource(application.Name + "_PANEL_RESOURCE");
            _requestExecutor = _fbrtRequestExecutor;
            break;
        default:
            break;
    }

    var appSettings = application.Settings;

    //Message: >>> Downloading application: ...
    PostOutputMessage(string.Format(ForteDownloadMessages.Default.download_level2_info,
                                    application.Name,
                                    fullRuntimeName.Split('.').First(),
                                    fullRuntimeName.Split('.').Last(),
                                    resourceInstance?.Name ?? "default"),
                      OutputWindowMessageType.Info);

    Dependency.Resolve<IForteMonitoringService>().MonitoringOn = false;
    var downloadSuccessful = DownloadApplication(application.Name, applicationInstance, appSettings, 
        matchedConfigurationRuntimeModel, resourceInstance, runtimeDetails);

    lastRuntimeDetails = runtimeDetails;

    runCommandSuccessful = downloadSuccessful ?? return null;

    DownloadedApplications.Add(new DownloadedApplication()
    {
        Application = application,
        Device = matchedConfigurationDeviceModel,
        RuntimeDetails = runtimeDetails,
        Resource = resourceInstance,
        Running = (bool) downloadSuccessful
    });
    _eventAggregator.GetEvent<RunningApplicationStatusChangedEvent>().Publish(DownloadedApplications);
}

Answer 3

The key to getting rid of the foreach loop was to create the following extension method:

 public static bool? ForEach<T>(this IEnumerable<T> source, Func<T, bool?> func)
        {
            bool? commandSuccessful = true;

            foreach (var element in source)
            {
                var rv = func(element);

                if (rv == null)
                {
                    return null;
                }

                if (rv == false)
                {
                    commandSuccessful = false;
                }
            }

            return commandSuccessful;
        }

Then I have split the OnStartApplicationsCommand function into four functions:

Note the usage of ForEach extension method above.

        public bool? OnStartApplicationsCommand()
        {
            // Get all the services for download begin
            ResolveAndUpdateDependencies();

            CancelOperationCommand = new AsyncDelegateCommand(StartApplicationsCommand.Cancel, 
                () => !StartApplicationsCommand.CanExecute(null));
            ProjectCommands.CancelOperation.RegisterCommand(CancelOperationCommand);

            _eventAggregator.GetEvent<OutputWindowSourceChangedEvent>().Publish(OutputWindowSourceType.Run);

            // Message: >>>>>> Downloading and running applications...
            PostStartOutputMessage(string.Format(ForteDownloadMessages.Default.download_level1_info,
                                                _projectManagerService.GetProjectName(),
                                                    _configurationService.GetActiveConfiguration().Name), 
                                   OutputWindowMessageType.Info);

            // Reset the list everytime on start command 
            // (regardless of the previous running status of the applications)
            DownloadedApplications.RemoveAll(application => true);  

            var activeConfiguration = _configurationService.GetActiveConfiguration();

            // Used to remember (for disconnecting) where the last application was downloaded to
            RuntimeDetails lastRuntimeDetails = null;
            var startCommandSuccessful = 
                activeConfiguration.Applications.ForEach( 
                    _ => PrepareRuntimeDetailsAndDownload( _ , ref lastRuntimeDetails));

            // The last runtime still needs to be disconencted
            DisconnectFromRuntime(lastRuntimeDetails); 
            return startCommandSuccessful;
        }

        private bool? PrepareRuntimeDetailsAndDownload(Application configurationApplication, 
            ref RuntimeDetails lastRuntimeDetails)
        {
            if (StartApplicationsCommand.CancellationRequested)
            {
                ProjectCommands.CancelOperation.UnregisterCommand(CancelOperationCommand);
                return null;
            }

            var runtimeDetails =
                ConstructRuntimeDetailsFromConfigurationApplication(configurationApplication);

            // Need to close connection when switching runtimes
            // Do this before assigning the request executor so that 
            // disconnect is not skipped when runtimes are switched
            if (lastRuntimeDetails != null)
                if (lastRuntimeDetails.Ip != runtimeDetails.Ip ||
                        lastRuntimeDetails.Port != runtimeDetails.Port)
                            DisconnectFromRuntime(lastRuntimeDetails);


            // FullRuntimeName is of the form deviceName.runtimeName 
            var fullRuntimeName = configurationApplication.MappedRuntimeName;
            Debug.Assert(fullRuntimeName != string.Empty, "fullRuntimeName != string.Empty");

            var mappedDeviceName = fullRuntimeName.Split('.').First();
            var mappedRuntimeName = fullRuntimeName.Split('.').Last();

            // Message: >>> Downloading application: ...
            PostStartOutputMessage(string.Format(ForteDownloadMessages.Default.download_level2_info, configurationApplication.Name,
                                            mappedDeviceName, mappedRuntimeName),
                                   OutputWindowMessageType.Info);

            var downloadSuccessful = DownloadApplication(configurationApplication,
                 _configurationModelProvider.
                    GetConfigurationRuntimeModelFromFullRuntimeName(fullRuntimeName),
                 runtimeDetails);

            // Publish for UI update
            _eventAggregator.GetEvent<RunningApplicationStatusChangedEvent>().
                Publish(DownloadedApplications);

            lastRuntimeDetails = runtimeDetails;

            return downloadSuccessful;
        }

        private void DisconnectFromRuntime(RuntimeDetails lastRuntimeDetails)
        {
            switch (lastRuntimeDetails.RuntimeType)
            {
                case RuntimeType.FZRTE:
                    break;
                case RuntimeType.FORTE:
                    _dependency.Resolve<IForteApplicationStartService>().DownloadRequestsExecutor.Disconnect();
                    break;
                case RuntimeType.FBRT:
                    _dependency.Resolve<IFbrtApplicationStartService>().DownloadRequestsExecutor.Disconnect();
                    break;
                default:
                    throw new ArgumentOutOfRangeException();
            }
        }

        public RuntimeDetails ConstructRuntimeDetailsFromConfigurationApplication(Application application)
        {
            // Fully is of the form deviceName.runtimeName 
            var fullRuntimeName = application.MappedRuntimeName;
            Debug.Assert(fullRuntimeName != string.Empty, "fullRuntimeName != string.Empty");

            var mappedConfigurationDevice = _configurationModelProvider.
                GetConfigurationDeviceFromFullRuntimeName(fullRuntimeName);

            var ipAddress = mappedConfigurationDevice.IPAddress;

            var mappedDevice = _componentModelProvider.
                GetDeviceModelFromDeviceType(mappedConfigurationDevice.Type);

            var mappedConfigurationRuntime = _configurationModelProvider.
                GetConfigurationRuntimeModelFromFullRuntimeName(fullRuntimeName);

            var runtimeDetails = new RuntimeDetails
            {
                DeviceName = mappedConfigurationDevice.Name,
                Platform = mappedDevice.Platform,
                RuntimeType = mappedConfigurationRuntime.Type,
                RuntimeName = mappedConfigurationRuntime.Name,
                Ip = ipAddress,
                Port = Convert.ToInt32(mappedConfigurationRuntime.Port)
            };

            return runtimeDetails;
        }