HiveBrain v1.2.0
Get Started
← Back to all entries
principlecsharpMinor

Refactor (DRY, KISS) QueryStatus event for VS Addin

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
kissquerystatusaddinfordryrefactorevent

Problem

I have complex logic in QueryStatus event (CommandTarget) for a VS Addin (VS 2010).

I'm a newbie using C#.

I would like elegant, easily maintainable code that follows DRY and KISS principles. Maybe using helper with ActionT> or FuncT> methods.

There are many commands to use: using if statements and switch statements.

Main method: QueryStatus event call to comandosOracleNET method.

comandosOracleNET method determines if a option in Menu is visible or not.

void IDTCommandTarget.QueryStatus(string CmdName, vsCommandStatusTextWanted NeededText, ref vsCommandStatus StatusOption, ref object CommandText)
        {
            if (!OnStartupCompleted) return;

            try
            {
                if (NeededText == vsCommandStatusTextWanted.vsCommandStatusTextWantedNone)
                {

                    comandosOracleNET(CmdName, NeededText, ref StatusOption, ref CommandText);

                    return;

                }

            }
            catch (Exception exc)
            {
                ErroresManager.TratamientoExcepcionEnConnectQueryStatus(exc, "Error en Command. Excepción en IDTCommandTarget.QueryStatus.");
            }

        }


Main method:

