Modify casing of marked letters
15
$begingroup$
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();
}
}
}
c# beginner strings console
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
asked Mar 26 at 10:13
FreddyFreddy
762
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
$endgroup$
add a comment |
15
$begingroup$
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();
}
}
}
c# beginner strings console
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
asked Mar 26 at 10:13
FreddyFreddy
762
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
$endgroup$
add a comment |
15
15
15
2
$begingroup$
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();
}
}
}
c# beginner strings console
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
asked Mar 26 at 10:13
FreddyFreddy
762
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
$endgroup$
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();
}
}
}
c# beginner strings console
c# beginner strings console
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
asked Mar 26 at 10:13
FreddyFreddy
762
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
asked Mar 26 at 10:13
FreddyFreddy
762
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
edited Mar 26 at 17:46
Sᴀᴍ Onᴇᴌᴀ
9,98762166
9,98762166
asked Mar 26 at 10:13
FreddyFreddy
762
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked Mar 26 at 10:13
FreddyFreddy
762
asked Mar 26 at 10:13
FreddyFreddy
762
762
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Freddy is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |
add a comment |
3 Answers
3
active
oldest
votes
7
$begingroup$
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
StringBuilderinstead of of the char arraynewText, 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.
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
$endgroup$
add a comment |
4
$begingroup$
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
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
$endgroup$
add a comment |
3
$begingroup$
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...
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
$endgroup$
add a comment |
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Freddy is a new contributor. Be nice, and check out our Code of Conduct.
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216232%2fmodify-casing-of-marked-letters%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
7
$begingroup$
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
StringBuilderinstead of of the char arraynewText, 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.
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
$endgroup$
add a comment |
7
$begingroup$
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
StringBuilderinstead of of the char arraynewText, 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.
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
$endgroup$
add a comment |
7
7
7
$begingroup$
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
StringBuilderinstead of of the char arraynewText, 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.
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
$endgroup$
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
StringBuilderinstead of of the char arraynewText, 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.
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
edited Mar 26 at 11:53
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
answered Mar 26 at 11:43
JanDotNetJanDotNet
7,0131339
7,0131339
add a comment |
add a comment |
4
$begingroup$
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
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
$endgroup$
add a comment |
4
$begingroup$
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
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
$endgroup$
add a comment |
4
4
4
$begingroup$
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
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
$endgroup$
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
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
edited Mar 26 at 17:55
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
answered Mar 26 at 13:44
CiaPanCiaPan
1,3721513
1,3721513
add a comment |
add a comment |
3
$begingroup$
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...
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
$endgroup$
add a comment |
3
$begingroup$
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...
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
$endgroup$
add a comment |
3
3
3
$begingroup$
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...
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
$endgroup$
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...
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
edited Mar 26 at 15:35
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
answered Mar 26 at 15:28
Henrik HansenHenrik Hansen
8,24511231
8,24511231
add a comment |
add a comment |
Freddy is a new contributor. Be nice, and check out our Code of Conduct.
draft saved
draft discarded
Freddy is a new contributor. Be nice, and check out our Code of Conduct.
Freddy is a new contributor. Be nice, and check out our Code of Conduct.
Freddy is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216232%2fmodify-casing-of-marked-letters%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
