HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Slow two-strings comparer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
twostringscomparerslow

Problem

I have 2 strings for example:

abcabc and bcabca

or

aaaabb and abaaba

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

  • t1, t2, t1c, and t2c are static variables, which essentially make them "global" state. This makes your code difficult to follow.



  • t2c is a write-only variable — assigned but never used.



  • t1c is 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 single letter and a startIndex) 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 be public.



  • A for loop gathers all the code related to t together, 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.