Tech Off Thread

41 posts

is 'new object().property' or 'new object().method()' a good idea?

Back to Forum: Tech Off
  • User profile image
    ScanIAm

    There is a completely different thread about a slightly tangential topic, but I'm noticing a number of pieces of example code that concerns me.

    I've done this myself, but how 'safe' is it:

    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.

    The other side of the coin, however, is that you could end up with twice as many lines of code if you check every single variable before using it.

    What do you do?

  • User profile image
    wkempf

    ScanIAm wrote:
    

    There is a completely different thread about a slightly tangential topic, but I'm noticing a number of pieces of example code that concerns me.

    I've done this myself, but how 'safe' is it:

    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.

    The other side of the coin, however, is that you could end up with twice as many lines of code if you check every single variable before using it.

    What do you do?



    First, your example is bad:

    Console.WriteLine("Bob".Length);

    However, the idea is there, so I'll address it.  The new String will either be created, thus no problem, or an exception will be thrown and so Length won't be called, so also no problem.  The only problems that exist here exist whether or not you "chain" the calls to the constructor.

    Check the variable before using it?  Like I said, either it worked, and so there's no reason to check, or it threw an exception, so there's no reason to check.  I guess I don't get what you think the problem is?

  • User profile image
    evildictait​or

    ScanIAm wrote:
    

    There is a completely different thread about a slightly tangential topic, but I'm noticing a number of pieces of example code that concerns me.

    I've done this myself, but how 'safe' is it:

    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.

    The other side of the coin, however, is that you could end up with twice as many lines of code if you check every single variable before using it.

    What do you do?



    In an unsafe language like C that is absolutely correct:

    #include <string.h>

    int main(){
      int len;
      string b;

      b = (string)malloc(sizeof(char) * 3);
      strcpy(b, "Bob\0", 3);
      len = strlen(b);
    }


    Should have checks in it:


    int main(){
      int len;
      string b;

      b = (string)malloc(sizeof(char) * 3);
      assert(b != NULL);
      strcpy(b, "Bob\0", 3);
      len = strlen(b);
    }

    In case you run your system down to the point where it can't allocate 3 bytes for you (heaven forbid).

    In a language like C#, however, new is a memory safe operation, since if the internal malloc fails a new OutOfMemoryException() is thrown at the il line where the newobj instruction is.

    There is no T such that
    new T( args ...) === null

    however, the line new T(args ... ) can throw an OutOfMemoryException if there is not enough system memory to allocate T on the stack.

  • User profile image
    odujosh

    I think the whole topic is splitting hairs in managed code. C or C++ I can see worrying. When you hard coding the string like you suggest I see no problem not testing for IsEmptyOrNull as you will always get the same result. In random input scenarios like reading in from file, form etc I would always test for invalid input before you commit it to anything non trivial.

  • User profile image
    Curt Nichols

    ScanIAm wrote:
    
    I've done this myself, but how 'safe' is it:

    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.


    You absolutely know that the new String has been created before Length is called. An exception will be thrown if the String could not be created.

    [Edge case ahead.]

    Now, in the case of

        new MyObject().LongRunningMethod();

    the instance of MyObject may be garbage collected before LongRunningMethod returns. This usually doesn't cause a problem, but it can; to cause a problem usually involves a questionable design decision (e.g., a disposed MyObject cleans up a field, and this happens to occur after LongRunningMethod's last reference to your MyObject instance but before using a locally stored reference to that field). I can make it happen but I don't expect to see it in the wild.

  • User profile image
    evildictait​or

    Curt Nichols wrote:
    
    ScanIAm wrote:
    
    I've done this myself, but how 'safe' is it:

    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.


    You absolutely know that the new String has been created before Length is called. An exception will be thrown if the String could not be created.

    [Edge case ahead.]

    Now, in the case of

        new MyObject().LongRunningMethod();

    the instance of MyObject may be garbage collected before LongRunningMethod returns. This usually doesn't cause a problem, but it can; to cause a problem usually involves a questionable design decision (e.g., a disposed MyObject cleans up a field, and this happens to occur after LongRunningMethod's last reference to your MyObject instance but before using a locally stored reference to that field). I can make it happen but I don't expect to see it in the wild.


    This is interesting, and would be a bug in the CLR.

    You won't be able to test for it either since

    1:  MyObject o = new MyObject();
    2:  if(null == o)
    3:    throw new Exception();
    4:  var result = o.LongRunningMethod();

    If what you say is correct then lines 2 and 3 are superflouous since o could be garbage collected between 2,3 and 4.

    To protect against it you could do

    MyObject o = new MyObject();
    var result = o.LongRunningMethod();
    GC.KeepAlive(o);

    But in fairness this is a bug that should be fixed by Microsoft rather than coded around, and you should write an bugreport if you find in your code that this is happening.

  • User profile image
    ScanIAm

    wkempf wrote:
    
    ScanIAm wrote:
    

    There is a completely different thread about a slightly tangential topic, but I'm noticing a number of pieces of example code that concerns me.

    I've done this myself, but how 'safe' is it:

    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.

    The other side of the coin, however, is that you could end up with twice as many lines of code if you check every single variable before using it.

    What do you do?



    First, your example is bad:

    Console.WriteLine("Bob".Length);

    However, the idea is there, so I'll address it.  The new String will either be created, thus no problem, or an exception will be thrown and so Length won't be called, so also no problem.  The only problems that exist here exist whether or not you "chain" the calls to the constructor.

    Check the variable before using it?  Like I said, either it worked, and so there's no reason to check, or it threw an exception, so there's no reason to check.  I guess I don't get what you think the problem is?


    Yep, that's what I get for not actually compiling the code and testing it, but it was meant as an example.

    I've seen this used, more often, when adding ListItem to a radiobutton/combobox/dropdownlist. 

    I guess, for me, the fear of using this construct comes from C/C++ training.

  • User profile image
    AndyC

    evildictaitor wrote:
    

    Should have checks in it:


    int main(){
      int len;
      string b;

      b = (string)malloc(sizeof(char) * 3);
      assert(b != NULL);
      strcpy(b, "Bob\0", 3);
      len = strlen(b);
    }



    assert really doesn't count as a check, since it is only functional during debugging sessions. For runtime checks you really should use a proper if else construct.

  • User profile image
    evildictait​or

    AndyC wrote:
    
    evildictaitor wrote:
    

    Should have checks in it:


    int main(){
      int len;
      string b;

      b = (string)malloc(sizeof(char) * 3);
      assert(b != NULL);
      strcpy(b, "Bob\0", 3);
      len = strlen(b);
    }



    assert really doesn't count as a check, since it is only functional during debugging sessions. For runtime checks you really should use a proper if else construct.


    Well if we're being fussy we can go into propagating error results or implementing the C Exception library, but I was just trying to get my point across, not write release code Tongue Out

  • User profile image
    littleguru

    Curt Nichols wrote:
    
    ScanIAm wrote:
    
    I've done this myself, but how 'safe' is it:

    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.


    You absolutely know that the new String has been created before Length is called. An exception will be thrown if the String could not be created.

    [Edge case ahead.]

    Now, in the case of

        new MyObject().LongRunningMethod();

    the instance of MyObject may be garbage collected before LongRunningMethod returns. This usually doesn't cause a problem, but it can; to cause a problem usually involves a questionable design decision (e.g., a disposed MyObject cleans up a field, and this happens to occur after LongRunningMethod's last reference to your MyObject instance but before using a locally stored reference to that field). I can make it happen but I don't expect to see it in the wild.


    The MyObject instance won't be collected by the GC if you are accessing any fields that held by that instance... otherwise - as said - it would be a heavy CLR (GC) bug!

  • User profile image
    Sven Groot

    ScanIAm wrote:
    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.

    Ignoring that String is a bad example because you can do "Bob".Length (as was mentioned), I still don't see what you're getting at.

    What is it you want to check? Any error conditions that cause the object not to be created would throw an exception.

    Code like what evildictaitor wrote:
    MyObject o = new MyObject();
    if( o == null )
       throw new Exception();

    That's completely pointless. O can never be null after a "new", since if the object couldn't be created an exception would be thrown (e.g. an OutOfMemoryException).

  • User profile image
    staceyw

    I ~think what your asking is if that var gets collected correctly as it is an internal assignment.  That is interesting as I recall a c# ng thread circa 2.0 that someone showed that is was never collected even after some GCs.  I personally did not test or repro the issue, but IIRC, others could repo the issue.  Me thinks it was fixed, but not sure.  Save that issue, I don't see a problem other then it would be easier to debug if ctor and assignment was on a line by itself and maybe more clear.

  • User profile image
    DinoViehland

    littleguru wrote:
    

    The MyObject instance won't be collected by the GC if you are accessing any fields that held by that instance... otherwise - as said - it would be a heavy CLR (GC) bug!


    Just to shed a little light on the situation the typical example of this is using IntPtr's and the Dispose pattern.  Such as:

    class MyDisposable : IDisposable {
        private IntPtr _foo;
        public void DoIt() {
            SomePInvokeOperation(_foo);
        }

        ~MyDisposable() {
            Dispose();
        }
        public void Dispose() {
            CloseMyHandle(_foo);
            GC.SuppressFinalization(this);
        }
    }

    Now you do: new MyDisposable().DoIt();  The IntPtr is read and right after that read there are no longer any references to the MyDisposable() instance.  Before SomePInvokeOperation can run the GC kicks in and collects the MyDisposable class.  Next the finalizer thread runs which runs the finalizer which closes the native resource.  Finally your SomePInvokeOperation runs and is operating on a _foo that is no longer valid.

    The worst case scenario here is that you can use this for a handle recycling attack which violates security.  If you're lucky the app is just going to crash.  Anyway, the way to fix this is to put a GC.KeepAlive(this); in DoIt() so that the object survives the lifetime of the call.  But it's really only a problem when you're interacting with native resources which the GC doesn't understand and keep alive its self.  So still not a bug, but a good thing to know about if you're doing any interop programming.

  • User profile image
    Pop Catalin Sever

    Curt Nichols wrote:

    ...

    [Edge case ahead.]

    Now, in the case of

        new MyObject().LongRunningMethod();

    the instance of MyObject may be garbage collected before LongRunningMethod returns. This usually doesn't cause a problem, but it can; to cause a problem usually involves a questionable design decision (e.g., a disposed MyObject cleans up a field, and this happens to occur after LongRunningMethod's last reference to your MyObject instance but before using a locally stored reference to that field). I can make it happen but I don't expect to see it in the wild.


    This is not a problem and this is why:

    1) if LongRunningMethod() acceses fields on MyObject instance the "this" parameter is passed on the stack to the method meaning you have a rooted reference to the object instance, the method call practicaly is like this

    newobj MyObject //Returns a reference in the stack
    call
    LongRunningMethod

    so when the LongRunningMethod is called there is a reference in the stack pointing at the
    MyObject instance. Also because LongRunningMethod acceses fields of the object it keeps a referente to the object instace in it's own stack frame and the object won't be garbage colected.

    2)
    LongRunningMethod() doesn't access any fields on MyObject instance so that means even if the object is garbage colected (because now the jit can optimize the method to eliminate the reference from the stack) the method will never try to dereference a null object (the this pointer) so won't get a NullReference exception.

    To test it you can even try to call with reflection a instance method that doesn't access any fields passing a null value and it will work.

    NullReference is only thown when you try to dereference a null pointer and not by calling a method (if that method doesn't try to dereference a null pointer the exception will never be thrown)

  • User profile image
    ScanIAm

    Sven Groot wrote:
    
    ScanIAm wrote:
    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.

    Ignoring that String is a bad example because you can do "Bob".Length (as was mentioned), I still don't see what you're getting at.

    What is it you want to check? Any error conditions that cause the object not to be created would throw an exception.

    Code like what evildictaitor wrote:
    MyObject o = new MyObject();
    if( o == null )
       throw new Exception();

    That's completely pointless. O can never be null after a "new", since if the object couldn't be created an exception would be thrown (e.g. an OutOfMemoryException).


    Exactly, and I'm fine with that, now, but it doesn't 'feel' right.  That's my own cross to bear Smiley

    string s = "bob";
    if(s!=null)
       Console.WriteLine(bob.Length);

    just seems safer.

  • User profile image
    stevo_

    Well, you do that..

    The concept behind most things in .NET is; the operation either worked, or it didn't (and caused an exception)..

    All these pointless null checks are just chipping away at your performance slowly.. your code will be utterly confusing for anyone else who has to take it over..

    I'd put the action to work, and work on getting over mentally, because carrying on doing that is bad practice for a .NET app..

    Wink

  • User profile image
    staceyw

    ScanIAm wrote:
    
    Sven Groot wrote:
    
    ScanIAm wrote:
    Console.WriteLine(new String("Bob").Length);

    The part that concerns me is the accessing of the function 'Length' when you don't really know that the new String has actually been created.

    Ignoring that String is a bad example because you can do "Bob".Length (as was mentioned), I still don't see what you're getting at.

    What is it you want to check? Any error conditions that cause the object not to be created would throw an exception.

    Code like what evildictaitor wrote:
    MyObject o = new MyObject();
    if( o == null )
       throw new Exception();

    That's completely pointless. O can never be null after a "new", since if the object couldn't be created an exception would be thrown (e.g. an OutOfMemoryException).


    Exactly, and I'm fine with that, now, but it doesn't 'feel' right.  That's my own cross to bear Smiley

    string s = "bob";
    if(s!=null)
       Console.WriteLine(bob.Length);

    just seems safer.



    Feeling are important.  You should do what feels right.  However "abc".length will *never be null, unless there is some very strange memory issue or hardware failure - in which case, you have bigger issues to deal with.  Happy coding Smiley

  • User profile image
    Pop Catalin Sever

    ScanIAm wrote:


    Exactly, and I'm fine with that, now, but it doesn't 'feel' right.  That's my own cross to bear Smiley

    string s = "bob";
    if(s!=null)
       Console.WriteLine(bob.Length);

    just seems safer.



    It may seem to you safer, but if you dig deeper into the innner workings of .Net (reading books, blogs, articles etc)  it won't seem safer anymore it will seem redundant and also less performant.

Comments closed

Comments have been closed since this content was published more than 30 days ago, but if you'd like to continue the conversation, please create a new thread in our Forums, or Contact Us and let us know.