```
private void comandosOracleNET(string CmdName, vsCommandStatusTextWanted NeededText, ref vsCommandStatus StatusOption, ref object CommandText)
{
EnvDTE80.Solution2 solucion = (EnvDTE80.Solution2)_applicationObject.Solution;
string etiqueta = null;
bool bajoSC = false;

if (_applicationObject.SourceControl != null
&& _applicationObject.Solution != null
&& _applicationObject.Solution.IsOpen
&& !solucion.FullName.IsNullOrWhiteSpace()
&& _applicationObject.SourceControl.IsItemUnderSCC(solucion.FullName))
{
etiqueta = Logica.OperacionesEtiquetado.ObtenerEtiquetaSolucion(solucion);
bajoSC = true;
}

#region Branch y Merge

if (CmdName == _addInInstance.ProgID + "." + ComandoBranchItemSqlForSW_Na

Solution

Using #region in a method is a very strong indicator that the method should be broken into smaller methods. It is considered as code smell.

You have to much vertical spacing (new lines) in your code so Sam the Maintainer can't grasp the code at first glance.

Using braces {} for single if statements is a good start to make your code less error prone. If you consider to not use them, you should stick to your style you choose. Right now you are mixing them.

Using the english language for naming variables will help Sam the Maintainer to maintain the code if he/she doesn't speak your language.

While we are at the naming of variables, based on the naming guidelines input parameters should be named using camelCase casing.

A if..else like

if (condition) 
{
    // some code
    return;
}
else
{
    //some other code
}


makes the else part redundant, you can simplify this to

if (condition) 
{
    // some code
    return;
}
//some other code


By using a second guard clause in the IDTCommandTarget.QueryStatus() method, you can reduce the horizintal spacing which add readability to your code.

The return statement in the try..catch is redundant and should be removed.

void IDTCommandTarget.QueryStatus(string cmdName, vsCommandStatusTextWanted neededText, ref vsCommandStatus statusOption, ref object commandText)
{
    if (!OnStartupCompleted) { return; }
    if (neededText != vsCommandStatusTextWanted.vsCommandStatusTextWantedNone) { return; }

    try
    {
        comandosOracleNET(cmdName, neededText, ref statusOption, ref commandText);
    }
    catch (Exception exc)
    {
        ErroresManager.TratamientoExcepcionEnConnectQueryStatus(exc, "Error en Command. Excepción en IDTCommandTarget.QueryStatus.");
    }
}


By extracting the if statements of #region Branch y Merge to a separate method you can reduce code duplication.

private bool IsBrancheOrMerge(String cmdName)
{
    return (cmdName == _addInInstance.ProgID + "." + ComandoBranchItemSqlForSW_Name) ||
           (cmdName == _addInInstance.ProgID + "." + ComandoMergeItemSqlForSW_Name);
}


now the former 2 if statements can be reduced to

if(IsBrancheOrMerge(cmdName))
{
    if (_applicationObject.SelectedItems.Count == 1 && _applicationObject.SelectedItems.Item(1).ProjectItem.FileCount == 1)
    {
        string aux = _applicationObject.SelectedItems.Item(1).ProjectItem.get_FileNames(1);
        if (aux.ToLower().EndsWith(".sql"))
        {
            StatusOption = vsCommandStatus.vsCommandStatusSupported | vsCommandStatus.vsCommandStatusEnabled;
        }
        else
        {
            StatusOption |= vsCommandStatus.vsCommandStatusInvisible;
        }
    }
    return;
}


and by extracting the inner if statements to a method ProcessBranchOrMerge()

private void ProcessBranchOrMerge(ref vsCommandStatus statusOption)
{
    if (_applicationObject.SelectedItems.Count == 1 && _applicationObject.SelectedItems.Item(1).ProjectItem.FileCount == 1)
    {
        string aux = _applicationObject.SelectedItems.Item(1).ProjectItem.get_FileNames(1);
        if (aux.ToLower().EndsWith(".sql"))
        {
            statusOption = vsCommandStatus.vsCommandStatusSupported | vsCommandStatus.vsCommandStatusEnabled;
        }
        else
        {
            statusOption |= vsCommandStatus.vsCommandStatusInvisible;
        }
    }
}


it can be further reduced to

if(IsBrancheOrMerge(cmdName))
{
    ProcessBranchOrMerge(ref statusOption);
    return;
}


In a similiar way you can extract more of your code to separate methods.

case ComandoDespliegueSqlServer:

This case can be easily extracted to a improved method like

private void ProcessComandoDespliegueSqlServer(ref vsCommandStatus statusOption, EnvDTE80.Solution2 solucion)
{
    statusOption = vsCommandStatus.vsCommandStatusSupported | vsCommandStatus.vsCommandStatusInvisible;

    if (!solucion.IsOpen) { return; }

    object[] proyectos = (object[])_applicationObject.ActiveSolutionProjects;

    if (proyectos.Length != 1) { return; }

    if (Logica.Operaciones.IsDeploySqlServerProject((Project)proyectos[0]))
    {
        statusOption |= vsCommandStatus.vsCommandStatusEnabled;
    }
}


leaving the case

case ComandoDespliegueSqlServer:
    ProcessComandoDespliegueSqlServer(ref statusOption, solucion);
    return;

Code Snippets

if (condition) 
{
    // some code
    return;
}
else
{
    //some other code
}
if (condition) 
{
    // some code
    return;
}
//some other code
void IDTCommandTarget.QueryStatus(string cmdName, vsCommandStatusTextWanted neededText, ref vsCommandStatus statusOption, ref object commandText)
{
    if (!OnStartupCompleted) { return; }
    if (neededText != vsCommandStatusTextWanted.vsCommandStatusTextWantedNone) { return; }

    try
    {
        comandosOracleNET(cmdName, neededText, ref statusOption, ref commandText);
    }
    catch (Exception exc)
    {
        ErroresManager.TratamientoExcepcionEnConnectQueryStatus(exc, "Error en Command. Excepción en IDTCommandTarget.QueryStatus.");
    }
}
private bool IsBrancheOrMerge(String cmdName)
{
    return (cmdName == _addInInstance.ProgID + "." + ComandoBranchItemSqlForSW_Name) ||
           (cmdName == _addInInstance.ProgID + "." + ComandoMergeItemSqlForSW_Name);
}
if(IsBrancheOrMerge(cmdName))
{
    if (_applicationObject.SelectedItems.Count == 1 && _applicationObject.SelectedItems.Item(1).ProjectItem.FileCount == 1)
    {
        string aux = _applicationObject.SelectedItems.Item(1).ProjectItem.get_FileNames(1);
        if (aux.ToLower().EndsWith(".sql"))
        {
            StatusOption = vsCommandStatus.vsCommandStatusSupported | vsCommandStatus.vsCommandStatusEnabled;
        }
        else
        {
            StatusOption |= vsCommandStatus.vsCommandStatusInvisible;
        }
    }
    return;
}

Context

StackExchange Code Review Q#71296, answer score: 5

Revisions (0)

No revisions yet.