Variable initalisation in while loop

Question

I have a function that reads a file in chunks.

public static DataObject ReadNextFile(){ ...}

And dataobject looks like this:

public DataObject
{
   public string Category { get; set; }

   // And other members ...
}

What I want to do is the following basically

List<DataObject> dataObjects = new List<DataObject>();

while(ReadNextFile().Category == "category")
{
   dataObjects.Add(^^^^^ the thingy in the while);
}

I know it's probably not how it's done, because how do I access the object I've just read.

Solution 1

I think what you're looking for is:

List<DataObject> dataObjects = new List<DataObject>();

DataObject nextObject;
while((nextObject = ReadNextFile()).Category == "category")
{
   dataObjects.Add(nextObject);
}

But I wouldn't do that. I'd write:

List<DataObject> dataObject = source.ReadItems()
                                    .TakeWhile(x => x.Category == "Category")
                                    .ToList();

where ReadItems() was a method returning an IEnumerable<DataObject>, reading and yielding one item at a time. You may well want to implement it with an iterator block (yield return etc).

This is assuming you really want to stop reading as soon as you find the first object which has a different category. If you actually want to include all the matching DataObjects, change TakeWhile to Where in the above LINQ query.

(EDIT: Saeed has since deleted his objections to the answer, but I guess I might as well leave the example up...)

EDIT: Proof that this will work, as Saeed doesn't seem to believe me:

using System;
using System.Collections.Generic;

public class DataObject
{
    public string Category { get; set; }
    public int Id { get; set; }
}

class Test
{

    static int count = 0;

    static DataObject ReadNextFile()
    {
        count++;
        return new DataObject
        {
            Category = count <= 5 ? "yes" : "no",
            Id = count
        };
    }

    static void Main()
    {
        List<DataObject> dataObjects = new List<DataObject>();

        DataObject nextObject;
        while((nextObject = ReadNextFile()).Category == "yes")
        {
            dataObjects.Add(nextObject);
        }

        foreach (DataObject x in dataObjects)
        {
            Console.WriteLine("{0}: {1}", x.Id, x.Category);
        }
    }
}

Output:

1: yes
2: yes
3: yes
4: yes
5: yes

In other words, the list has retained references to the 5 distinct objects which have been returned from ReadNextFile.

Solution 2

This is subjective, but I hate this pattern (and I fully recognize that I am in the very small minority here). Here is how I do it when I need something like this.

var dataObjects = new List<DataObject>();
while(true) {
    DataObject obj = ReadNextFile();
    if(obj.Category != "category") {
        break;
    }
    dataObjects.Add(obj);
}

But these days, it is better to say

List<DataObject> dataObjects = GetItemsFromFile(path)
                                   .TakeWhile(x => x.Category == "category")
                                   .ToList();

Here, of course, GetItemsFromFile reads the items from the file pointed to by path and returns an IEnumerable<DataObject>.

Solution 3

List<DataObject> dataObjects = new List<DataObject>();
string category = "";

while((category=ReadNextFile().Category) == "category")
{
   dataObjects.Add(new DataObject{Category = category});
}

And if you have more complicated object you can do this (like jon):

List<DataObject> dataObjects = new List<DataObject>();
var category = new DataObject();

while((category=ReadNextFile()).Category == "category")
{
   dataObjects.Add(category);
}

Comments

[Jon Skeet]

@Saeed: Sorry, my previous comment apparently didn't make it - I'm on a flaky 3G network connection. This won't compile, because you're trying to add a string to a List<DataObject>. The idea isn't to remember the last category, it's to remember the last DataObject. See my answer for the fixed version.

[Saeed Amiri]

@Jon Skeet, I'd edited the answer, thanks, I didn't think about the type of list I just want to show the way OP wants.

[Jon Skeet]

@Saeed: Still -1, because while all we've been shown is the Category property, in real life I'd expect there to be more data in the object... which gets lost with your current code. As the OP says, he wants to remember the object he's just read, not reconstruct it from the one bit of information he's filtering on. It's not hard to do - look at my answer.

[Jon Skeet]

(Also bear in mind that you only left 2 minutes between editing your answer and expecting all downvotes to be removed. I happen to be watching this question, but not everyone will be...) Considering the previous version of your answer wouldn't have compiled, and even now it's not really what was asked for, I'd say that 5 upvotes and 3 downvotes is pretty generous ;)

[Saeed Amiri]

No, I got downvote after editing my answer :), and my current answer will exactly addresses current OP question.

[Jon Skeet]

@Saeed: No, it doesn't. From the question: how do I access the object I've just read. Your answer doesn't address that.

[Saeed Amiri]

@Jon skeet, you know better than the OP about his question? read first comment above, Also the idea was what I said, It's not good to get four downvote because of syntax or ... , I didn't test it and think about it, I just said the idea, others downvoted me because their answer was joke.

[Jon Skeet]

@Saeed: When the OP commented, the code didn't even compile. But I can read what the OP's question says, as I quoted - and your answer simply doesn't cope with that. If the OP really wants to return an object which only consists of a string, and then add that same exact string to the list every time, I'd be very surprised... do you really think that the real DataObject class only consists of a single property? It's interesting how you claim I can't understand the OP's question, but you're psychic enough to know reasons for downvoting an answer which doesn't work...

