Condividi tramite

What's Wrong with this code - #4

  • Articolo

Here's another one for you. This segment of code gets the list of all responses to be sent and sends them.

public void TransmitResponse(ArrayList responses,
StreamWriter streamWriter)
{
foreach (DataResponse response in responses)
{
NetworkResponse networkResponse = response.GetNetwork();
networkResponse.Send(streamWriter);
}
GC.Collect(); // clean up temporary objects
}

Three questions:

1) What's wrong with this code?
B) Why?
III) What should you do instead?

Comments

  • Anonymous
    December 06, 2004
  1. Responses is not being closed. streamWriter is not being closed, they should both be in a using block, unless you do not wish to dispose your streamWriter.

    2. I prefer to declare variables that are involved in an iteration outside the looping block so you don't have the overhead of creating the object each time.

    3. I see no reason to call GC.Collect() here.

    I would write this piece of code like this:
    NetworkResponse networkResponse;
    using(responses) {
    using(streamWriter) {
    foreach (DataResponse response in responses) {

    networkResponse = response.GetNetwork();
    networkResponse.Send(streamWriter);
    }
    }
    }
  • Anonymous
    December 06, 2004
    The comment has been removed
  • Anonymous
    December 06, 2004
    I don't see a reason to close responses or streamWriter here as they're both parameters to the method, they could be used elsewhere.

    GC.Collect is probably bad for performance.

    There is no check to see if GetNetwork retured a value and it could be null.

    No check to see if streamWriter is null or is even open.

  • Anonymous
    December 06, 2004
    You can't know if you need to call Dispose on NetworkResponse? GetNetwork might return a reference to an already existing object (although it might should be a property if that is the case).

  • Anonymous
    December 06, 2004
    The number one thing wrong is calling GC.Collect()

    Let the garbage collector do it's job.
    Monitor performance see if things are going into Gen 2, You may want to consider nulling the objects or you may want to consider reususing the objects. Unfortunately reusing in this case might be like shooting youself in the foot, because it is a stream you have no idea how big that stream is going to be each time. So reuse could be also bad.
  • Anonymous
    December 06, 2004
    The code gives the impression that is not the case. I wouldn't make that change without having access to source code or documentation.
    Its just my educated guess.
  • Anonymous
    December 06, 2004
  1. Possible to throw InvalidCastException.
    2) Because an ArrayList is being passed in that can contain references to any type.
    3) Use typed list as argument.
  • Anonymous
    December 06, 2004
    Some of you guys are calling Dispose on an ArrayList. As far as I know, ArrayList does not even implement IDisposable. You are even doing a "using" on an ArrayList? Bizarre.

    Furthermore, you should not close or dispose objects that are passed as parameters. In particular that stream is almost certainly used after your method returns.

    Looks like "whats wrong with this code" has generated even worse code than what you originally had.

  • Anonymous
    December 06, 2004
    I'm going for most of what's been said here and the fact that you failed to check whether the ArrayList responses is null before iterating over it with foreach.
  • Anonymous
    December 06, 2004
    Your are right Omer. Maybe I should change:

    1) Capability to throw multiple exceptions
    2)responses could be null, streamWriter could be null, and responses could contain references to objects other than DataResponse.
    3) Use asserts.
  • Anonymous
    December 06, 2004
    If the caller deals with any possible exceptions generated by this method, then the only glaring error is GC.Collect(). If that's the case then only (1) below applies. If we are expected to catch other errors then (1) - (4) applies.

    1) No need for GC.Collect() here. Calling GC.Collect could force a collection of the temporary NetworkResponse objects in the body of the foreach loop. A lot depends on wether Send is a synchronous or asynchronous call. If it's synchronous then there are no problems. If it is then a whole lot of errors including multiple threads trying to access the same StreamWriter could occur.

    2) The foreach loop could throw a ClassCastException. There is no guarantee that the ArrayList contains objects of type DataResponse.

    3.) networkResponse.Send(streamWriter) may generate an exception if networkResponse is null.

    4.) streamWriter may be null or closed.


    Fix could be:

    public void TransmitResponse(ArrayList responses, StreamWriter streamWriter)
    {
    if (streamWriter != null){
    foreach (object obj in responses)
    {
    DataResponse response =
    obj is DataResponse ?
    (DataResponse)obj : null;

    if (response != null){
    NetworkResponse networkResponse
    = response.GetNetWork();

    if (networkResponse != null){
    networkResponse.Send(streamWriter);
    }
    }
    }
    }
    }

    Assumption here is that networkResponse will deal with a closed streamWriter. If not then we probably want to propagate any generated exception to the caller.
  • Anonymous
    December 06, 2004
    You should check the state of streamWriter because you are assuming it is open and not null. You should use a try/finally block and write "streamWriter.Close();" in the finally block. You should not call GC.Collect(). Also, you probably shouldn't use ArrayList because ArrayList is inherently slow because the compiler can't optimize all of the virtual methods.
  • Anonymous
    December 06, 2004
    The comment has been removed
  • Anonymous
    December 06, 2004
    I beleive that you cannot pass a streamwiter reference over the network, so the call to send should really pass a string, so the 2nd parameter should be converted to a string or bytes.

    Contrary to what others are saying, you should not dispose of the writer or the responses. The method that created these objects should dispose of them. The method name suggests transmiting responses, not closing responses.

    Also, you should not necessarily dispose of the NetworkResponse object because those belong to the responses in the ArrayList. The responses are likely burdenend with the lifecycle of these objects unless their documentation says otherwise.
  • Anonymous
    December 06, 2004
    public void TransmitResponse(ArrayList responses,
    StreamWriter streamWriter)
    {
    foreach (DataResponse response in responses)
    {
    using (NetworkResponse networkResponse = response.GetNetwork())
    {
    networkResponse.Send(streamWriter);
    }
    }
    }

    I don't know what the NetworkResponse object is - does it properly reset streams? In any case forced GC is seldom a good idea, such as this case.
  • Anonymous
    December 06, 2004
    GC.Collect is even less useful than many of hte posters noted. In this case, the only object in this method can be released is the enumerator silentlt created behind the scenes for the foreach statement (assuming this is not written with v2.0. In that case, I believe the enumerator is stack based and you gain even less). GC.Collect will NOT release any of the temp response objects because they are still referenced by the ArrayList.
  • Anonymous
    December 06, 2004
    I think there isn't more to say, except that GC.Collect does not belong here:)

    I don't pay much attention to the untyped list, but I think I would use IList instead. If this is a public part of some library, then I would argue for a ResponseList or Collection. If not, then you should have the proper tests in place to secure the type of objects in the list. That goes for nulls in the list and the list itself, as well.

    Isn't the rule that every resource you open, you should also close? I can't see we are opening anything. It's difficult to know if GetNetWork returns something we are expected to close or whether it's IDisposable so we can using it.

    A question for the crowd: How should we write code to communicate that something is IDisposable and using is expected, or that a call to Close is expected?

    The StreamWriter is not used in this context, only passed on. The one who use it should check it for null, closed, etc.

    The method should be GetNetworkResponse, not GetNetWork.

    Finally: Why not just send a list of NetworkResponses?
  • Anonymous
    December 06, 2004
    Letting it sink in further.

    1) GC.Collect does not belong in this method.
    2) Because the caller thinks that all they are doing is sending data (from the name), but actually they are sending data AND doing a GC collect (side effect).
    3) Remove the GC.Collect
  • Anonymous
    December 06, 2004
    I have a question: isn't streamwriter being passed by reference? If so, then couldn't the stream theoretically change during the foreach loop? Seems a little problematic to me.

    I agree with others that any I/O methods should be closed. Any casting done from an ArrayList should be checked for null and cast tested and I never use GC.Collect (never had to..).

    Gabe
  • Anonymous
    December 06, 2004
  1. C#

    2) It is the wrong language

    3) Use java ;-)