0
\$\begingroup\$

I've been writing my code like this for a really long time now, I am wondering if there is a more efficient way to do this. This is especially after a senior developer said my code was very amateurish.

App Purpose: I need to set some data for new employees when they join our organisation. This is a simple command app written in .net that takes an argument, splits it and then assigns the variables. A command might look like this:

Setdata.exe -Marcus -true -true -false -false. 

The output is a file that we install on one of our sql servers.

The example below is a small one, but it is not common for the settings to hit 40-50 lines.

Code:

  internal static void ExportSolution(SqlService service, string parms)
    {
        try
        {
            parms = parms.Replace(" ", "");
            string[] args = parms.Split('-');

            string solutionName = args[1];
            bool managed = Convert.ToBoolean(args[2]);
            bool prefix= Convert.ToBoolean(args[3]);
            bool profile= Convert.ToBoolean(args[4]);
            bool email= Convert.ToBoolean(args[5]);
            bool calendar= Convert.ToBoolean(args[6]);
            bool appSettings= Convert.ToBoolean(args[7]);


            AppSettingsRequestAppSettingsRequest= new AppSettingsRequest();

            AppSettingsRequest.SolutionName = solutionName;

            AppSettingsRequest.Managed = (managed == true) ? true : false;

            AppSettingsRequest.prefix= (prefix== true) ? true : false;

            AppSettingsRequest.Managed profile= (profile== true) ? true : false;

            AppSettingsRequest.email= (email== true) ? true : false;

            AppSettingsRequest.calendar= (calendar== true) ? true : false;

            AppSettingsRequest.appSettings= (appSettings== true) ? true : false;


            byte[] exportXml = exportSolutionResponse.ExportSolutionFile;
            string filename = solutionName + ".zip";
            File.WriteAllBytes(filename, exportXml);

            Console.WriteLine("Solution exported to {0}.", filename);
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex);
        }

Initial thoughts are that I could create a new class and then instantiate a new object with these variables, but is there anything to gain from doing this?

\$\endgroup\$
  • 3
    \$\begingroup\$ Would you mind posting the entire application? \$\endgroup\$ – t3chb0t Dec 10 '18 at 9:10
  • \$\begingroup\$ AppSettingsRequest.Managed = (managed == true) ? true : false; and the following can be written as AppSettingsRequest.Managed = managed; \$\endgroup\$ – Holger Dec 10 '18 at 9:26
5
\$\begingroup\$

Ignored code

From the code sample provided, you're not actually doing anything with the AppSettingsRequest object after you've created it. Why? What is the purpose of this object?


Global variables

byte[] exportXml = exportSolutionResponse.ExportSolutionFile;

What is exportSolutionResponse? It comes out of nowhere, without being defined anywhere. This seems to be a global variable (I can't otherwise explain its existence).

Global variables are generally a bad idea. They create shortcuts in code that is one of the straws that will eventually break the camel's back (in this case, unmaintainable spaghetti code).


Stringly typing

As you seem to be using a console application, the parameters can be parsed an an array of values, not as a single string. Why are you concatenating these values into a string, only to then split the string again?

This can be improved by using explicit method parameters:

internal static void ExportSolution(SqlService service, string name, params bool[] values)

This means you no longer have to do the conversion in this method; which is a good separation of concerns.

Given that you are expect a fixed amount of booleans, it's better to explicitly name these booleans instead of allowing any array of arbitrary length:

internal static void ExportSolution(SqlService service, string name, bool managed, bool prefix, bool managedprofile, bool email, bool calendar, bool appsettings)

Vague names

Your variable names leave something to be desired. I can only guess at what these variables mean. Rename them to something clearer. I've guessed at what these booleans are used for:

internal static void ExportSolution(SqlService service, string userName, bool isManaged, bool hasPrefix, bool useManagedProfile, bool importEmail, bool importCalendar, bool overrideAppSettings)

Don't get caught up on the names, my interpretations are only a guess. But these examples are much clearer on what the impact of these booleans is.


No null checking

This ties into the earlier point. In your original code, you are taking in any string and splitting it into an array, and then assuming that this array will contain at least 8 values.

What happens when the array contains less than 8 values? You're going to end up with an IndexOutOfRangeException. While you do catch and display exceptions, it's still better to avoid exceptions where possible. It's not that hard to validate the length of the array before you try to access its elements.

if(args.Length < 8)
{
    Console.WriteLine("Insufficient parameters provided");
    return;
}

Pokémon exception handling (link)

catch (Exception ex)
{
    Console.WriteLine(ex);
}

Instead of catching all exceptions, catch the exceptions you're interested in. For example:

catch (IndexOutOfRangeException ex)
{
    Console.WriteLine("Insufficient parameters provided");
}

Note: as mentioned before, it is better to prevent the exception when possible, instead of just catching it afterwards. This only serves as a simple example of how you catch specific exceptions instead of all possible exception types.


Needlessly verbose code

AppSettingsRequest.Managed = (managed == true) ? true : false;

this can immediately be refactored to:

AppSettingsRequest.Managed = managed;

For two reasons:

  1. == true is redundant. If the variable is already a boolean, its "trueness" doesn't need to be checked. managed == true will always equal managed.
  2. ? true : false is redundant. The evaluation (left of the ?) is already a boolean, and what you're doing here is saying "if this evaluation is true, set this to true; if this evalutation is false, set it to false". It's much short to just say "set this to the evaluation's value". managed ? true : false will always equal managed.

Note: if you were using nullable booleans, bullet point 1 wouldn't necessarily be correct (as you're also checking for non-null in that case), but you're not using nullable booleans here.


Saving some lines of code

bool managed = Convert.ToBoolean(args[2]);
AppSettingsRequest.Managed = managed;

Can be shortened to

AppSettingsRequest.Managed = Convert.ToBoolean(args[2]);

In general, I wouldn't advise this. The added clarity from separating the steps makes things easier. However, given that one of your main issues is the amount of settings, combining the two steps will halve your line count. At the same time, the combined line is not unreadable by any stretch of the imagination so it's perfectly okay to combine them here.


Separating your concerns

In general, your method is taking on too many tasks. It manipulates the input string, converts the values, handles them, writes to storage and updates the UI (console output). This is too many responsibilities for a single method.

However, this seems to be a very small and simple console application, and the overhead cost of implementing some separations may end up costing more effort than it's worth. I would still suggest a basic separation between the following tasks:

  • Parsing the input values (to their correct string/bool types)
  • Instantiating the settings object and settings its properties
  • Writing your data to disk
\$\endgroup\$
3
\$\begingroup\$

Because creating/readings settings and creating the output file are two separate things, consider separating the settings parsing part to different function/class so it can be actually tested without creating files.

Separating the parsing to different file also makes it easier to take the settings from different sources in future if needed (config-files, etc.)

\$\endgroup\$
-3
\$\begingroup\$

The code looks good, but I think the code could look more structured, like maybe in the appsetings you could give them one more indentation. This way the code won't just be one long line and it would be much easier to differentiate the code structure.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.