make the below code cleaner

fsociety

New member
Joined
Sep 23, 2019
Messages
1
Programming Experience
10+
Hi all,

I've been developing software on two proprietory platforms for the last 15 years. Crestron and AMX. Until recently, both have always used a procedural C style language. In fact, the Crestron Simpl+ language is a direct overlay of C I believe. More recently, on the Crestron side, it is now possible to develop in C# and .NET, albeit an old version. The hardware depends on .NET 3.5 Compact meaning I have to use VS2008. Even though it's an old IDE, it's worlds apart from what I'm used to using.

For anyone that doesn't know, Crestron and AMX are manufacturers of Audio/Visual control systems. Boxes with a lot of serial ports, IO ports, relay ports etc for controlling any aspect of an Audio/Visual system. My work involves interfacing with a lot of RS232 devices to control projectors and monitors etc using the device manufacturer's communication protocol.

Everything I have done so far with C# has been by self-education and a lot of trial and error.

I would like to ask for some advice to make the below code cleaner. It's a logic engine for controlling a Sony TV. This is a callback from a Timer object that runs every 200ms. Every passing second, it queries the device and on all other entries, it performs checks to evaluate if any tasks like power control or input switching are required.

Although this would be completely fine and acceptable before, in C# and with what I've learned so far, it feels too complicated and like a code smell. My hunch is there is probably a much simpler and cleaner way to do this. More repeatable. But I'm not seeing it.

Any help or advice would be greatly appreciated. If you have any question or need further information just ask. I can post other parts of the class if required.

Thanks,

C#:
private void DriveLogicEngine(object obj) {
    if (!_client.IsConnected) {
        return;
    }

    switch (++_logicEngineLoopCount) {
        case 5: {
            switch (_query) {
                case GenericQuery.Power: {
                    _queue.Enqeue(BuildPayload(_messageType[MessageType.Enquiry], _messageFunction[MessageFunction.Power], _queryValue[_query]), CommunicationQueue.MessageType.Query);
                    break;
                }
                case GenericQuery.Input: {
                    _queue.Enqeue(BuildPayload(_messageType[MessageType.Enquiry], _messageFunction[MessageFunction.Input], _queryValue[_query]), CommunicationQueue.MessageType.Query);
                    break;
                }
            }

            _logicEngineLoopCount = 0;
            break;
        }
        default: {
            if (_taskInProgress) return;

            if (PowerTaskIsComplete()) {
                _requiredPower = GenericState.None;
                return;
            }

            if (InputTaskIsComplete()) {
                _requiredInput = GenericInput.None;
                return;
            }

            if (PowerTaskIsRequired()) {
                _queue.Enqeue(BuildPayload(_messageType[MessageType.Command], _messageFunction[MessageFunction.Power], _powerValue[_requiredPower]), CommunicationQueue.MessageType.Command);
                _taskInProgress = true;
                _taskComplete = new CTimer(OnTaskComplete, null, 0, 8000);
                _query = GenericQuery.Power;
                _logicEngineLoopCount = 4;

                return;
            }

            if (InputTaskIsRequired()) {
                _queue.Enqeue(BuildPayload(_messageType[MessageType.Command], _messageFunction[MessageFunction.Input], _inputValue[_requiredInput]), CommunicationQueue.MessageType.Command);
                _taskInProgress = true;
                _taskComplete = new CTimer(OnTaskComplete, null, 0, 2000);
                _query = GenericQuery.Input;
                _logicEngineLoopCount = 4;
                return;
            }

            break;
        }
    }
}

private void OnTaskComplete(object obj) {
    _taskInProgress = false;
    _taskComplete.Dispose();
}

private bool PowerTaskIsRequired() {
    return _requiredPower != GenericState.None && _requiredPower != _actualPower;
}

private bool PowerTaskIsComplete() {
    return _requiredPower != GenericState.None && _requiredPower == _actualPower;
}

private bool InputTaskIsRequired() {
    return _requiredInput != GenericInput.None && _requiredInput != _actualInput &&
        _actualPower == GenericState.On;
}
 
Your obj parameters for DriveLogicEngine() and OnTaskComplete() methods doesn't seem to be used.

Your methods PowerTaskIsRequired(), PowerTaskIsComplete[icode], and [icode]InputTaskIsRequired should be getters:
C#:
bool IsPowerTaskRequired {
    get { /* implementation here */ }
}

bool IsPowerTaskCompleted {
    get { /* implementation here */ }
}

bool IsInputTaskRequired {
    get { /* implementation here */ }
}

Unlike in C where big long functions are accepted because of historical reasons (e.g. it used to be expensive to make a function call so people would write thousand line functions), in C# the general rule of thumb is have functions follow the Single Responsibility Principle and keep them short and sweet. Ideally under 20-25 lines, but I've heard of some teams coding conventions where they have a 15 line maximum. Any which way, your DriveLogicEngine() should be broken up into multiple methods.

Also, what the deal with all those "magic numbers" in DriveLogicEngine? What does 0, 4 and 5 mean for the loop count? What is 2000 and 8000 as parameters to the CTimer constructor? What is 0 as a parameter to the CTimer constructor?

Do you really need that switch statement in DriveLogicEngine()? It looks like you just have the case when the loop count has been incremented to 5, and then all other cases. Wouldn't it be more readable if that method were just written as:

C#:
void DriveLogicEngine()
{
    if (client.IsConnected)
    {
        ++_logicEngineLoopCount;
        if (_logicEngineLoopCount == 5)
            HandleQueries();
        else
            HandleTasks();
    }
}

Lines 10 and 14 are very similar. Should there be a helper method to encapsulate the common stuff and take parameters for what varies?

Lines 36-40 and 46-50 are very similar. Should there be a helper method to encapsulate the common stuff and take parameters for what varies?
 
Last edited:
Back
Top Bottom