2
\$\begingroup\$

The code is an implementation of looping text (similar to a circular buffer - wraps around when it reaches the edge of the defined bounds) with directional control.

The code is functional and works as intended, but I'm curious if there is anyway to improve the code even if it is only a small improvement (e.g. less garbage produced per loop cycle as a micro-optimisation, etc).

using System;
using System.Text;
using System.Threading;

namespace LoopingTextScroll
{
    internal class Program
    {
        private enum Direction
        {
            LeftToRight = -1,
            RightToLeft = 1
        }

        private static StringBuilder PadText(string text, int padding = 5)
        {
            var stringBuilder = new StringBuilder(text);
            stringBuilder.Append(new string(' ', padding));
            return stringBuilder;
        }

        private static void ShiftText(StringBuilder textBuilder, Direction direction)
        {
            var directionOffset = (int)direction;
            var textLength = textBuilder.Length;

            switch (direction)
            {
                case Direction.LeftToRight:
                {
                    for(var i = textLength - 1; i >= 0; i--) textBuilder[i] = textBuilder[Modulo(i + directionOffset, textLength)];
                    break;
                }

                case Direction.RightToLeft:
                {
                    var firstElement = textBuilder[0];
                    for(var i = 0; i < textLength; i++) textBuilder[i] = textBuilder[Modulo(i + directionOffset, textLength)];
                    textBuilder[textLength - 1] = firstElement;
                    break;
                }
            }
        }

        private static void UpdateLoop(string text, Direction direction = Direction.LeftToRight, int updateRate = 100)
        {
            var textBuilder = PadText(text);

            while (true)
            {
                ShiftText(textBuilder, direction);
                Console.Write($"\r{textBuilder}");
                Thread.Sleep(updateRate);
            }
        }

        private static int Modulo(int x, int m)
        {
            var remainder = x % m;
            return remainder < 0 ? remainder + m : remainder;
        }

        public static void Main(string[] args)
        {
            UpdateLoop("Beautiful Lie", Direction.RightToLeft);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ mhmm... are you sure this is working? Because this prints the text without cleaning the console so it just prints and prints and prints... is this how it should work? \$\endgroup\$ – t3chb0t Sep 30 '18 at 16:46
  • 1
    \$\begingroup\$ Take notice of the \r which will replace the text on screen. \$\endgroup\$ – Yaseen Ssenyonjo Sep 30 '18 at 16:49
  • \$\begingroup\$ ok, then I guess it must be linqpad's issue and it doesn't respect it... \$\endgroup\$ – t3chb0t Sep 30 '18 at 16:51
  • 1
    \$\begingroup\$ But I have to tell you... your one-liner for loops are an absolute no-go! \$\endgroup\$ – t3chb0t Sep 30 '18 at 16:54
  • \$\begingroup\$ Could you explain what's the Modulo method about and what x and m are and what you are calculating this for? \$\endgroup\$ – t3chb0t Sep 30 '18 at 16:58
2
\$\begingroup\$

Your implementation can be greatly improved by using the System.Memory package. This will give you the new Memory<char> type (and if you're using .net-core it's already part of the SDK). With that you can create views of the string without creting any garbage.

At the same time you can make it more LINQ friendly. For this you can create the Slide extension that will return the slices of each part of the string and that accepts a Func for generating the offset for slicing. This one runs infinitely.

public static IEnumerable<(Memory<char> X, Memory<char> Y)> Slide(this string text, Func<int, int> calcOffset)
{
    var span = new Memory<char>(text.ToArray());
    while (true)
    {
        var offset = calcOffset(text.Length);
        var left = span.Slice(0, offset);
        var right = span.Slice(offset, text.Length - offset);
        yield return (right, left);
    }
}

You should also separate the logic for sliding and encapsulate each part in its own method. So you now have two of them.

public static Func<int, int> LeftToRight()
{
    var i = -1;
    return length =>
    {
        i++;
        if (i > length)
        {
            i = 0;
        }
        return i;
    };
}

public static Func<int, int> RightToLeft()
{
    var i = -1;
    return length =>
    {
        i--;
        if (i < 0)
        {
            i = length;
        }
        return i;
    };
}

To print it you can create another helper that encapsulates that part. Here you should replace the Thread.Sleep with Task.Delay which doesn't block/freeze the thread. With TimeSpan you can set the delay to whatever you want.

public static async Task PrintAsync(this IEnumerable<(Memory<char> X, Memory<char> Y)> source, TimeSpan delay)
{
    foreach (var element in source)
    {
        await Task.Delay(delay);
        Console.Clear();
        Console.Write(element.X);
        Console.WriteLine(element.Y);
    }
}

When you put everything together it'll look like that:

using  static Slider;

class Program
{
    static async Task Main(string[] args)
    {
        await
            "Hallo World"
                //.Slide(LeftToRight())
                .Slide(RightToLeft())
                .Take(20) // remove to run infinitely
                .PrintAsync(TimeSpan.FromMilliseconds(100));
    }
}

This is now garbage-free and does not create any additional sub-strings.

\$\endgroup\$
1
\$\begingroup\$

I think the use of StringBuilder is making the code harder to read and isn't giving you a great benefit for the cost of read-ability or maintainability.

To add spaces to the end of a string there is a method build in PadRight. No need to create a new method just to use a string builder.

Also ShiftText seems over complicated and again just using string would make it more maintainable and easier to read. Compare it to this example

private static void ShiftText(ref string text, Direction direction)
{
    var textLength = text.Length;

    switch (direction)
    {
        case Direction.LeftToRight:
            text = text.Substring(textLength - 1) + text.Substring(0, textLength - 1);
            break;
        case Direction.RightToLeft:
            text = text.Substring(1, textLength - 1) + text.Substring(0, 1);
            break;
    }
}

No need for Modulo method or a loop to shift array of chars.

Then the UpdateLoop pads the text using the built in PadRight and passes it by ref.

private static void UpdateLoop(string text, Direction direction = Direction.LeftToRight, int updateRate = 100)
{
    var movingText = text.PadRight(text.Length + 5);

    while (true)
    {
        ShiftText(ref movingText, direction);
        Console.Write($"\r{movingText}");
        Thread.Sleep(updateRate);
    }
}

I wouldn't worry about micro optimizations and would recommend reading The Sad Tragedy Of Micro Optimization blog

If you really really wanted to be optimized the best would be to create a dictionary that contained all the strings shifted and just used the index position to get the string again to reuse. But I wouldn't go that way unless performance or memory usage was an issue.

\$\endgroup\$
  • \$\begingroup\$ About your last paragraph and the dictionary... nowadays this can be easily implemented and greatly optimized (nearly without effort) with the new System.Memory package and the Span<T> family. \$\endgroup\$ – t3chb0t Oct 1 '18 at 15:11
  • \$\begingroup\$ Using Substring definitely looks cleaner but I'm worried about it creating a constant stream of garbage which has to be collected, however thanks for the mention of PadRight that is a great improvement. \$\endgroup\$ – Yaseen Ssenyonjo Oct 1 '18 at 17:48

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.