Modify casing of marked letters

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.

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 

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...