current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I was trying just as a practice to implement substring on my own.

the Test is inside the Ctor just because it is easy.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication2
{
    public class Substring
    {
        public Substring()
        {
            string test = "giladdarmonwhatareyoudoing";
            string res = ApplySubString(5, test);
            string res2 = ApplySubString2(5, test);
        }

        public string ApplySubString(int i, string test)
        {
            char[] charArray = test.ToArray();
            StringBuilder sb = new StringBuilder();

            for (; i < charArray.Length; i++)
            {
                sb.Append(charArray[i]);    
            }
            return sb.ToString();       
        }

        public string ApplySubString2(int i, string test)
        {
            char[] charArray = new char[test.Length - i];
            Array.Copy(test.ToCharArray(), i, charArray, 0, test.Length - i);
            return new String(charArray);
        }
    }
}

Please Let me know if you think I'm missing something or can offer a faster way.

share|improve this question
    
One item that lots of people appear to be skipping over: your StringBuilder. You know the length of the StringBuilder: String.Length-Offset. Pass it along to the StringBuilder constructor to stop reallocations. – RomSteady Dec 16 '14 at 18:54

4 Answers 4

up vote 8 down vote accepted

Assorted things pop into my head here.

  1. What's wrong with using the String.Substring(Int32) method?

  2. At first glance, I would tend to think your 2nd method would be faster, but they really aren't the same. If input i is greater than test.Length, the 2nd method will throw an exception but the first method would return an empty string.

  3. I'd suggest having the test string be the first parameter since it's really the object that you are processing upon. Plus you could then make them static extension methods to the input string.

EDIT - Adding extension method:

namespace System
{
    public static class GiladStrExt
    {
        public static string ApplySubString(this string value, int startIndex)
        {
            // You can invoke an extension method as a straight-up call,
            // e.g. GiladStrExt.ApplySubString(test, 5) and the value
            // for test could be null.
            if (value == null) throw new ArgumentNullException("value");
            if ((value.Length == 0) && (startIndex == 0)) return string.Empty;
            // Usually one uses the extension method with non-null values
            // so all we need to worry about is startIndex compared to value.Length.
            if ((startIndex < 0) || (startIndex >= value.Length)) throw new ArgumentOutOfRangeException("startIndex");
            var sb = new StringBuilder();
            for (int i = startIndex; i < value.Length; i++)
            {
                sb.Append(value[i]);
            }
            return sb.ToString();
        }
    }
}
share|improve this answer
    
I did it to practice – Gilad Dec 15 '14 at 18:59
up vote 4 down vote

Why not just append the elements of the string test without allocating space for an array? I mean like this:

public string ApplySubString(int i, string test)
    {
        StringBuilder sb = new StringBuilder();

        for (int j = i; j < test.Length; j++)
        {
            sb.Append(test[j]);
        }
        return sb.ToString();
    }

And, as Rick stated, you should probably make sure that i is not greater than the string test's length, maybe through the use of an assert. Also, I am not 100% sure about this, but the Array.Copy in your second method is probably doing the same thing as your first (i.e looping through the array, copying character by character), so I would prefer the first method method without the char array, as I have shown above, as in the above method, you are allocating only a StringBuilder, whereas in the second, you are allocating both an array and a new string.

share|improve this answer
    
right you are, I forgot about it. – Gilad Dec 15 '14 at 20:12
    
Actually the stringbuilder method is allocating a stringbuilder and a new string. So that point is probably moot. – tinstaafl Dec 16 '14 at 18:27
up vote 2 down vote

Personally I wouldn't do this with a loop

    public string ApplySubString(int i, string test)
    {
        char[] charArray = test.ToArray();
        StringBuilder sb = new StringBuilder();

        for (; i < charArray.Length; i++)
        {
            sb.Append(charArray[i]);    
        }
        return sb.ToString();       
    }

I don't like it at all

I would do it like this

public string ApplySubString(int start, string test)
{
    char[] charArray = test.ToArray();
    StringBuilder sb = new StringBuilder();

    for (int i = start; i < charArray.Length; i++)
    {
        sb.Append(charArray[i]);    
    }
    return sb.ToString();    
}

and then the only thing that is left to worry about it the 0-based array and how are you going to let the user know that you aren't taking into consideration that it is 0-based? Are you just going to assume that they always use the 0-base?

Also, there is no check to make sure that the input for start isn't bigger than charArray.Length or equal to.

share|improve this answer
up vote 1 down vote

Please Let me know if you think I'm missing something

You could simplify without using an external loop by returning a new string constructed from the ToCharArray overload that accepts a startindex and a length:

static string SubStr(string input,int startindex, int length)
{
    return new string(input.ToCharArray(startindex,length));
}

static string SubStr(string input, int startindex)
{
    return new string(input.ToCharArray(startindex, input.Length - startindex));
}

This method will throw the same exceptions as the String.Substring method. However, through the use of some simple conditionals one could easily handle the exceptions differently.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.