6
\$\begingroup\$

I am given two strings and return the position if in the larger string the smaller string is found. If not I am returning -1 for position. Any suggestions for improving my code.

using System;
using System.Collections.Generic;
using System.Linq;

namespace Reverse
{
    class MainClass
    {
        static int IsFound (string stringOne, string stringTwo){
            int j = 0;
            int position = -1;
            int length = stringTwo.Length - stringOne.Length;
            for (int i = 0; i < length; i++) {
                if (stringOne [j] == stringTwo [i]) {
                    position = i;
                    while (stringOne [j] == stringTwo [i]) {
                        j++;
                        i++;
                        if (stringOne.Length == j) {
                            return position;
                        }
                    }
                    j = 0;
                    position = -1;
                } 
            }
            return position;
        }
        public static void Main (string[] args)
        {
            string stringOne = "charter";
            string stringTwo = "cstarchabsccharldmscharterfadgafga";
            int test = IsFound (stringOne, stringTwo);
            Console.WriteLine(test);
            string stringThree = "atfc";
            test = IsFound (stringThree, stringTwo);
            Console.WriteLine(test);
        }
    }
}
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

This is for an interview. Don't test from the console, write and use unit tests. I don't mean to be harsh, but I would rule out any submissions that were testing code manually from a console program like this. Doing it this way means you have to visually scan for correctness. It's all too easy to make a mistake. You're also more likely to consider edge cases when writing proper unit tests too.

namespace Reverse
{
  class MainClass

The namespace and class have nothing to do with the task at hand. They both deserve more thought and care.

On the other hand, IsFound would be a pretty good name if the method returned a bool. It doesn't however, and your parameter names don't make it clear which string is being searched, and which one is being searched for.

static int IsFound (string stringOne, string stringTwo){

Again, more thought needs to go into your naming. Don't get so caught up in the algorithm that you neglect the basics. I'd rather have a programmer on my team that knows how to name things than someone who can write an efficient string search algorithm.

I'm assuming that they've explicitly said to reinvent the wheel, because this function is already part of the .Net framework: String.IndexOf(string). I would expect a professional to use that instead of rolling their own. I mention it because you might want to mention it to them whether or not they explicitly said to reinvent the wheel.

\$\endgroup\$
4
\$\begingroup\$

Small improvement here:

Your IsFound method is making an assumption that stringTwo is longer than stringOne. Might I suggest:

Math.Abs(stringOne.Length - stringTwo.Length)

This way length will never be negative and the for loop will always execute.

\$\endgroup\$
1
  • \$\begingroup\$ Worth noting that the method still returns -1 when stringTwo isn't longer than stringOne - very good point nonetheless, welcome to Code Review! \$\endgroup\$ Aug 20, 2015 at 3:10
2
\$\begingroup\$

Style

Mixed/inconsistent bracing style is something to improve. Pick one, stick to it.

  1. "next line"

    namespace Reverse
    {
        class MainClass
        {
            public static void Main (string[] args)
            {
    
  2. "same line"

    static int IsFound (string stringOne, string stringTwo){
        for (int i = 0; i < length; i++) {
            if (stringOne [j] == stringTwo [i]) {
                position = i;
                while (stringOne [j] == stringTwo [i]) {
    

I prefer "next line" bracing as it's more C#-ish, but what matters is consistency, and that consistency isn't found in the code you presented.


Whitespace is also either inconsistent, or just non-standard:

public static void Main (string[] args)
static int IsFound (string stringOne, string stringTwo)

I'd remove the space between the method's name and the parentheses:

public static void Main(string[] args)
static int IsFound(string stringOne, string stringTwo)

That's something Visual Studio automaticallly adjusts, normally - as is the whitespace between a type and the square brackets:

string[] args

That's how Visual Studio automatically aligns them, and that's how array indices are most legible. This looks awkward:

stringOne [j] == stringTwo [i]

Compared to:

stringOne[j] == stringTwo[i]

Bug?

Where does it say that stringOne has to be the shorter string?

int length = stringTwo.Length - stringOne.Length;
for (int i = 0; i < length; i++) {
    if (stringOne [j] == stringTwo [i]) {

What happens if it isn't? You're returning -1 as would be expected.. but it's not clear from the code that it's intended to be that way.

What happens if either string is empty? What if either argument is null? Did you consider that possibility? Or perhaps you made assumptions about the requirements?


Structure

RubberDuck's answer already addresses the naming and return type, and most of what I'd have to say here.

The first thing I would have done, would have been to create a class. And even that would have been wrong: the first thing that should be done, is to create a test class, and document the requirements up front:

[TestMethod]
public void ReturnsZeroBasedIndexOfFirstOccurrenceOfShorterString()
{
}

[TestMethod]
public void ReturnsMinusOneWhenNotFound()
{
}

[TestMethod]
public void ReturnsZeroWhenShorterStringIsEmpty()
{
    // your code doesn't seem to be doing this
}

[TestMethod]
[ExpectedException(typeof(ArgumentNullException))]
public void ThrowsArgumentNullExceptionWhenShorterStringIsNull()
{
    // your code throws a surprising NullReferenceException in this case
}

So I'd have gone with a class, named it something that makes it clear what the purpose is (what's a MainClass object supposed to be doing anyway?), and then implement the code that makes all these tests pass: even if you don't have the time to complete the exercise, the evaluator/interviewer has enough material to read your mind, and sees how you're working, where you're headed, what's left to implement and how that unimplemented code is supposed to behave.


Lastly, RubberDuck's answer makes another very good point - knowing your tools could very well be what the interviewer is expecting here, and this could very well be the top-scoring code:

// rule #1: know your tools
return longest.IndexOf(shortest);

And all the above tests would pass.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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