2
\$\begingroup\$

I have a custom attribute in my assembly called SemverAttribute, and I have a helper class called AppInfo that has a function to return a number called the Semver number. It accepts an id of null-6. Is this the most efficient way to do this or should I break all this up into separate strings instead of one inline function with an argument?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Reflection;
using authenticator.Properties;

namespace authenticator.Helpers
{
    public class AppInfo
    {

        // Grab the SemVer from assembly
        // You may pass any value 0-6 or null for id
        // null or 0 will return the entire SemVer string (X.Y.Z-pre+meta)
        // 1 will return the SemVer Major.Minor.Patch/Micro (X.Y.Z)
        // 2 will return the SemVer Major (X)
        // 3 will return the SemVer Minor (Y)
        // 4 will return the SemVer Patch/Micro (Z)
        // 5 will return the SemVer Pre data
        // 6 will return the SemVer Meta data
        public static string SemverPart(int id = 0)
        {
            var attribute = Assembly.GetExecutingAssembly().GetCustomAttributes(false).OfType<SemverAttribute>().FirstOrDefault();
            string str = (attribute == null) ? string.Empty : attribute.getversion;

            // Define delimiter to split pre, and meta information off the string
            string[] d = { "-", "+" };

            // Convert SemVer string to array
            var n = str.Split(d, StringSplitOptions.RemoveEmptyEntries);

            if (id > 0 & id <= 6 )
            {
                if (id >= 1 & id < 5)
                {
                    // Grab the full version number
                    string full = n[0];

                    // Define delimiter to split version number into Major.Minor.Patch/Micro
                    string[] x = { "." };

                    // Convert version string to array
                    var v = full.Split(x, StringSplitOptions.RemoveEmptyEntries);

                    // Passing 1 returns entire version number (X.Y.Z)
                    if (id == 1)
                    {
                        return (full == null) ? string.Empty : full;
                    }
                    // Passing 2 returns major version number (X)
                    else if (id == 2)
                    {
                        return (v[0] == null) ? string.Empty : v[0];
                    }
                    // Passing 3 returns minor version number (Y)
                    else if (id == 3)
                    {
                        return (v[1] == null) ? string.Empty : v[1];
                    }
                    // Passing 4 returns patch/micro version number (Z)
                    else
                    {
                        return (v[2] == null) ? string.Empty : v[2];
                    }
                }
                // Passing 5 returns pre data
                else if (id == 5)
                {
                    return (n[1] == null) ? string.Empty : n[1];
                }
                // Passing 6 returns the meta data
                else
                {
                    return (n[2] == null) ? string.Empty : n[2];
                }
            } 
            // Return full string if no argument is passed or an invalid argument is passed
            else 
            {
                return str;
            }
        }
    }
}

EDIT: Version 3 based on answer by unholysamper. This can still be improved if I can break up the version string ONLY if 1-4 or selected.:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Reflection;
using Authenticator.Properties;

namespace Authenticator.Helpers
{
    public class AppInfo
    {