[Jon Skeet]

(I'd also point out that none of the other answers seem like "jokes" to me. Which ones are you thinking of, exactly?)

[Saeed Amiri]

And Also I have 4 downvoter just one of them said why? and non of them remove it.

[poindexter12]

@Saeed: Down voted this because this solution is hard to read. Also, nerd fight.

[Saeed Amiri]

@Jon skeet, I just can judge about what I can see (DataObject class), In all OP after learn this can have any variation on it, I never said you can't understand the OPs question, I saing that you can't understand what the OP wants better than him,At last non of the others answer addresses near to question.

[Saeed Amiri]

@poindexter12, first it's hard for you, second it's a valid answer, third you should downvote Jon's answer by your reason.

[poindexter12]

@Saeed: I find Jon's answer to be more readable with his alternate suggestion, but I agree without it I would have down voted as well. Also, it is hard for ME to read which is why I down voted it with my down vote.

[Jon Skeet]

@Sawed: This answer does address the question, as well as ordering an alternative (better) approach. Stop being so childish - your answer has been downvoted because it's a bad answer... get over it, and stop downvoting other answers out of spite.

[Saeed Amiri]

@poindexter12, If the answer hard for you is because you are not familiar with this syntax, for example if OP wants the answer by linq I can downvote someone who answered because i.e linq is hard for me? It's not a good reason, while the answer is valid you shouldn't downvote someone, My linq skill is good enough to answer this by linq, but the OP wants to change the value in while may be it's not a good practice but it's an answer to question. you can comment is not good practice or add your answer and say why you do it as this but It's not a reason to downvote.

[Jon Skeet]

@Saeed: I suspect the OP misunderstood what your answer was doing. I doubt that he'd have upvoted it if he'd seen that it didn't compile, for example. Now, could you explain in what way your answer addresses the OP's question of "how do I access the object I've just read"?

[Saeed Amiri]

@Jon skeet, that's your idea. and I just downvoted Jason not others. my answer is not bad, It's an answer. forgot about this. and my name is not like saw it's saeed :)

[jason]

@Saeed: What do you mean "because it's out of the question?" This is strange behavior, to be sure.

[Jon Skeet]

@Saeed: It's an answer which doesn't answer the question. To my mind, that makes it a bad answer. This answer, however - the one you've downvoted - does answer the question. Can you give any reason for downvoting it other than because you're annoyed that others have downvoted your answer?

[Jon Skeet]

@Saeed: Apologies for misspelling your name, btw - that was a typo as I was writing the comment on a phone, and it autocorrected it badly :(

[Saeed Amiri]

@Jon skeet, In first glance, I think OP want's to read Category, item which is fetched in while so it is how do I access the object I've just read, so OP got the point, you read my answer and others and then left your answer, I answered the question after my browser loads the page :)

[Jon Skeet]

@Saeed: Given that he's got a List<DataObject> rather a List<string>, that seems an unlikely reading to me. As for the point about when I wrote my answer... are you suggesting that I couldn't have come up with my answer without seeing the others first? How amusing.

[Jon Skeet]

You don't need to go to all the hassle of implementing it yourself - use iterator blocks instead. They rock :)

[poindexter12]

I agree you don't NEED to, but if that is the purpose of that object (which it looks like it is), it seems like it's a more elegant solution IMO.

[Saeed Amiri]

@jon Skeet, No you can write your answer after thinking and judging or without this, but you will get 100+ upvote for every simple answer you provide because you are Jon Skeet, and if i provide your answer after others answer may be I got downvote (or zero point) because I'm not Jon Skeet, and may be I'm a cheater, So now I should get upvote by just online answers or answering hard problems, until my rep becomes good enough, and at last, Jon it is very easy after just seeing some answers(may be wrong) saying better answer, and this is for you not us (under 10000 or 100000 reps)

[Saeed Amiri]

@Jon, @Jason, I just downvoted you because I think the linq is not an answer (is advise) and the first one does not address the OPs signature. just this, I didn't downvote @poindexter12 because his upvote is zero, and I personally do not like make someone has -1 score unless the answer is very bad (like p=np or very slow)

[jason]

@Saeed: Why is "LINQ not an answer?" What do you mean "the first one does not address the OPs signature" (sic).

[Jon Skeet]

@Saeed: If you post a correct answer, you're unlikely to get a downvote. If I post an incorrect answer, I do get downvotes - it happens sometimes, and I either delete the answer (if there's already a correct one) or just fix it.

[Jon Skeet]

@poindexter12: Creating a whole type explicitly and then using it is more elegant than just writing a method using an iterator block? I suggest you try writing a complete solution both ways and then judge again...

[poindexter12]

@Jon: If you look at what he is asking, and don't try to add anything, yeah, an iterator is the smallest, quickest solution. Based on what he was asking though, it seemed like going through a list of objects and looking at what the current item exactly mirrors the purpose of the IEnumerator, hence the suggestion.

