7
\$\begingroup\$

For a small personal project, I am looking to convert an object to an ordered by SemVer Markdown list. You can find the original code for the sort logic here and here for the writing into Markdown logic. I merged them to a full working snippet. I am using C# 5 in .NET 4.5.1.

Basically, the goal is to transform this:

var versions = new[]
{
    new Version { Name = "v0", Evolutions = new[] { "Add foo 8", "Add bar 9" }},
    new Version { Name = "v2", Evolutions = new[] { "Add foo 0", "Add bar 1" }},
    new Version { Name = "v1", Evolutions = new[] { "Add foo 6", "Add bar 7" }},
    new Version { Name = "v1.1", Evolutions = new[] { "Add foo 4", "Add bar 5" }},
    new Version { Name = "v1.2-alpha", Evolutions = new[] { "Add foo 2", "Add bar 3" }}
};

into that:

## Evolutions

### v2

* Add foo 0
* Add bar 1

### v1.2-alpha

* Add foo 2
* Add bar 3

### v1.1

* Add foo 4
* Add bar 5

### v1

* Add foo 6
* Add bar 7

### v0

* Add foo 8
* Add bar 9

I wrote the code to do it but I am not very proud of it. It smells not right. I am sure I can clear it and use betters way to achieve what I want. You can try the whole program in .NETFiddle.

What do you think of it? How can I improve it?

using System;
using System.Linq;
using System.Text;
using System.Collections.Generic;

public class Program
{
    public class Version
    {
        public string Name { get; set; }
        public string[] Evolutions { get; set; }
    }

    public static void Main()
    {
        var versions = new[]
        {
            new Version { Name = "v0", Evolutions = new[] { "Add foo 8", "Add bar 9" }},
            new Version { Name = "v2", Evolutions = new[] { "Add foo 0", "Add bar 1" }},
            new Version { Name = "v1", Evolutions = new[] { "Add foo 6", "Add bar 7" }},
            new Version { Name = "v1.1", Evolutions = new[] { "Add foo 4", "Add bar 5" }},
            new Version { Name = "v1.2-alpha", Evolutions = new[] { "Add foo 2", "Add bar 3" }}
        };

        var cleanedVersions = OrderByVersion(versions).ToArray();
        Console.WriteLine(WriteVersions(cleanedVersions));
    }

    /// <summary>
    /// Order by version an enumerable of Version
    /// </summary>
    /// <param name="versions">An enumerable of Version to sort</param>
    /// <returns>An enumerable of Version sorted</returns>
    /// <example>
    /// [[1, 2], [1, 1], [2, 0]] will give [[2, 0], [1, 2], [1, 1]]
    /// </example>
    private static IEnumerable<Version> OrderByVersion(IEnumerable<Version> versions)
    {
        return
            from v in versions.Select(v => new
            {
                v.Name,
                v.Evolutions,
                SemVer = ExtractSemVer(v.Name)
            })
            .OrderByDescending(v => v.SemVer[0])
            .ThenByDescending(v => v.SemVer[1])
            .ThenByDescending(v => v.SemVer[2])
            select new Version { Name = v.Name, Evolutions = v.Evolutions };
    }

    /// <summary>
    /// Extract SemVer format from string into an array
    /// </summary>
    /// <param name="version">A version in the SemVer format</param>
    /// <returns>An array of string composed by SemVer format</returns>
    /// <example>
    /// v0 will give [0, "", ""]
    /// v1.2.3-foobar will give [1, 2, 3]
    /// </example>
    private static string[] ExtractSemVer(string version)
    {
        var semver = new string(version.Where(c => c.Equals('.') || Char.IsDigit(c)).ToArray());
        return !semver.Contains('.')
            ? new[] { semver, String.Empty, String.Empty }
            : new List<string>(
                semver.Split(new[] { '.' }, StringSplitOptions.RemoveEmptyEntries))
                {
                    String.Empty,
                    String.Empty
                }.ToArray();
    }

    /// <summary>
    /// Write a Markdown list in a string form from a list of versions
    /// </summary>
    /// <param name="versions">A list of version to write in Markdown</param>
    /// <returns>A Markdown list in a string form</returns>
    public static string WriteVersions(IList<Version> versions)
    {
        Func<bool, string> lineOrEmpty = b => b ? Environment.NewLine : String.Empty;
        Func<int, int, bool> isLimit = (n, limit) => n + 1 < limit;
        Func<int, string> line = n => lineOrEmpty(isLimit(n, versions.Count));

        var stringBuilder = new StringBuilder("## Evolutions" + Environment.NewLine + Environment.NewLine);
        for (var i = 0; i < versions.Count; i++)
        {
            stringBuilder.AppendFormat("### {0}{1}{1}",
                versions[i].Name,
                Environment.NewLine);
            for (int index = 0; index < versions[i].Evolutions.Length; index++)
            {
                var length = versions[i].Evolutions.Length;
                stringBuilder.AppendFormat("* {0}{1}",
                    versions[i].Evolutions[index],
                    lineOrEmpty(!String.IsNullOrEmpty(line(i)) || isLimit(index, length)));
            }
            stringBuilder.Append(line(i));
        }
        return stringBuilder.ToString();
    }
}

