0

I'm following a beginner C# guide that is teaching how to use public methods. I understand very little about how to properly use methods/functions, so sorry in advance if this question is obvious. I researched a bunch of questions asking the same thing, but wasn't able to find an answer for this situation.

The program is supposed to take the string text, send it to the CheckDuplicate function, and determine if it contains more than one of the same number. If so it should return string result with the value "Duplicate" or "No Duplicate", and then display it on the console.

Right now references to string return under the CheckDuplicate function have the error "variable assigned but its value is never used" and the program doesn't return "Duplicate" or "No Duplicate" when inputting a string.

class Program
{
    public static string result;
    static void Main(string[] args)
    {
        Console.WriteLine("Enter several numbers separated by -");
        string text = Console.ReadLine();
        if (String.IsNullOrEmpty(text))     
        {
            Console.WriteLine("Empty");
        } 

        else
        {
            result = CheckDuplicate(text);
        }
        Console.WriteLine(result);
    }

    public static string CheckDuplicate(string text)
    {
        var textArray = text.Split('-');    
        int[] textArrayNum = textArray.Select(s => int.Parse(s)).ToArray();
        if (textArrayNum.Length != textArrayNum.Distinct().Count()) 
        {
            string result = "Duplicate";
        }
        else
        {
            string result = "No Duplicate";
        }
        return result;
    }
}
JohnZ12
  • 183
  • 3
  • 14
  • 2
    You are defining a new string inside your `if` and `else` blocks, but returning the static string defined before Main. Remove the `string` -initializer from the assignments in your if and else. – sibel1us Oct 01 '18 at 20:24

1 Answers1

3

You´re cnfusing yourself because you have multiple variables with the same name that overlap in their scope.

  1. The most outer-scope is the class-level, where you have a static field result.

  2. Then you have another result defined in CheckDuplicate that don´t have anything to do with the field from above. More precisely you have three different results in that method, two in the different if/else-statements and one in the outer-scope.

    public static string CheckDuplicate(string text)
    {
        var textArray = text.Split('-');    
        int[] textArrayNum = textArray.Select(s => int.Parse(s)).ToArray();
        if (textArrayNum.Length != textArrayNum.Distinct().Count()) 
        {
            string result = "Duplicate";  // most inner scope, hides the static field
        }
        else
        {
            // same level as before but completely unrelated to that one, also hides the static field
            string result = "No Duplicate";
        }
        return result;  // references the static field
    }
    

Anyway you could easily avoid such confusion by using meaningful names for your variables. In particular a field name result seems odd as it indicates that your entire class has some kind of a result which is very unlikeley and thus should be replaced by something like IsDuplicate. A method on the other hand may have a result. Having said this its usually a good idea to limit the scope of variables as much as possibles.

In your example however you even can completely omit the static field, as you´re only using the methods return-value in Main. Just use a local variable and print it to the console:

static void Main(string[] args)
{
    ...
    var result = string.IsNullOrEmpty(text) ? "Empty" : CheckDuplicate(text);
    Console.WriteLine(result);
}

Making your method returning directly in the if/else-blocks also reduces that kind of error:

public static string CheckDuplicate(string text)
{
    var textArray = text.Split('-');    
    int[] textArrayNum = textArray.Select(s => int.Parse(s)).ToArray();
    if (textArrayNum.Length != textArrayNum.Distinct().Count()) 
    {
        return "Duplicate";
    }
    else
    {
        return "No Duplicate";
    }
}
MakePeaceGreatAgain
  • 35,491
  • 6
  • 60
  • 111