How can I modify this C# code so that Visual Studio recognizes that I'm not an idiot?

Question

I have 3 lines that go

int selectedOrgId; 

foreach (Organization o in PD.orgs)
    if (o.orgname == selectedOrgName)
         selectedOrgId = o.orgid;

PD.cats.InsertOnSubmit(new Category {
    orgid = selectedOrgId,
    catname = newCatName
});

The middle line, the loop, is guaranteed, in the context of my program, to set a value for selectedOrgId. However, Visual Studio is flagging the last line because

Use of unassigned local variable 'selectedOrgId'

What is a way to solve this other than

int selectedOrgId = 69; 

foreach (Organization o in PD.orgs)
    if (o.orgname == selectedOrgName)
         selectedOrgId = o.orgid;

PD.cats.InsertOnSubmit(new Category {
    orgid = selectedOrgId,
    catname = newCatName
});

????

Although it works, it seems like an inelegant solution, since it involves a magic number. I want to know the proper C# style of solving this.

EDIT:

Looking at some of the discussion here, I should've specified that there is only such orgid in the database. My foreach statement should've been written like

foreach (Organization o in PD.orgs)
{
    if (o.orgname == selectedOrgName)
    {
         selectedOrgId = o.orgid;
         break;
    }
 }

Thanks for showing me some ways to better do this whole thing!

Visual Studio doesn't know anything about the context of your program... — Matt Mills, May 14 '15 at 17:16
[This](http://stackoverflow.com/questions/8417595/best-practice-for-initializing-object-attributes-in-oo-languages) states that best practice is to let it fail then run silently with a bad value. I would suggest, if you want to do an exception if selectOrgId is 0 after the loop. This would make your code much more intentional. — JasonWilczak, May 14 '15 at 17:17
The reason it is complaining is because `selectedOrgId` may never get set because of the `If` statement on line 2. If it never gets set then it will have an uninitialized value when you try to use it on the 3rd line which could cause problems. You need to make sure that a value gets set on all code paths, or change your logic to only use that variable when it has a known valid value. IMHO Visual Studio is right to warn you about this code. — Bradley Uffner, May 14 '15 at 17:29
This seems like a problem that should be easily resolved without having to learn Linq. int? should do it — The One, May 14 '15 at 17:48
Gave you a star for sense of humor. Thanks for making my day. :) — Ken Palmer, May 14 '15 at 18:00

Grant Winney · Answer 1 · 2015-05-14T17:36:10.363

It appears that you're iterating through a collection, trying to find a single matching value based on the organization name. You can use LINQ's SingleOrDefault() to find (at most) one match:

var selectedOrg = PD.orgs.SingleOrDefault(o => o.orgname == selectedOrgName);

Then only call InsertOnSubmit() if a value is found: (otherwise, selectedOrg will be null)

if (selectedOrg != null)
    PD.cats.InsertOnSubmit(new Category { orgid = selectedOrg.orgid, catname = newCatName });

score 3 · Answer 2 · answered May 14 '15 at 17:25

No.

What happens if the condition o.orgname == selectedOrgName is not satisfied for any value in PD.orgs? Then selectedOrgId will be left uninitialized.

However, the following code might be more 'elegant' according to your approach:

int selectedOrgId = PD.orgs.Single(o => o.orgname == selectedOrgName).orgid;
PD.cats.InsertOnSubmit(new Category { orgid = selectedOrgId, catname = newCatName });

Please note that your code will set selectedOrgId to the last instance of it, while mine will assume that only one exists.

This also throws an exception if no match is found or more than one match is found. — juharr, May 14 '15 at 17:59

Yuval Itzchakov · Answer 3 · 2015-05-14T17:37:56.367

2

You can use a int?:

int? selectedOrgId = null;

foreach (Organization o in PD.orgs)
{
    if (o.orgname == selectedOrgName)
    {
         selectedOrgId = o.orgid;
    }  
 }

 PD.cats.InsertOnSubmit(new Category 
 { 
        orgid = selectedOrgId,
        catname = newCatName 
 });

edited May 14 '15 at 17:37

answered May 14 '15 at 17:17

Yuval Itzchakov

146,575
32
257
321

1

What's the point of "int?" and assigning null to it when you can simply assign 0 to the variable? Besides, Category.orgid is not nullable. – Mehrzad Chehraz May 14 '15 at 17:20
2