The class is self-contained and documented but if you want more information, feel free to ask for it.

\$\endgroup\$
5
\$\begingroup\$

OrderByVersion

  1. You've taken the time to add doc comments, but a vital piece of info is missing. The enumerable is sorted in descending order. That should be there in the doc comments.
  2. It would be nice to have an option to sort ascending. (It's also an interesting problem to solve. =;)-
  3. The mixture of query and lambda syntax seems a bit strange to me. I'd pick one and stick with it. I don't necessarily mean use just one style everywhere, but you should probably stick to just one of the two within a statement/method.

ExtractSemVer

  1. There's no reason to abbreviate. This method should be named ExtractSemanticVersion.
  2. I would seriously consider introducing a SemanticVersionNumber struct.

    public struct SemanticVersionNumber
    {
        public int Major { get; }
        public int Minor { get; }
        public int Patch { get; }
        public string Build { get; }
    
        ....
     }
    

    You could give a nice ToString method and a constructor that turns a string into an instance of this struct. Nicely tucking away the logic to do so, with the added benefit of possibly simplifying the ordering logic. You'd probably want to implement IComparable. Yes. I like that idea. You could take all of this logic out of your console class and put it in here where it probably belongs. You could also possibly make it part of your existing Version class, replacing Name with the ToString method that I mentioned.

WriteVersions

  1. You never really use the index of the loop, so it will be cleaner to just use a foreach loop.
  2. Use the AppendLine method to append new lines to the string instead of awkwardly formatting them in.
  3. Only set the length variable once, outside of the loop.

public static string WriteVersions(IList<Version> versions)
{
    Func<bool, string> lineOrEmpty = b => b ? Environment.NewLine : String.Empty;
    Func<int, int, bool> isLimit = (n, limit) => n + 1 < limit;
    Func<int, string> line = n => lineOrEmpty(isLimit(n, versions.Count));

    var stringBuilder = new StringBuilder("## Evolutions");
    stringBuilder.AppendLine()
        .AppendLine();

    foreach (var version in versions)
    {
        stringBuilder.AppendFormat("### {0}", version.Name)
            .AppendLine()
            .AppendLine();

        var length = version.Evolutions.Length;

        foreach (var evolution in version.Evolutions)
        {
            stringBuilder.AppendFormat("* {0}", evolution)
                .AppendLine();
        }
        stringBuilder.AppendLine();
    }
    return stringBuilder.ToString();
}
\$\endgroup\$
  • \$\begingroup\$ OrderByVersion() => 1. True. 2. Maybe later. ;) 3. Bye Query syntax. ExtractSemVer() => 1. True. 2. I will look to build that. Thank you. \$\endgroup\$ – aloisdg says Reinstate Monica Oct 6 '15 at 4:51
  • \$\begingroup\$ Any idea to improve the code himself? There are some OrderedList out there, maybe one of this data structure can help me? Any ideas? \$\endgroup\$ – aloisdg says Reinstate Monica Oct 6 '15 at 4:53
  • \$\begingroup\$ Which part in particular aren't you happy with @aloisdg? \$\endgroup\$ – RubberDuck Oct 6 '15 at 5:50
  • \$\begingroup\$ The whole WriteVersions() seems to complicated for what he do. I think I will split it in more small functions. For the order logic, I search a bit and I found SortedSet. If I implement IComparable, I should be able to order my list directly. A set is great because each version is unique. \$\endgroup\$ – aloisdg says Reinstate Monica Oct 6 '15 at 6:08
  • 1
    \$\begingroup\$ I agree to say that I don't need to do all of that to do this. Thank you for all the help! \$\endgroup\$ – aloisdg says Reinstate Monica Oct 6 '15 at 7:46
4
\$\begingroup\$
/// <summary>
/// Extract SemVer format from string into an array
/// </summary>
/// <param name="version">A version in the SemVer format</param>
/// <returns>An array of string composed by SemVer format</returns>
/// <example>
/// v0 will give [0, "", ""]
/// v1.2.3-foobar will give [1, 2, 3]
/// </example>
private static string[] ExtractSemVer(string version)
{
    var semver = new string(version.Where(c => c.Equals('.') || Char.IsDigit(c)).ToArray());
    return !semver.Contains('.')
        ? new[] { semver, String.Empty, String.Empty }
        : new List<string>(
            semver.Split(new[] { '.' }, StringSplitOptions.RemoveEmptyEntries))
            {
                String.Empty,
                String.Empty
            }.ToArray();
}  

