Trjtdtk

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();

 
 

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() 
 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('');

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