Modify casing of marked letters

Question

Trying to learn coding in my 30s.

Could anybody review my code and give me some feedback if possible? It works, but I am sure there are better ways to solve this problem.

The problem:

A text is given. Write a program that modifies the casing of letters to uppercase at all places in the text surrounded by <upcase> and </upcase> tags. Tags cannot be nested.

Here is what I've come up with:

namespace Task6Casing
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.Write("Text: ");
            string text = Console.ReadLine();

            char[] textNew = new char[text.Length];

            int i = 0, j = 0, k = 0;

            while (i < text.Length)
            {
                if (text.IndexOf("<upcase>", k) < 0)            // check if there is any <upcase> tag
                {                                               // if no, copy everyting
                    while (i < text.Length)
                    {
                        textNew[j++] = text[i++];
                    }
                }
                else                                  
                {
                    while (i < text.IndexOf("<upcase>", k))     // if there is an <upcase> tag, copy letters until the tag
                    {
                        textNew[j++] = text[i++];
                    }

                    i += 8;                                     // move index i to the position right next to the <upcase> tag
                    k = i;
                }

                if (text.IndexOf("</upcase>", k) < 0)           // check if there is any </upcase> tag
                {                                               // if no, copy everyting in CAPITAL letters
                    while (i < text.Length)
                    {
                        textNew[j++] = Char.ToUpper(text[i++]);
                    }
                }
                else
                {
                    while (i < text.IndexOf("</upcase>", k))    // if there is an </upcase> tag, copy letters in CAP letters until the tag
                    {
                        textNew[j++] = Char.ToUpper(text[i++]);
                    }

                    i += 9;                                     // move index i to the position right next to the </upcase> tag
                }
            }

            foreach(char c in textNew)
            {
                Console.Write(c);
            }
            Console.WriteLine();

        }
    }
}

Answer 1

Your code is easy to understand and very performant.

Some improvements:

• String for start / end tag could be stored as constant. That has the advantage that it can be changed on one central location and the length of the string can be accessed like i += START_TAG.Length
• When using a StringBuilder instead of of the char array newText, the running variable 'j' can be dropped.
• The 2 code parts

            if (text.IndexOf("<upcase>", k) < 0)            // check if there is
            any <upcase> tag
            {                                               // if no, copy everyting
                while (i < text.Length)
                {
                    textNew[j++] = text[i++];
                }
            }
            else                                  
            {
                while (i < text.IndexOf("<upcase>", k))     // if there is an <upcase> tag, copy letters until the tag
                {
                    textNew[j++] = text[i++];
                }

                i += 8;                                     // move index i to the position right next to the <upcase> tag
                k = i;
            }

and

            if (text.IndexOf("</upcase>", k) < 0)           // check if there is any </upcase> tag
            {                                               // if no, copy everyting in CAPITAL letters
                while (i < text.Length)
                {
                    textNew[j++] = Char.ToUpper(text[i++]);
                }
            }
            else
            {
                while (i < text.IndexOf("</upcase>", k))    // if there is an </upcase> tag, copy letters in CAP letters until the tag
                {
                    textNew[j++] = Char.ToUpper(text[i++]);
                }

                i += 9;                                     // move index i to the position right next to the </upcase> tag
            }

are very simlar. Probably it is possible to create one more generic code fragment that coveres both cases.

---

Since your solution is still understandable for such a simple use case, it will fast become unmaintainable if the use case becomes more complex. Therefore, it makes sense to think about a more abstract OOP concepts to model the solution.

One alternative impl. (that is probably over engineered for the given problem) gives an idea how a more object oriented design could look like:

        public class Tag
        {
            private readonly Func<char, char> map;
            public Tag(string start, string end, Func<char, char> map)
            {
                this.Start = start;
                this.End = end;
                this.map = map;
            }

            public string Start { get; }
            public string End { get; }
            public char Map(char input) => this.map(input);
        }

        public class TagProcessor
        {
            private readonly Tag tag;
            private readonly StringBuilder output = new StringBuilder();

            private string input;
            private bool isTagOpen;
            private int index;

            public TagProcessor(Tag tag)
            {
                this.tag = tag;
            }

            public string Process(string input)
            {
                this.input = input;
                this.index = 0;
                this.isTagOpen = false;
                this.output.Clear();

                do
                {
                    var tagProcessed = this.TryOpenTag() || this.TryCloseTag();
                    if (!tagProcessed)
                    {
                        this.ApplyCurrentChar();
                    }
                }
                while (this.MoveNext());

                return output.ToString();
            }

            private bool IsEndTag() => input.IndexOf(tag.End, this.index) == this.index;

            private bool IsStartTag() => input.IndexOf(tag.Start, this.index) == this.index;

            private bool MoveNext()
            {
                index++;
                return index < this.input.Length;
            }

            private void ApplyCurrentChar()
            {
                var inputChar = this.input[this.index];
                var transfomed = this.isTagOpen ? tag.Map(inputChar) : inputChar;
                this.output.Append(transfomed);
            }

            private bool TryOpenTag()
            {
                if (!isTagOpen && IsStartTag())
                {
                    this.index += this.tag.Start.Length - 1;
                    this.isTagOpen = true;
                    return true;
                }

                return false;
            }

            private bool TryCloseTag()
            {
                if (isTagOpen && IsEndTag())
                {
                    this.index += this.tag.End.Length - 1;
                    this.isTagOpen = false;
                    return true;
                }

                return false;
            }
        }

        public static void Main(string[] args)
        {
            var processor = new TagProcessor(new Tag("<upcase>", "</upcase>", char.ToUpper));

            var test = new[]
            {
                "abc<upcase>test</upcase>",
                "abc<upcase>test",
                "abc<upcase></upcase>test",
                "abc<upcase>test</upcase>test",
                "abc<upcase>te<upcase>st</upcase>test",
                "a</upcase>bc<upcase>te<upcase>st</upcase>te</upcase>st",
            };

            foreach (var t in test)
                Console.WriteLine(t + ": " + processor.Process(t));
            Console.ReadLine();
        }

