2

I have made a function that in my opinion will always return a value, but the function still says

All code paths does not return a value

Am I missing something?

public static bool CheckIfSignatureAlreadySignedByUser(SPSite site, SPWeb web, int RowID) { RevertToAppPool revert = new RevertToAppPool(); try { revert.UseAppPoolIdentity(); string dbConnectionString = site.WebApplication.Properties["dbConnection"].ToString(); using (dbDWDataContext dataContext = new dbDWDataContext(dbConnectionString)) { var signatures = dataContext.CM_Signatures.Where(c => c.ParagraphID == RowID).ToList(); if (signatures.Any()) { foreach (var sig in signatures) { if (sig.LoginName.ToLower() == web.CurrentUser.LoginName.ToLower()) { return false; } else return true; } } else { return true; } } } catch (Exception error) { SEPUtilities.WriteErrorToLog("Error in DWUtilities.AddSignature: {0}", error.ToString()); return false; } finally { revert.ReturnToImpersonatingCurrentUser(); } }
Improve this question
1
  • 1
    you use foreach and then if else that both return from method? your foreach is useless i think. it doesnt make sense because it will always return from first iteration. Commented Oct 23, 2015 at 12:35

4 Answers 4

Reset to default
9

The compiler doesn't know that signatures always will yield at least one result, since you checked it before:

if (signatures.Any()) { foreach (var sig in signatures) { ... } // problem here.

What it wants you to do is return anything after your foreach statement. (And for what it is worth, signatures can change between the Any and foreach statements due to another thread modifying it).

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

4 Comments

But can signatures, realistically, be changed? It's a function-scoped instance, not a member variable, that isn't returned out of scope. Perhaps the static analyzer can't 100% determine that?
Indeed, but what if Any starts a thread, modifying it in the background? The compiler doesn't know all this.
You're right. It is passed out of scope - to the Any extension method. Good call. Any could, theoretically, even without threads, return true while cleaning out the list.
@AvnerShahar-Kashtan Furthermore, the instance is actually not function-scoped, only the variable is. The instance is instantiated elsewhere and only a reference is obtained via the external calls to Where and ToList, and so there may well be other references to it.
4

@Patrick Hofman has the right answer.

That said, I would refactor:

if (signatures != null && signatures.Any()) { foreach (var sig in signatures) { if (sig.LoginName.ToLower() == web.CurrentUser.LoginName.ToLower()) { return false; } else return true; } } else { return true; }

to:

return !signatures.Any(z => z.LoginName.ToLower() == web.CurrentUser.LoginName.ToLower());
Improve this answer

3 Comments

I agree with the refactoring, but note that it does NOT do the same as the original code! This would only be a correct refactoring if you are correcting a bug in the OP's code.
@MatthewWatson True, but I assumed the OP's code was buggy: the Where clause that was written should only return one element (or the code was really wrong: no OrderBy and then an enumeration that only takes the first element (which one? dunno as there is no order) into account...), so it should be equivalent.
Yep, I also think the OP's code has a bug. Seems that some random downvoter disagrees, so he's downvoted my answer (but seems to think your's is OK ;) Anyway, have an upvote.
1

Others have already answered, but here's some additional information.

I think that your loop is incorrect:

var signatures = dataContext.CM_Signatures.Where(c => c.ParagraphID == RowID).ToList(); if (signatures.Any()) { foreach (var sig in signatures) { if (sig.LoginName.ToLower() == web.CurrentUser.LoginName.ToLower()) { return false; } else return true; } } else { return true; }

The foreach loop will only iterate once. If you are trying to find out if no signature matches the current user, then this will fail if the first signature does match, even if a later one doesn't match, because the return false will terminate the loop early.

Perhaps you really mean to do this:

return !signatures.Any(sig => string.Compare(sig.LoginName, web.CurrentUser.LoginName, StringComparison.OrdinalIgnoreCase) == 0);
Improve this answer

3 Comments

@downvoter: Can you explain what you think is wrong? And if there is something wrong, then Ken2K's answer is also wrong (but you didn't downvote that). I'm thinking that you are likely missing something...
Doesn't deserve a downvote at all. So here's my +1 to balance :)
@downvoter And now we see that you are indeed missing something, since the OP has marked Ken's answer as the correct one. :) (and thanks Ken ;)
0

I guess the branch analysis for if-foreach { if-else } fails because VS does not now that signatures.Any will not enter the if. Instead try to write 

public static bool CheckIfSignatureAlreadySignedByUser(SPSite site, SPWeb web, int RowID) { RevertToAppPool revert = new RevertToAppPool(); try { revert.UseAppPoolIdentity(); string dbConnectionString = site.WebApplication.Properties["dbConnection"].ToString(); using (dbDWDataContext dataContext = new dbDWDataContext(dbConnectionString)) { var signatures = dataContext.CM_Signatures.Where(c => c.ParagraphID == RowID).ToList(); if (signatures != null && signatures.Any()) { foreach (var sig in signatures) { if (sig.LoginName.ToLower() == web.CurrentUser.LoginName.ToLower()) { return false; } else return true; } } return true; } } catch (Exception error) { SEPUtilities.WriteErrorToLog("Error in DWUtilities.AddSignature: {0}", error.ToString()); return false; } finally { revert.ReturnToImpersonatingCurrentUser(); } }
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.