@MehrzadChehraz because you want to be able to tell the diffrence between a id of 0 and no id found. The OP never showed what orgid was. – Scott Chamberlain May 14 '15 at 17:21
@Mehrzad 0 might be a valid value for an `int`. While `null` indicates an "empty value" in this case, which is completely different. – Yuval Itzchakov May 14 '15 at 17:22
Category.orgid is not nullable, since he didn't provided more info, you can't assume it is nullable and/or null is valid or zero is invalid. – Mehrzad Chehraz May 14 '15 at 17:23
In this case the null value is acting like a flag to indicate that there was no matching `orgName`. – Bradley Uffner May 14 '15 at 17:32
It's not clear that his objects have nullable int properties, so for category orgid you would need to do selectedOrgId.GetValueOrDefault(). This, I feel, just confuses the code more. – JasonWilczak May 14 '15 at 17:35
The braces here different than the original question. The `PD.cats.InsertOnSubmit` happens outside the foreach. For example, try running this: `foreach (var i in Enumerable.Range(1, 5)) if (true) Console.WriteLine(i); Console.WriteLine("finally here");` – David Sherret May 14 '15 at 17:35
@JasonWilczak Even if his code isn't nullable, he may make it so if he wants to indicate a value wasn't found. Not sure what the big fuss around the nullability is all about. – Yuval Itzchakov May 14 '15 at 17:37
@YuvalItzchakov [here](http://stackoverflow.com/questions/830592/why-shouldnt-i-always-use-nullable-types-in-c-sharp) is a nice discussion on the usage of nullables. The basic idea is that sometimes you can't help it (data from databases), but, when you can, used a primitive type. – JasonWilczak May 14 '15 at 17:40
1

@JasonWilczak It's definitely a nice discussion, but it is also a matter of flavor. I see nothing wrong with using a nullable in this case, although other answers which use LINQ are as good (or may be even better) than this solution. It is still a possibility. – Yuval Itzchakov May 14 '15 at 17:42
@YuvalItzchakov You are making an assumption that is not proved to be right. Actually no answer could be given to the question until OP tells us what behavior is expected when specified "Organization" is not found. – Mehrzad Chehraz May 14 '15 at 17:48
@MehrzadChehraz Yes, I am making an assumption. You seem to be really bothered by that fact, not sure why. If it isn't possible or doesn't fit the OPs needs then he can discard it, plenty of great answers provided to his problem. – Yuval Itzchakov May 14 '15 at 17:50
1

@YuvalItzchakov at least you could include the assumption in the answer. Actually everybody are making an assumption and throwing an answer. Yea OP will use the answers hopefully. I let it go :D. – Mehrzad Chehraz May 14 '15 at 17:54
I've edited my post to make clear that I'm looking for a single record – Fired from Amazon.com May 14 '15 at 18:07
THIS, TAKES THE VALUE OUT FROM THE LOOP AND ELIMINATES THE WARNING WHICH WAS THE ORIGINAL PROBLEM, SO, REMOVE THE DOWNVOTES AND MARK IT AS THE CORRECT ANSWER – The One May 14 '15 at 18:26

score 2 · Answer 4 · edited May 23 '17 at 11:58

2

Edit: OP changed the question to specify that only one item will ever be found and a multiple solution isn't needed, here is Option 1a

Option 1a - One line Linq method

This provides a single line linq query that will filter out the unnecessary orgs, grab the single item and select a new Category object to be used as the param for your insert. This method WILL throw an exception if the single item cannot be located, but that is explicitly, what should happen based on your question.

PD.cats.InsertOnSubmit(
    PD.orgs.Where(o=>o.orgname==selectedOrgName)
    .Single()
    .Select(o=>new Category { orgid = o.orgId, catname = newCatName })
);

Option 1b - Iterate a filtered list and perform work

All of the other answers here suggest using linq and assume that only one record will ever be found. Why not just do everything in the loop and use linq to filter the results?

foreach (Organization o in PD.orgs.Where(o=>o.orgname==selectedOrgName)) 
{
    PD.cats.InsertOnSubmit(new Category { orgid = o.orgId, catname = newCatName });
}

The benefit here is no if statements and it handles single or multiple cases. There is a way to make this on one line and remove the explicit for each and use List.ForEach (see the comparisons):

PD.orgs.Where(o=>o.orgname==selectedOrgName).ToList()
.ForEach(o=>PD.cats.InsertOnSubmit(new Category { orgid = o.orgId, catname = newCatName }));

Option 2 - Use an exception

This will make it very clear what your code intentions are and let Visual Studio know that you have this taken care of. The idea is to keep your code very close to the way it is now:

int selectedOrgId; 
foreach (Organization o in PD.orgs) 
{
    if (o.orgname == selectedOrgName) 
       selectedOrgId = o.orgid;
}

However, at this point I would suggest you use an exception, such as:

if(selectedOrgId == 0) throw new InvalidDataException("Selected Org Id cannot be 0");
PD.cats.InsertOnSubmit(new Category { orgid = selectedOrgId, catname = newCatName });

edited May 23 '17 at 11:58

Community

1
1

answered May 14 '15 at 17:21

JasonWilczak

2,303
2
21
37

2

But you ARE using Linq – The One May 14 '15 at 17:25
@JAT you are correct, my fingers got away from me. I edited the text to make my point more clear. – JasonWilczak May 14 '15 at 17:26
@JAT: He wasn't saying he wasn't. He was saying that, unlike other answers suggesting Linq, his accounts for more than one record found. – cHao May 14 '15 at 17:26
Like the idea. Hate the horizontal scrolling. :P – cHao May 14 '15 at 17:29
@cHao Ok, but it's clear to me that the OP is looking for a single record – The One May 14 '15 at 17:29
1

`List.ForEach` is not a LINQ method. In fact, it's a method that is very much contrary to the goals and coding style of LINQ. The "more LINQ" solution is to use `foreach`, not `ForEach`. – Servy May 14 '15 at 17:41
@Servy thank you for pointing that out, i rephrased my statement for accuracy. – JasonWilczak May 14 '15 at 17:44
I've edited my post to make clear that I'm looking for a single record – Fired from Amazon.com May 14 '15 at 18:07
@FiredfromAmazon.com Ok, I've edited my answer to include a single item only variation. – JasonWilczak May 14 '15 at 18:19

H H · Answer 5 · 2015-05-14T18:07:55.237

2

It's not the only problem. You know you will find 1 or more matches, the compiler doesn't.

But the "or more" is also a problem. The code just isn't clear about what you want, that's the root cause. You have an implicit "last one wins" strategy.

When you use a solution that matches the requirement more closely, the compiler problem goes away without any hacks.

Without Linq:

// int selectedOrgId; 

foreach (Organization o in PD.orgs)
    if (o.orgname == selectedOrgName)
    {
       PD.cats.InsertOnSubmit(new Category {
         orgid = o.orgid,
         catname = newCatName
      }); 
      return;   // or break;
    }
// shouldn't get here
throw new ...

And with Linq

Organization org =  PD.orgs.Single(o => o.orgname == selectedOrgName);
PD.cats.InsertOnSubmit(new Category {
          orgid = selectedOrgId,
          catname = newCatName
       });

The Single(Predicate) method closely matches your problem. It will throw when the resultset has != 1 element.

edited May 14 '15 at 18:07

answered May 14 '15 at 17:54

H H

263,252
30
330
514

selectedOrgId exists only inside the loop, OP wants it outside – The One May 14 '15 at 17:54
2

@JAT: there is no "outside the loop" anymore, and there shouldn't be. Read it carefully. – H H May 14 '15 at 17:59
I'm trying to understand, what's the point of having `int selectedOrgId = o.orgid;` inside the loop if you can't use it elsewhere? – The One May 14 '15 at 18:00
You could eliminate that easily but I left it to stay close to the original code. It really doesn't matter. – H H May 14 '15 at 18:02
If you're not going to use anywhere else just do: `PD.cats.InsertOnSubmit(new Category { orgid = o.orgid, catname = newCatName});` – The One May 14 '15 at 18:02
Um, I still think the point of all this, is that the OP wants to get the value out from the loop, so can use it in the method scope, but who knows. – The One May 14 '15 at 18:05
@JAT - The code is incomplete but the so far as we can see the InsertOnSubmit is the only use case. – H H May 14 '15 at 18:05
I guess my question is how would you get the value out from the loop without using Linq, nullable int? – The One May 14 '15 at 18:09
The answer is that you shouldn't want to. – H H May 14 '15 at 18:12
Looking at your experience and reputation I suppose you're right, but I don't know why. – The One May 14 '15 at 18:14
@JAT: I think you're looking for the wrong kind of 'flexible' usage. This code runs under the assumption of exactly 1 result, it should express and use that assumption. And preferably break (throw) when the assumption was wrong. – H H May 15 '15 at 13:11

David Sherret · Answer 6 · 2015-05-14T17:27:59.523

Use linq to select the org with the selectedOrgName and get the orgid:

PD.cats.InsertOnSubmit(new Category { 
    orgid = PD.orgs.First(o => o.orgname == selectedOrgName).orgid, 
    catname = newCatName
});

That assumes selectedOrgName will always be in PD.orgs and I'm making that assumption based on the variable name; however, if that isn't always the case, you can do the following:

var selectedOrg = PD.orgs.FirstOrDefault(o => o.orgname == selectedOrgName);

if (selectedOrg != null)
{
    PD.cats.InsertOnSubmit(new Category { 
        orgid = selectedOrg.orgid, 
        catname = newCatName
    });
}

Robert McKee · Answer 7 · 2015-05-14T18:29:03.500

1

The following is the exact equivalent of what your code does, but it will throw an exception if you ever don't have an org that matches (which you say can't happen):

PD.cats.InsertOnSubmit(new Category { 
    orgid = PD.orgs.Last(o => o.orgname == selectedOrgName).orgid, 
    catname = newCatName
});

edited May 14 '15 at 18:29

answered May 14 '15 at 18:23

Robert McKee

21,305
1
43
57

How can I modify this C# code so that Visual Studio recognizes that I'm not an idiot?

7 Answers7