[Jon Skeet]

@poindexter12: If you create an iterator block you can still use the generated IEnumerator<DataObject>. The calling code would be the same either way - the iterator block just saves you a lot of hassle. As I say, you should try implementing it both ways...

[Saeed Amiri]

@Jon Skeet,I talk about other thing but for this question my answer is true, but I got downvote from you and 3 other people (may be has compile error at first but fixed), non of a downvoters have a good logical reason for do not removing downvote, but they didn't. you personally have a reason?

[Saeed Amiri]

first because "(is advise)" you can solve this problem with thousand way like this linq but they aren't answer, second "while(ReadNextFile().Category == "category")" is the signature of OP code and OP said how do I access the object I've just read, And why you want explain what I think? non of your friends which are upvoted you, explain why downvoting me (and may be you). if explaining downvote reason is a rule in site is rule for all not just me.

[Jon Skeet]

@Saeed: Yes, I have the reason that I've said throughout: it doesn't answer the question, IMO. The other downvotes may have been cast before the users saw the edit, or they may just agree with my analysis of this not answering the question.

[Jon Skeet]

@Saeed: The question as written cannot be answered while keeping the while(ReadNextFile().Category == "category") part (which isn't a signature, by the way). I see you're also assuming that upvotes for this answer are due to friendship rather than just agreeing with the answer... and you're also assuming that the downvotes cast against you correspond with upvotes for this answer. They do in my case, but you shouldn't assume that's true for others.

[Saeed Amiri]

@Jon Skeet, It doesn't answer which part of the question?

[Jon Skeet]

@Saeed: The bit which I keep quoting: "how do I access the object I've just read" - i.e. the DataObject. You're only retaining the category, not the whole object that's just been read. It's a pretty simple distinction, and I've raised it several times now... I find it hard to believe that you really can't see the difference between your answer and mine, in terms of that specific part of the question.

[Saeed Amiri]

They do in my case, but you shouldn't assume that's true for others You think which is difference between you and others? and It's better to let them speak themselves.

[Saeed Amiri]

@Jon Skeet, I think you never read my last edit 13 hours ago, If you read I didn't know why downvote I just can say is out of logic, and I can't understand it.

[Jon Skeet]

@Saeed: Indeed, I hadn't seen that edit. Now the useful part of the answer: a) just replicates my answer, b) uses a misleading variable name (the category variable doesn't actually represent a category, it represents a DataObject), c) creates a DataObject for no reason. I've removed the downvote as it's no longer an actively unhelpful answer - but equally I don't think it actually adds anything useful compared to what my answer presented ages ago. Personally if this were the other way round (i.e. if I'd had to modify my answer and it ended up like someone else's) I'd just delete it.

[Jon Skeet]

@Saeed: Um, I'm not speaking for others, that's the point - whereas you've assumed motives for upvoting this answer and downvoting yours.

[Saeed Amiri]

@Jon Skeet, a) just replicates my answer if you want talk in this way, I can say you just copy my way by assigning value to variable in conditional statement. this part was the important part of my answer, And I do it before you. how to initializing, and what is good practice, bad practice, .... are not the skeleton of answer.

[Jon Skeet]

@Saeed: I don't understand what you mean - given that the important point was to assign a value to a temporary variable of the right type it's hard to see in what way I copied you. I took the OP's general structure and modified it to answer his question. And I believe that giving sensible code in terms of variable names and avoiding pointless assignments is useful in all answers. I'm just explaining why I don't think your answer deserves my upvote.

[Jon Skeet]

@Saeed: I should clarify that my point about your answer replicating mine wasn't meant in an "accusing you of cheating" sense - just that it's redundant as my answer already provides that suggestion. I frequently delete my own answers if I find that by the time I post them someone else has already posted one which is basically the same.

[Saeed Amiri]

@Jon Skeet, I think the main part of my answer is assigning value to variable in conditional statement (I can't explain it better because of my English) and it's not redundant because I do it before other answers, Yes some times I submit the answer and when page refreshed I will see similar answers exists, So I going to remove it and in my opinion it's not a case.

[Jon Skeet]

@Saeed: If you say so... I still think that assigning the wrong value to a variable made your answer effectively useless until your final edit... which was pointless by that time as my own answer had covered it by then. (And I'd still leave my answer up anyway, as I think it's better due to that part of the code being cleaner, and also the suggestion to avoid this awkward loop in the first place. LINQ is a much cleaner solution.)

[Timo Willemsen]

Heya, this really seems like the best solution for this. There is however one thing, the file I'm reading from can be exceptionally big (few megabytes). Reading all items upfront, can that be a perfomance issue?

[ToolmakerSteve]

6 years later, its still a poor answer. The first code snippet DOES NOT DO WHAT OP ASKED. Indeed, it doesn't do anything that anyone is likely to ever want to do. Which makes it a waste of time to try to make sense of this answer. The second code snippet does what is asked, but is poorly written: (1) the variable category is poorly named, as it no longer contains a category. (2) the variable category is initialized to a default DataObject. Why? It does not need to be initialized at all in this case, as it is only used in the loop. If anything, initialize it to null.