Not all code paths return a value - where?

Question

I have the following C# code. For reasons I won't go into, this is the required way of localising.

My problem is, I cannot for the life of me figure out what path is not returning a value. There are no other errors in the below code:

ResourceManager ResMain = new ResourceManager("TorrentSched.Main", typeof(Main).Assembly); /// <summary>Returns a localised string based on the Current UI Culture</summary> public string Str(string Name) { Boolean StringNotFound = false; switch(Thread.CurrentThread.CurrentUICulture.ToString()) { case "en-GB": switch(Name) { case "MinimizeToTray": return "Closing the program minimises it to the tray. To fully quit the program, right-click the icon in your tray and choose Exit."; default: StringNotFound = true; break; } break; default: StringNotFound = true; break; } if(StringNotFound) { StringNotFound = false; switch(Name) { case "AppName": return ResMain.GetString("$this.Text"); case "MinimizeToTray": return "Closing the program minimizes it to the tray. To fully quit the program, right-click the icon in your tray and choose Exit."; case "ReallyExit1": return "Do you really want to exit?"; case "ReallyExit2": return "Torrents will not be checked and downloaded until you launch the program again!"; default: return "String not found!"; } } }

the function presented is not complete. There is a problem on the bottom of the code, I guess. — Tigran
– Tigran, Commented Nov 28, 2012 at 13:48
What is the purpose of the if-block? As far a I can see, if code below the switch is executed, StringNotFound will always be true, so the if-block is not necessary, but it may very well confuse the code analysis. — Fredrik Mörk
– Fredrik Mörk, Commented Nov 28, 2012 at 13:50
@FredrikMörk That's the perfect point! Make that an answer and I'll choose that! — Danny Beckett
– Danny Beckett, Commented Nov 28, 2012 at 13:52
You don't have a return statement for the else condition. You don't need the if condition at all. The simple solution to this problem is have a single return outside of the if condition. I suggest you post on the code review stack exchange website there several bad habits outlined in this code. — Security Hound
– Security Hound, Commented Nov 28, 2012 at 14:09

Fredrik Mörk · Accepted Answer · 2012-11-28 13:54:49Z

5

What is the purpose of the if-block? As far a I can see, if code below the switch is executed, StringNotFound will always be true so the if-block is not necessary, but it may very well confuse the code analysis.

answered Nov 28, 2012 at 13:54

Fredrik Mörk

159k31 gold badges296 silver badges349 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Danny Beckett Over a year ago

Perfect answer. Strange though that Visual Studio doesn't realise that StringNotFound will never be false, therefore all code paths return a value.

Skizz Over a year ago

@DannyBeckett: Doing that level of analysis is hard to do and would slow the compilation time (see "The Halting Problem"). So it's a compromise between accuracy and time. Here, the compiler sees a return in the true block and no equivalent in the (missing) else block with no final return. So it assumes there's a possible path that can have no return value. It's not smart enough (by design) to statically analyse the code.

Honza Brestan · Accepted Answer · 2012-11-28 13:48:05Z

4

If StringNotFound is false, you don't return anything.

answered Nov 28, 2012 at 13:48

Honza Brestan

11k2 gold badges37 silver badges44 bronze badges

Comments

Michal Klouda · Accepted Answer · 2012-11-28 13:48:13Z

At the very end of your method, if StringNotFound is false:

if(StringNotFound) { [...] } // missing return statement here

Th3Minstr3l · Accepted Answer · 2012-11-30 10:57:42Z

Maybe you could refactor it so that at the bottom of your code you have a SINGLE return statement. And all your decision-making code simply 'sets' the return value.

Then in your switch(Name) block SET the value of what you want to return - then break out of the switch (rather than having a whole bunch of returns). IMO, this would make the code neater. Also, I think it'd make it easier to maintain.

ie.

switch(Name) { case "AppName": retString = ResMain.GetString("$this.Text"); case "MinimizeToTray": retString = "Closing the program minimizes it to the tray. To fully quit the program, right-click the icon in your tray and choose Exit."; case "ReallyExit1": retString = "Do you really want to exit?"; case "ReallyExit2": retString = "Torrents will not be checked and downloaded until you launch the program again!"; default: retString = "String not found!"; } ... return retString;

Myrtle · Accepted Answer · 2012-11-28 13:50:17Z

To prevent mistakes or puzzles like this, you better not do both of this (below) , as there is no single logical flow anymore when you:

Maintain a retVal variable, together with
Multiple uses of the return statement.

Choose one solution:

only return a retVal variable, or
return a default at the bottom of the function.

Olivier Jacot-Descombes · Accepted Answer · 2012-11-28 14:08:37Z

Consider using a dictionary

private static Dictionary<string,string> stringDict = new Dictionary<string,string>();

Add the strings

// Add default strings stringDict.Add("AppName", ResMain.GetString("$this.Text")); stringDict.Add("MinimizeToTray", "Closing the program ..."); stringDict.Add("ReallyExit1", "Do you really ..."); // Add culture specific strings stringDict.Add("en-GB;AppName", ResMain.GetString("$this.Text")); stringDict.Add("en-GB;MinimizeToTray", "Closing the program ..."); stringDict.Add("en-GB;ReallyExit1", "Do you really ...");

The you can get the strings very quickly with

// Get culture specific string string culture = Thread.CurrentThread.CurrentUICulture.ToString(); string s; If (stringDict.TryGetValue(culture + ";" + Name, out s)) { return s; } // Get default If (stringDict.TryGetValue(Name, out s)) { return s; } return String.Format("String '{0}' not found!", Name);

This is easier to maintain.

(As other have already pointed out, there is a return-statement missing at the very end of your method and the boolean variable is superfluous.)

Chadwick13 · Accepted Answer · 2012-11-28 14:31:39Z

0

if(StringNotFound)

If this is false there is no else statment to catch it.

use

if(StringNotFound)
{
//your code
}
Else
{
return "String not found!";
}

edited Nov 28, 2012 at 14:31

answered Nov 28, 2012 at 13:53

Chadwick13

3571 gold badge7 silver badges15 bronze badges

2 Comments

Security Hound Over a year ago

There is an error in this code. Its a single character so I can't propose an edit.

Chadwick13 Over a year ago

yer should make more senes now :)

Collectives™ on Stack Overflow

Not all code paths return a value - where?

7 Answers 7

2 Comments

Comments

Comments

Comments

Comments

Comments

2 Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

2 Comments

Comments

Comments

Comments

Comments

Comments

2 Comments

Linked

Related