        // Grab the SemVer from assembly
        // You may pass any value 0-6 for id
        // 0 or nothing will return the entire SemVer string (X.Y.Z-pre+meta)
        // 1 will return the SemVer Major.Minor.Patch/Micro (X.Y.Z)
        // 2 will return the SemVer Major (X)
        // 3 will return the SemVer Minor (Y)
        // 4 will return the SemVer Patch/Micro (Z)
        // 5 will return the SemVer Pre data
        // 6 will return the SemVer Meta data
        public static string SemverPart(int id = 0)
        {
            var attribute = Assembly.GetExecutingAssembly().GetCustomAttributes(false).OfType<SemverAttribute>().FirstOrDefault();
            string SemVer = (attribute == null) ? string.Empty : attribute.getversion;

            string[] delimiter = { "-", "+" };
            var SemVerArray = SemVer.Split(delimiter, StringSplitOptions.RemoveEmptyEntries);
            var VersionArray = SemVerArray[0].Split('.');

            switch (id)
            {
                case 1:
                    return SemVerArray[0] ?? string.Empty;
                case 2:
                    return VersionArray[0] ?? string.Empty;
                case 3:
                    return VersionArray[1] ?? string.Empty;
                case 4:
                    return VersionArray[2] ?? string.Empty;
                // Determine if SemVer contains pre data and return it if it does
                case 5:
                    if (SemVer.Contains("-"))
                    {
                        return SemVerArray[1] ?? string.Empty;
                    }
                    else 
                    {
                        return string.Empty;
                    }
                case 6:
                    if (SemVer.Contains("-"))
                    {
                        return SemVerArray[2] ?? string.Empty;
                    }
                    else
                    {
                        return SemVerArray[1] ?? string.Empty;
                    }
                default:
                    return SemVer;
            }
        }
    }
}
\$\endgroup\$
11
  • 1
    \$\begingroup\$ Why are you using dynamic instead of var or string[]? \$\endgroup\$ Commented Jun 5, 2014 at 15:27
  • 2
    \$\begingroup\$ just use var, which will represent a string[]. dynamic should be used only when it's really necessary (so in many many case : never). \$\endgroup\$ Commented Jun 5, 2014 at 15:33
  • 1
    \$\begingroup\$ Dynamic basically means the compiler doesn't worry about the type (which is generally a bad thing, but sometimes needed). var accepts the type that is assigned to it - in this case string[] \$\endgroup\$
    – Magus
    Commented Jun 5, 2014 at 15:36
  • 1
    \$\begingroup\$ In C#, public things are PascalCase. Private and member variables are camelCase. Some people make member variables _hungarianCamelCase, but your IDE can color them differently anyway, so the advantage of doing so is very much debatable. For the most part, snake_case isn't a thing in C#. \$\endgroup\$
    – Magus
    Commented Jun 5, 2014 at 15:47
  • 1
    \$\begingroup\$ The best place is probably this MSDN page. I'd also like to mention that your current code does not, in fact, support nulls. You could change the argument to an int? to allow that, but only if you really need to. \$\endgroup\$
    – Magus
    Commented Jun 5, 2014 at 16:08

2 Answers 2

2
\$\begingroup\$

You have a lot of comments saying what you are doing (almost one for every line of code). Code is good at saying what is happing, comments are better at saying why something is happening. If it is not clear to a reader what the code does, this is an indication that you might need better variable names or more descriptive method names. Breaking a block of coding into a separate private function is a good way to make the code more readable.


(n[1] == null) ? string.Empty : n[1];

is equivalent to

n[1] ?? string.Empty;

if (id == 1) {
  return 1;
}
else if (id == 2)
{
  return 2;
} //...

is equivalent to

switch (id) {
case 1:
  return 1;
case 2:
  return 2;
//...
}
\$\endgroup\$
3
  • \$\begingroup\$ See edit version 2. The problem now is I'm being told not all code paths return a value. \$\endgroup\$ Commented Jun 5, 2014 at 17:49
  • 1
    \$\begingroup\$ @aaronmallen: This is likely due to the compiler not being smart enough to know that the if clauses excluding all other values for id. You can add a default case that throws an exception saying "this should never happen". \$\endgroup\$ Commented Jun 5, 2014 at 18:06
  • \$\begingroup\$ See edit version 3: Still one improvement that can be made here. If I can I initiate breaking up the Major.Minor.Patch ONLY when option 2-4 are selected? I tried wrapping those variables in an if statement first but I couldn't figure out how to call them since wrapped in an if statement they wouldn't be in the context and I don't know how to define var VersionArray without assigning it a value first which can't be assigned a value without first assigning string version a value. \$\endgroup\$ Commented Jun 5, 2014 at 18:18
2
\$\begingroup\$

This would be a lot cleaner as a switch

 // Passing 1 returns entire version number (X.Y.Z)
if (id == 1)
{
    return (full == null) ? string.Empty : full;
}
// Passing 2 returns major version number (X)
else if (id == 2)
{
    return (v[0] == null) ? string.Empty : v[0];
}
//etc...

Compared to:

 // Passing 1 returns entire version number (X.Y.Z)

switch(id)
{
    case 1:
        return (full == null) ? string.Empty : full;
    case 2:
        return (v[0] == null) ? string.Empty : v[0];
        //etc...
}

Note that if you use a switch without a return, you would need to break;, as the code would continue to "fall through".

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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