patterncsharpMinor
Slow two-strings comparer
Viewed 0 times
twostringscomparerslow
Problem
I have 2 strings for example:
or
I checked that second string is the same or not like first string but shifted.
How can I improve this code to make it faster? Also, it will be nice if someone could explain what makes this 'program' slow.
abcabc and bcabcaor
aaaabb and abaabaI checked that second string is the same or not like first string but shifted.
bcabca = ..abca + bc...using System;
public class TOPSES
{
static string t1, t2;
static char[] t1c;
static char[] t2c;
static void Main()
{
int t;
t = int.Parse(Console.ReadLine());
while (t > 0)
{
t1 = Console.ReadLine();
t2 = Console.ReadLine();
string result = "no";
t1c = t1.ToCharArray();
t2c = t2.ToCharArray();
result = ChangeString(t1c[0], 0);
Console.WriteLine(result);
t--;
}
}
public static string ChangeString(char letter, int startIndex)
{
int index = t2.IndexOf(letter, startIndex);
if (index == -1)
return "no";
else
{
string newString = t2.Remove(0, index);
newString += t2.Remove(index, t2.Length - index);
if (t1 != newString)
return ChangeString(letter, ++index);
return "yes";
}
}
}How can I improve this code to make it faster? Also, it will be nice if someone could explain what makes this 'program' slow.
Solution
This program is organized in a bizarre way.
I would expect the program to follow this outline:
Notes:
The algorithm you use is also too complicated: you're looking for places in
This involves just one string concatenation, and no slicing and dicing.
t1,t2,t1c, andt2care static variables, which essentially make them "global" state. This makes your code difficult to follow.
t2cis a write-only variable — assigned but never used.
t1cis only barely used — you're only ever interested in the first character.
- It's not obvious what the purpose of
ChangeString()is. What are you changing? At first glance, it appears to transform the input (which consists of a singleletterand astartIndex) into a "yes"/"no" string.
I would expect the program to follow this outline:
using System;
public class TOPSES
{
public static void Main()
{
for (int t = int.Parse(Console.ReadLine()); t >= 0; t--)
{
string s1 = Console.ReadLine(),
s2 = Console.ReadLine();
Console.WriteLine(IsRotation(s1, s2) ? "yes" : "no");
}
}
public static bool IsRotation(string s1, string s2)
{
…
}
}Notes:
Main()should bepublic.
- A
forloop gathers all the code related tottogether, to make it easy to see what it's for.
- Avoid unnecessarily separating declarations and assignments.
The algorithm you use is also too complicated: you're looking for places in
t2 that start with the initial character of t1, then checking if t2, if sliced and reassembled there, matches t1. But why use string.IndexOf(Char, Int) when you could just do string.IndexOf(string)?public static bool IsRotation(string s1, string s2)
{
return s1.Length == s2.Length &&
(s2 + s2).IndexOf(s1) >= 0;
}This involves just one string concatenation, and no slicing and dicing.
Code Snippets
using System;
public class TOPSES
{
public static void Main()
{
for (int t = int.Parse(Console.ReadLine()); t >= 0; t--)
{
string s1 = Console.ReadLine(),
s2 = Console.ReadLine();
Console.WriteLine(IsRotation(s1, s2) ? "yes" : "no");
}
}
public static bool IsRotation(string s1, string s2)
{
…
}
}public static bool IsRotation(string s1, string s2)
{
return s1.Length == s2.Length &&
(s2 + s2).IndexOf(s1) >= 0;
}Context
StackExchange Code Review Q#23091, answer score: 5
Revisions (0)
No revisions yet.