Never heard of foreach?



  • The following code has been anonymized, but only the comments in square brackets are new ones added by me.  The rest of it is all there in the original.

     

    Dim CurrentThing As Thing
    Dim ThingIndex As Integer
    

    If anArgument Is Nothing Then
    Throw New ArgumentNullException("anArgument")
    Else

    'If instance variable is available [this is never, ever, ever null]
    If ThingCollectionArray Is Nothing Then
    Else
    
    	'If the collection contains things
    	If ThingCollectionArray.Count > 0 Then
    	
    		'Cycle through all remaining members of the collection
    		For ThingIndex = 0 To ThingCollectionArray.Count - 1
    		
    			'Get thing in collection
    			CurrentThing = DirectCast(ThingCollectionArray.Item(ThingIndex), Thing)
    			
    			'If thing is available
    			If CurrentThing Is Nothing Then
    			Else
    				
    				'[finally we do stuff here]
    


  • Considered Harmful

    Nice use of

    if( condition ) { /* do nothing */ } else { /* do something */ }

    so much simpler than

    if( ! condition ) { /* do something */ }



  • @djork said:

    If anArgument Is Nothing Then
    Throw New ArgumentNullException("anArgument")
    Else

    'If instance variable is available [this is never, ever, ever null]
    If ThingCollectionArray Is Nothing Then
    Else

    (Emphasis mine)

    Besides the obvious retardation required to not just 'not' the condition and avoid the empty true condition, it looks like you expect this person to not verify their inputs?

    Just by reading the code, and without any knowledge of the particular code in play here, I would sure hope this programmer (who may have missed class on the day of foreach) would do a sanity check on all variables they need.

    I would rather see careful coding then reckless assumptions of variable states.

    I would agree there are better ways to implement this check, but your comment doesn't seem to argue this.



  • @joe.edwards@imaginuity.com said:

    Nice use of

    if( condition ) { /* do nothing */ } else { /* do something */ }

    so much simpler than

    if( ! condition ) { /* do something */ }

    I suppose we are lucky there wasn't a temporary boolean and a switch.

    case true;

    break;

    case false;

     //do stuff

    break;

    case File_Not_Found;

    break;

    ???

     



  • @joe.edwards@imaginuity.com said:

    Nice use of

    if( condition ) { /* do nothing */ } else { /* do something */ }

    so much simpler than

    if( ! condition ) { /* do something */ }

     

    Ahh...yes, but that would be "negative coding."  I've been accused of doing too much negative coding before....



  • Maybe he was used to a language like c++ that doesn't have foreach?



  • @JeffRules said:

    @joe.edwards@imaginuity.com said:

    Nice use of

    if( condition ) { /* do nothing */ } else { /* do something */ }

    so much simpler than

    if( ! condition ) { /* do something */ }

     

    Ahh...yes, but that would be "negative coding."  I've been accused of doing too much negative coding before....

    Personally, I tend to do that a lot when I find myself overwhelmed by logic to the point where I am getting confused what the conditions do. I can't make fun of him for it!



  • @rbowes said:

    @JeffRules said:
    @joe.edwards@imaginuity.com said:

    Nice use of

    if( condition ) { /* do nothing */ } else { /* do something */ }

    so much simpler than

    if( ! condition ) { /* do something */ }

     

    Ahh...yes, but that would be "negative coding."  I've been accused of doing too much negative coding before....

    Personally, I tend to do that a lot when I find myself overwhelmed by logic to the point where I am getting confused what the conditions do. I can't make fun of him for it!

    I've had too many times reading other peoples' code and misreading

    if(foo){}
    else{
       bar
    }

    as

    if(foo)
    {
       bar
    }


    So I'd always recommend the "negative" coding.



  • @vt_mruhlin said:

    @rbowes said:

    Personally, I tend to do that a lot when I find myself overwhelmed by logic to the point where I am getting confused what the conditions do. I can't make fun of him for it!

    I've had too many times reading other peoples' code and misreading

    if(foo){}
    else{
       bar
    }

    as

    if(foo)
    {
       bar
    }


    So I'd always recommend the "negative" coding.

    I'm going to have to agree with vt_mruhlin here, and add <asuffield>if you cannot understand a logical not operator in an if statement, maybe you shouldn't be programming.</asuffield> 



  • @MasterPlanSoftware said:

    @djork said:

    	'If instance variable is available [this is never, ever, ever null]
    If ThingCollectionArray Is Nothing Then
    Else

     

    ... it looks like you expect this person to not verify their inputs?

    Just by reading the code, and without any knowledge of the particular code in play here, I would sure hope this programmer (who may have missed class on the day of foreach) would do a sanity check on all variables they need.

    I would rather see careful coding then reckless assumptions of variable states.

    I would agree there are better ways to implement this check, but your comment doesn't seem to argue this.

    If you have a read-only instance variable (not an argument to a method) like ThingCollectionArray here, that is only ever assigned to in the constructor, you are better off not checking wether it is null.  If by some fluke it turns out to be null then you have major problems in your runtime or something far worse.  It would be a truly exceptional case (beyond the typical sense of exceptions in programming) for that kind of variable to be null, and should not simply be handled or ignored at runtime.



  • @vt_mruhlin said:

    Maybe he was used to a language like c++ that doesn't have foreach?

    The famous phrase, "write FORTRAN in any language," seems applicable here.



  • @djork said:

    @MasterPlanSoftware said:

    @djork said:

    	'If instance variable is available [this is never, ever, ever null]
    If ThingCollectionArray Is Nothing Then
    Else

     

    ... it looks like you expect this person to not verify their inputs?

    Just by reading the code, and without any knowledge of the particular code in play here, I would sure hope this programmer (who may have missed class on the day of foreach) would do a sanity check on all variables they need.

    I would rather see careful coding then reckless assumptions of variable states.

    I would agree there are better ways to implement this check, but your comment doesn't seem to argue this.

    If you have a read-only instance variable (not an argument to a method) like ThingCollectionArray here, that is only ever assigned to in the constructor, you are better off not checking wether it is null.  If by some fluke it turns out to be null then you have major problems in your runtime or something far worse.  It would be a truly exceptional case (beyond the typical sense of exceptions in programming) for that kind of variable to be null, and should not simply be handled or ignored at runtime.

    Call me dense, but I see nothing in the code that hints that this variable is anything special. (I imagine you just did not include enough code for your point to make sense) If I was writing that code, and new nothing other than what you showed us, I would certainly throw a null check in there. The comment suggests this is an instance variable, but I would not be trusting the comment based on the ridiculous nature of the rest of the code.

    This would be why I love intellisense so much.  Instant access to this kind of information as you type.

     


Log in to reply