this method isn't doing what the example in the documentation promises.

For a value of v1.2.3-foobar it will return an array having 5 elements "1", "2", "3", "", "". For a value like 1.2 it will return an array having 4 elements "1", "2", "", "".

In addition it seems overly complicated mostly because of this ugly tenary expression.

The condition semver.Contains('.') isn't needed. We can just Split by a dot and then use Enumerable.Repeat() to concat the remaining empty strings like so

private static readonly int maxVersionParts = 3;
private static string[] ExtractSemanticVersion(string version)
{
    var semanticVersions = new string(version.Where(c => c.Equals('.') || Char.IsDigit(c)).ToArray());

    string[] values = semanticVersions.Split(new[] { '.' }, StringSplitOptions.RemoveEmptyEntries);
    int neededValues = maxVersionParts - values.Length;

    return values.Concat(Enumerable.Repeat(string.Empty, neededValues)).ToArray();
}

public static string WriteVersions(IList<Version> versions)
{
    Func<bool, string> lineOrEmpty = b => b ? Environment.NewLine : String.Empty;
    Func<int, int, bool> isLimit = (n, limit) => n + 1 < limit;
    Func<int, string> line = n => lineOrEmpty(isLimit(n, versions.Count));

    var stringBuilder = new StringBuilder("## Evolutions" + Environment.NewLine + Environment.NewLine);
    for (var i = 0; i < versions.Count; i++)
    {
        stringBuilder.AppendFormat("### {0}{1}{1}",
            versions[i].Name,
            Environment.NewLine);
        for (int index = 0; index < versions[i].Evolutions.Length; index++)
        {
            var length = versions[i].Evolutions.Length;
            stringBuilder.AppendFormat("* {0}{1}",
                versions[i].Evolutions[index],
                lineOrEmpty(!String.IsNullOrEmpty(line(i)) || isLimit(index, length)));
        }
        stringBuilder.Append(line(i));
    }
    return stringBuilder.ToString();
}  

this method looks overly complex and is IMO named wrong. A much better name would just be Format() or FormatVersions().

It also adds an additional new line if the Evolutions does not contain any items.

Assume the following setup

var versions = new[]
{
new Version { Name = "v0", Evolutions = new[] { "Add foo 0", "Add bar 1" }},
new Version { Name = "v2", Evolutions = new string[0]},
new Version { Name = "v1", Evolutions = new[] { "Add foo 6", "Add bar 7" }}
};

the output of your method is

## Evolutions

### v2


### v1

* Add foo 6
* Add bar 7

### v0

* Add foo 0
* Add bar 1  

By using a foreach, a little bit of linq "magic" and string.Join() we can get a "better" output with a more readable and maintainable code like so

private static readonly string separator = Environment.NewLine + "* ";
public static string WriteVersions(IList<Version> versions)
{

    var stringBuilder = new StringBuilder("## Evolutions");
    stringBuilder.AppendLine().AppendLine();

    foreach (var version in versions)
    {
        stringBuilder.Append("### ")
            .AppendLine(version.Name)
            .AppendLine();

        IEnumerable<string> evolutions = version.Evolutions.Where(s => !string.IsNullOrWhiteSpace(s));
        if (evolutions.Any())
        {
            stringBuilder.Append("* ")
                .AppendLine(string.Join(separator, evolutions))
                .AppendLine();
        }
    }

    return stringBuilder.ToString().TrimEnd(Environment.NewLine.ToArray());
}  

Another hint regarding readability is to add vertical space (new lines) to group related code together and separate unrelated code.

\$\endgroup\$
  • \$\begingroup\$ I rewrite WriteVersions() with your great implementation. Also, now it is called FormatVersions(). Nice usage of TrimEnd()! \$\endgroup\$ – aloisdg says Reinstate Monica Oct 6 '15 at 11:43
  • \$\begingroup\$ In ExtractSV(), why maxVersionParts is a private static readonly instead of a const inside the method? \$\endgroup\$ – aloisdg says Reinstate Monica Oct 6 '15 at 11:52
  • \$\begingroup\$ Just a personal preference \$\endgroup\$ – Heslacher Oct 6 '15 at 11:57
  • \$\begingroup\$ Ok so I will switch for a const. Thank you for the answer. \$\endgroup\$ – aloisdg says Reinstate Monica Oct 6 '15 at 11:59

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.