The advantages are, that this solution remains readable if the complexity grows (e.g. more tags were added) and it allows to change / extend the logic without understanding the whole parsing logic. Further more, each method has a single pupose which increases comprehensibleness.

Answer 2

This loop

    while (i < text.IndexOf("<upcase>", k))
    {
        ....
    }

invokes IndexOf multiple times for nothing. I'd call it just once and use the result:

    const int upcasePos = text.IndexOf("<upcase>", k);
    if (upcasePos < 0)
    {
        while (i < text.Length)
        {
            textNew[j++] = text[i++];
        }
    }
    else                                  
    {
        while (i < upcasePos)
        {
            textNew[j++] = text[i++];
        }

        ....
    }

Edit

You can also speed things up by getting rid of char-by-char iteration, and instead processing longer parts of the string at once with standard routines. Here's an outline:

    StringBuilder sb = new StringBuilder();

    for (int k = 0; k < text.length; )
    {
         int tagOpen = text.IndexOf("<upcase>", k));
         if (tagOpen < 0)
         {
             sb.Append(text.Substring(k));  // take the tail
             break;
         }

         sb.Append(text.Substring(k, tagOpen - k));
         k = tagOpen + 8;                   // skip the tag

         int tagClose = text.IndexOf("</upcase>", k));
         if (tagClose < 0)
         {
             sb.Append(text.Substring(k).toUpper());  // take the tail in upper case
             break;
         }

         sb.Append(text.Substring(k, tagClose - k).toUpper());
         k = tagClose + 9;                  // skip the tag
    }

    result = sb.ToString();   // return this

Answer 3

Below is a refactoring of your algorithm with some inline comments.

string Review(string text)
{
  const string startTag = "<upcase>"; // Use declared string rather than string literals
  const string endTag = "</upcase>";

  // textNew is a somewhat "backward" name. result or newText would be better
  char[] result = new char[text.Length];
  // Instantiate each variable on a single line. It's easier to find and maintain
  // Provide some meaningful names instead for i, j and k. (i, j and k may be alright in a lessser complex context, but here they easily lose their meaining through the loop)
  int curIndex = 0;
  int resultIndex = 0;
  int searchIndex = 0;

  while (curIndex < text.Length)
  {
    if (text.IndexOf(startTag, searchIndex) < 0) 
    {        
      while (curIndex < text.Length)
      {
        result[resultIndex++] = text[curIndex++];
      }
    }
    else
    {
      // Repeatedly calling the same function with the same values is inefficient
      int startIndex = text.IndexOf(startTag, searchIndex);
      while (curIndex < startIndex)
      {
        result[resultIndex++] = text[curIndex++];
      }

      curIndex += startTag.Length; // Use the length of the tag string instead of a magic number
      searchIndex = curIndex;
    }

    if (text.IndexOf(endTag, searchIndex) < 0)
    {                                       
      while (curIndex < text.Length)
      {
        result[resultIndex++] = Char.ToUpper(text[curIndex++]);
      }
    }
    else
    {
      // Repeatedly calling the same function with the same values is inefficient
      int endIndex = text.IndexOf(endTag, searchIndex);
      while (curIndex < endIndex)
      {
        result[resultIndex++] = Char.ToUpper(text[curIndex++]);
      }

      curIndex += endTag.Length; // Use the length of the tag string instead of a magic number
    }
  }

  // Return the result instead of write it to the console.
  return new string(result.Take(resultIndex).ToArray()); // You have to truncate the char array in order to prevent a trailing new line
  //OR: return new string(textNew).TrimEnd('\0');
}

---

Below is some other approaches that you may find useful for inspiration:

string ToUpperInTags(string text)
{
  const string start = "<upcase>";
  const string stop = "</upcase>";

  List<string> fragments = new List<string>();
  int curIndex = 0;
  while (curIndex < text.Length)
  {
    int startIndex = text.IndexOf(start, curIndex);
    if (startIndex >= 0)
    {
      fragments.Add(text.Substring(curIndex, startIndex - curIndex));
      int stopIndex = text.IndexOf(stop, startIndex + start.Length);
      if (stopIndex < 0) throw new InvalidOperationException($"Unpaired start at {startIndex}");
      fragments.Add(text.Substring(startIndex + start.Length, stopIndex - (startIndex + start.Length)).ToUpper());
      curIndex = stopIndex + stop.Length;
    }
    else
    {
      fragments.Add(text.Substring(curIndex));
      break;
    }
  }

  return string.Join("", fragments);
}

string ToUpperInTags2(string text)
{
  string pattern = @"(?<start><upcase>)(?<content>[^<>]*)(?<stop></upcase>)?";
  foreach (Match match in Regex.Matches(text, pattern))
  {
    text = Regex.Replace(text, $"{match.Groups["start"]}{match.Groups["content"]}{match.Groups["stop"]}", match.Groups["content"].Value.ToUpper());
  }

  return text;
}

string ToUpperInTags3(string text)
{
  string pattern = @"<upcase>(?<content>[^<>]*)(</upcase>)?";
  return Regex.Replace(text, pattern, m => m.Groups["content"].Value.ToUpper());
}

They don't all behave exactly as yours, and are just provided for inspiration for further study...