do local variables need to be nulled/disposed in methods?

Question

I've seen such block of code, and cant understand why finally block is needed in such cases. can you tell me DO LOCAL VARIABLES NEED to be nulled/disposed in methods?

private void ParseNewsXMLAndPopulateNewsList(string xmlToParse)
{
    string title = null;
    XmlDocument rawXmlDoc = new XmlDocument();

    try
    {
        rawXmlDoc.LoadXml(xmlToParse);
        foreach (XmlNode currentEvent in rawXmlDoc.SelectNodes(@"//event"))
        {
            title = string.IsNullOrEmpty(currentEvent.SelectSingleNode("title").InnerText) ? "" : currentEvent.SelectSingleNode("title").InnerText;
            _SmthGlobalObject.Add(title); 
        }

    }
    catch (Exception e)    {    throw e;    }
    finally
    {
        title = null;
        rawXmlDoc = null;
    }
}

What language is this? Java? C#? You need to tag your question with the language you're using. — jwodder, Sep 27 '17 at 17:31
I would say it's not really necessary since all those four variables are local to the method. If, on the other hand, those were declared outside, or even passed as `ref`, it might be necessary depending on how they are used. — Sach, Sep 27 '17 at 17:36
There is no benefit or even any "cleanup" done by setting these variables to `null` at the end of the method. — crashmstr, Sep 27 '17 at 17:36
who ever wrote this, has clear values by null fetish. but its not needed at all. this comment only remains true for YOUR case. sometimes setting to null is necessary. — M.kazem Akhgary, Sep 27 '17 at 17:38
@M.kazemAkhgary thanks! I wanted to know for local variables! — T.Todua, Sep 27 '17 at 17:39
Possible duplicate of [Do you need to dispose of objects and set them to null?](https://stackoverflow.com/questions/2926869/do-you-need-to-dispose-of-objects-and-set-them-to-null) — JuanR, Sep 27 '17 at 17:44
@Juan, thanks, i had already reviewed that, however, i havent seen a good method example there, that's why I opened this topic. — T.Todua, Sep 27 '17 at 18:19

score 3 · Answer 1 · answered Sep 27 '17 at 17:36

3

In code like this, you should not need to explicitly set the variables to null. If the objects implement IDisposable, you should absolutely call the Dispose method, but otherwise they can just be allowed to fall out of scope. If the code was in a long running process where they would be kept in scope for a long time and were resource-intensive I might give you other advice, but that would be the exception and not the rule.

answered Sep 27 '17 at 17:36

Kyle Burns

1,164
7
16

Even calling `Dispose` is not absolutely a must. Eventually, everything is picked up the GC and there are cases where light-weight objects implement `IDisposable` and it would simply clutter the code to use the `using` pattern all over the place. But in general, that's surely good practice. – Dejan Sep 27 '17 at 17:39
4

@Dejan The whole point of `IDisposable` *existing* is to clean up *unmanaged* resources that the GC *won't clean up on its own*. – Servy Sep 27 '17 at 17:43
1

@Dejan, IMO, if something implements `IDisposable` it is absolutely a must that you call it. if the creator of the class felt the need to add it to their class, then there *should be* a reason to call it. I think people use IDisposable all willy-nilly, and at times it isn't needed, but I'd err on the side of caution. – Cory Sep 27 '17 at 17:44
@Cory the only exception is enumerator for most collections ;-) – M.kazem Akhgary Sep 27 '17 at 17:45
@M.kazemAkhgary *Lots* of enumerators need to be disposed of as they do relevant things in the disposal, and it's pretty rare to be explicitly enumerating an enumerator when you know the exact implementation. A much better example is `Task`, that doesn't actually need to be disposed of, and isn't going to be treated as an abstraction mixed with other things that *do* need to be disposed. – Servy Sep 27 '17 at 17:48
Perhaps @M.kazemAkhgary meant `IEnumerable`, which you wouldn't need to dispose. – Cory Sep 27 '17 at 17:51
@Cory That type doesn't implement `IDisposable`, unlike `IEnumerator`, so obviously it doesn't need to be disposed as it *can't* be disposed of. Considering you were talking about not disposing of something that implements `IDisposable`, he must have indeed meant `IEnumerator`, and it is true that many implementations don't do anything in their `Dispose` method, but because many *do* need it, you need to dispose of an `IEnumerator` unless you happen to specifically know which implementation it is and that it doesn't require disposal. – Servy Sep 27 '17 at 18:10
@Servy, you are correct. This is the example I had in mind: `IEnumerable numbers = new int[2] { 0, 1 }.Select(x => x); Console.WriteLine(numbers is IDisposable);` This would print "True" at runtime. Which, it's not actually an `IEnumerable` at run-time. I was being semi-sarcastic in my response. – Cory Sep 27 '17 at 18:37
@Cory The object returned from `Select` implements both `IEnumerable` and `IEnumerator`, hence why it implements `IEnumertor`. It will actually do stuff when disposed, so you should actually dispose of it if you end up getting that object as an enumerator (which you'd do via `GetEnumerator`). – Servy Sep 27 '17 at 18:49
1

@Servy, I get it. No more sarcasm for you. – Cory Sep 27 '17 at 19:00
IDisposable is a weird contract. Most contracts communicate capabilities that are exposed to clients, while IDisposable advertises an unexpectation that clients are expected to fill. Whether it is releasing database connections or unmanaged resource handles, you should *always* fulfill your part of the contract by ensuring Dispose is called unless there is a specific reason that you know it should not (such as on a faulted WCF channel). Don't assume that you know enough about what is going on inside the object to override the component developer's assertion that you need to call Dispose. – Kyle Burns Sep 27 '17 at 23:15
Unless you're working with Tasks. Don't try to dispose of tasks. https://blogs.msdn.microsoft.com/pfxteam/2012/03/25/do-i-need-to-dispose-of-tasks/ – Brian Sep 29 '17 at 13:12

score 3 · Answer 2 · answered Sep 27 '17 at 18:41

I've seen such block of code, and cant understand why finally block is needed in such cases.

You cannot understand it because the finally block is not needed. It should be removed.

do local variables to be nulled/disposed in methods?

Nulled? No. Disposed? Yes. A local holding a disposable that is not owned by code outside the method should be disposed as soon as possible. Normally you would use a using statement to do that.

A question you did not ask:

Is there anything else wrong with this method?

Yes. Catching and then throwing again is a worst practice because doing so mutates the stack trace of the exception. This practice makes it harder for the developer to track down the cause of the exception because the trace will be truncated at ParseNewsXMLAndPopulateNewsList, and not include the information about the actual trace of the method that threw.

If you have to catch and rethrow, the best practice is

try { whatever } 
catch (Exception ex) 
{ 
  // something here -- log the exception, say
  throw;
}

A bare "throw" means "rethrow the current exception without modification".

Anything else?

I don't like rewriting debugged, working code for no reason. But I do note that the method could be much, much shorter. You have ten statements where three would do nicely:

private void ParseNewsXMLAndPopulateNewsList(string xmlToParse)
{
  XmlDocument rawXmlDoc = new XmlDocument();
  rawXmlDoc.LoadXml(xmlToParse);
  _SmthGlobalObject.AddRange(
    rawXmlDoc
      .SelectNodes(@"//event")
      .Select(e => e.SelectSingleNode("title").InnerText));
}

That seems much clearer and more straightforward to me. What are we doing? Loading a document, extracting the events, adding their titles to a list. So do exactly that.

@T.Todua: Check the documentation: https://msdn.microsoft.com/en-us/library/system.xml.xmldocument As you can see, XmlDocument is *not* `IDisposable`. So there is no need to dispose it. — Eric Lippert, Sep 27 '17 at 18:48
btw, it would be nice if you provided some examples, when they need to be nulled, and when they dont need to be nulled . then your answer becomes a good tutorial :) p.s. throw may be problem, but mayeb it's better that you devided the below part of your answer with ____ underscores (for better readability) and the first part was dedicated to the topic directly, and in the below part of your answer, you can leave your nice recomendations! — T.Todua, Sep 27 '17 at 18:55
@T.Todua: **This is not a tutorial site; this is a question and answer site**. Variables do not need to be nulled out in C#. — Eric Lippert, Sep 27 '17 at 19:42
Eric, in "tutorial" i just meant a complete answer, with all ins and outs. — T.Todua, Sep 28 '17 at 08:59

do local variables need to be nulled/disposed in methods?

2 Answers2