Loop De Loo

[VBAHole22]

I think I have an easy one that is giving me trouble because it is friday afternoon but I gotta get this done to go home:

 

I want to make a loop that goes through items until there are no more items.

 

 

Do While Not pFeature is Nothing

 

However sometimes in the loop I realize that I don't want to work on this item becuase something is wrong with it or something. So I want to bail out of this one iteration of the loop and go to the next feature.

The features are in an enumeration so I have like so:

 

Do While Not pFeature is Nothing

    Something to do A

    Something to do B

    Something to do C

    pFeature.NextFeature 'Bump the enumeration
Loop

 

Now if something goes wrong in the 'B' area I want to quit that iteration and bump to the next feature and start the loop over at A.

Can this be done?

Old style I would go spaghetti and do Goto but not anymore. I also don't want a bunch of try catch blocks in my do loop.

 

Any suggestions?

[Malfunction]

in c# : continue;

[Joe Mamma]

Do While Not pFeature is Nothing

Something to do A

Something to do B

Something to do C

pFeature.NextFeature 'Bump the enumeration
Loop

 

Now it is way after 5, so I assume an solution has been reached.

So for the sake of code review, I ask you to clarify

 

You mention bumping the enumeration. . . more info needed here.

 

I am assuming you have a MainClass with a method called something like 'ProcessFeatures'.

I also assume you have a class called 'Feature' stored in some kind of list structure.

 

I would store the features in an ArrayList.

 

Now the question is. . . what is the interaction between MainClass and Feature while doing 'DoSomeThingX' where X is A, B, C, . . .

 

is it:

• MainClass.DoSomethingX(Feature)
  

or

• Feature.DoSomethingX(MainClass)
  

or simply

• Feature.DoSomethingX()
  

where I go from here would depend entirely on what this interaction is.

[Malfunction]

Now it is way after 5, so I assume an solution has been reached.

So for the sake of code review, I ask you to clarify

 

You mention bumping the enumeration. . . more info needed here.

 

I am assuming you have a MainClass with a method called something like 'ProcessFeatures'.

I also assume you have a class called 'Feature' stored in some kind of list structure.

 

I would store the features in an ArrayList.

 

Now the question is. . . what is the interaction between MainClass and Feature while doing 'DoSomeThingX' where X is A, B, C, . . .

 

is it:

• MainClass.DoSomethingX(Feature)
  

or

• Feature.DoSomethingX(MainClass)
  

or simply

• Feature.DoSomethingX()
  

where I go from here would depend entirely on what this interaction is.

i thought he wanted to jump out of a single iteration but not the whole loop under a certain conition. i still couldn't find vb's aequivalent to c#'s contnue which does that.

[Iceplug]

Continue is bad practice.

The proper way is to use an If block that executes the rest of the code only if the conditions are OK.

 

[color=Blue]Do While Not[/color] pFeature [color=Blue]Is Nothing[/color]
    Something [color=Blue]To Do[/color] A
    Something [color=Blue]To Do[/color] B
    [color=Blue]If[/color] Everything [color=Blue]Is[/color] OK [color=Blue]In[/color] B [color=Blue]Then[/color]
      Something [color=Blue]To Do[/color] C
      pFeature.NextFeature [color=Teal]'Bump the enumeration[/color]
    [color=Blue]End If[/color]
[color=Blue]Loop[/color]

equivalent to:

[color=Blue]Do While Not[/color] pFeature [color=Blue]Is Nothing[/color]
    Something [color=Blue]To Do[/color] A
    Something [color=Blue]To Do[/color] B
    [color=Blue]If[/color] Something [color=Blue]Is[/color] wrong [color=Blue]In[/color] B [color=Blue]Then[/color]
      Continue [color=Blue]Next[/color] Iteration [color=Teal]'continue[/color]
    [color=Blue]End If[/color]
    Something [color=Blue]To Do[/color] C
    pFeature.NextFeature [color=Teal]'Bump the enumeration[/color]
[color=Blue]Loop[/color]

:)

[VBAHole22]

I did managed to throw something together on Friday evening. Not sure it is the best but it appeared to work.

The VB.NET equivalent to the continue statement is the Goto and label.

I'm sure it is frowned upon and I would relish a better way to get it done.

The trouble with the if then and the try catch is that they only allow you to evaluate the last condition and do something then, like IcePlug did in his code. What I need to do is evaluate each condition and when one is false then I want to bump the feature and start at the entry into the loop. Here is how I got it done, more or less

 

Do While Not pFeature is Nothing

    If Not something to Do A then
       Goto Bail
    end if

    If Not something to Do B then
       Goto Bail
    end if

    If Not something to Do C then
       Goto Bail
    end if

Bail: pFeature.NextFeature
Loop

 

I could have chosen to use try catches instead of the Ifs but I have read that try catch in a loop is bad news. I could just as easily write it that way with the Goto in the catch.

 

It ain't perfect or pretty but it appeared to get the job done and it got me to the bar on Friday night with a smile. :D

[Joe Mamma]

Do While Not pFeature is Nothing

If Not something to Do A then
Goto Bail
end if

If Not something to Do B then
Goto Bail
end if

If Not something to Do C then
Goto Bail
end if

Bail: pFeature.NextFeature
Loop

is pFeature some kind of list structure with a default method that returns the current object, and NextFeature advances? similar, but not quite like IEnumerator?

[Joe Mamma]

refactored. . . no pasta

 

using System;
using System.Collections;
namespace EventNoSpaghetti
{
internal class Feature
{
 internal enum DoSomethingMethod
 {
  A, B, C
 }

 protected FeatureList _owner; 

 protected EventHandler tempHandler;

 internal int Number
 {
  get
  {
return _owner.IndexOf(this)+1;
  }
 }

 internal Feature(FeatureList owner)
 {
  _owner = owner;
  tempHandler += new EventHandler(DoSomethingA);
  tempHandler += new EventHandler(DoSomethingB);
  tempHandler += new EventHandler(DoSomethingC);
 }

 protected void DoSomething(DoSomethingMethod caller)
 {
  if (OnProcess == null) return;

  Random rdm = new
  Random(unchecked((int)DateTime.Now.Ticks));
  bool result = rdm.Next(2) == 1;
  System.Windows.Forms.MessageBox.Show(
string.Format("Feature #{0} -> {1}.{2}: {3}", 
  Number,
  caller.GetType().Name,
  caller.ToString(), 
  result ? "Success!" : "Failure!"));
  if (!result)
OnProcess = (EventHandler)EventHandler.Combine(null,null);
 }

 protected void DoSomethingA(object sender, EventArgs e)
 {
  DoSomething(DoSomethingMethod.A);
 }

 protected void DoSomethingB(object sender, EventArgs e)
 {
  DoSomething(DoSomethingMethod.B);
 }

 protected void DoSomethingC(object sender, EventArgs e)
 {
  DoSomething(DoSomethingMethod.C);
 }

 protected EventHandler OnProcess;

 internal void Process()
 {
  OnProcess = (EventHandler) EventHandler.Combine(null,tempHandler);
  if (OnProcess != null)
OnProcess(this, EventArgs.Empty);
 }
}

public class FeatureList:ArrayList
{
 public int AddFeature()
 {
  return Add(new Feature(this));
 }

 public void Process()

 {
  IEnumerator features = this.GetEnumerator();
  features.Reset();
  while (features.MoveNext())
((Feature) (features.Current)).Process();
 }
}

}

EventNoSpaghetti.zip

[VBAHole22]

Wow. It's going to take me a few days to digest that code. There is a lot to it Joe.

 

Yes the pFeature from a third party COM dll (ArcObjects form ESRI). The do Until is Nothing is the way they recommend it be worked with. I don't know the technical term for it but I believe it is a collection or an enumeration. You get at it via an interface.

 

Thanks for the low-carb, OOP suggestions. Let me chew on it and see if I can get it to fly.

[Joe Mamma]

Yes the pFeature from a third party COM dll (ArcObjects form ESRI). The do Until is Nothing is the way they recommend it be worked with. I don't know the technical term for it but I believe it is a collection or an enumeration. You get at it via an interface.

Then the above wont work. . .

 

The code I suggested assumed a DoSomething was a method of pFeature but it appears you want something along the lines of

 

MainClass.DoSomething(pFeature)

 

what esri interface is it?

[Joe Mamma]

This should probably go into a new thread/category as we will start talking about something different than looping. Perhaps a new section on refactoring is entailed.

 

Did some looking at this and your situation presents some of .COM's flaws that spurred the development of .NET. How nice it would be to actaully inherit from ESRI's FeatureList and IFeature Implementations. But as it stands, you can not inherit from com objects - only COM interfaces are implementable. this means you would have to implement the IFeature and present, or wrap, an instance of their COM object. Overwhelming for thier objects implementing IFeature but not necessarily for their IFeatureList implementations.

 

I guess thats what .NET interoping will do for you but I will address the COM interop issues at a later post, as this interests me. But for right now, your question was about refactoring, an interest you previously expressed, and one about which I enjoy rambling. Therefore, I will address my motivations for this refactoring.

 

If you refer back to my code, the technique I am employing is dynamic delegation. That is, based on the results of a sub process, the process set changes. This is something that was not available in VB6 - probably its biggest draw back. Also, one of the many features that made Delphi far superior.

 

So when does a process warrant a dynamic delegation refactoring? This is actually two questions. When does it warrant refactoring? If refactoring is appropriate, should dynamic delegation be employed?

 

The first one is the hardest to answer definitively as the problem with refactoring is that the resulting benefits are hard to immediately quantify. Most of us have had bosses who say, 'if it ain't broke, don't fix it.' And they want our development efforts to add features and functionality, which they can sell. Refactoring, by definition, produces niether. It may improve performance, scalability and extensibity. But in most cases, the performance increase does not justify the time spent refactoring.

 

Well, there are books written on this very subject so, I will assume your boss recognizes your skills, appreciates your insight and values well designed code, therefore we will go on the premise he has given you a green light.

 

So what refactorings should be applied? Well, the first smell is that there is set of sub processes that are to be executed. The first refactoring would be to get them all to be methods of the same object:

 

Feature.DoSomethingA, Feature.DoSomethingB, Feature.DoSomethingC

 

Next would be to refactor them so they all had the same signature. If they have diferent signatures, this might entail refactoring their argument lists into a third class-

Feature.DoSomethingA(ArgClass),

Feature.DoSomethingB(ArgClass),

Feature.DoSomethingC(ArgClass).

 

next would be to enumerate out the function call -

Feature.DoSomething(Enum.A, ArgClass),

Feature.DoSomething(Enum.B, ArgClass),

Feature.DoSomething(Enum.C, ArgClass).

 

we could leave it at that but, you see that you have spaghetti conditional execution, this implies applying a dynamic delegation.

 

Lets refer to my code:

 

First I enumerated the sub processes - DoSomethingMethod

 

Next, I declared an enmerated process, DoSomething(DoSomethingMethod) with behavior that is dependent on the state of DoSomethingMethod. My behavior is just a difference in the dialog that is shown. In practice you would select/case the enumeration to perform the related action.

 

Then, I declared a set of Event Handler methods, DoSomethingX, one for each of the DoSomethingMethod enumerations.

 

In my class delaration, I defined two event handlers, tempHandler and OnProcess. On construction, I added all the DoSomethingX methods, this will be used as my baseline process set. In Process, I copy the baseline process set to the OnProcess handler. During, DoSomething(DoSomethingMethod), I reset the process set if the call fails, inhibiting calling of any more sub processes . . .

 

protected void DoSomething(DoSomethingMethod caller)
{
// if a previous sub process failed, exit immediately
if (OnProcess == null) return;

// Do a conditiona process based uppon state of caller

// if any call fails, clear the process set
if (!result)
OnProcess = (EventHandler)EventHandler.Combine(null,null);
}

 

the benefit here is that this is highly extensible. to add a new sub processes, we change DoSomethingMethod:

internal enum DoSomethingMethod
{
A, B, C, D
}

add a new DoSomethingX method:

protected void DoSomethingD(object sender, EventArgs e)
{
DoSomething(DoSomethingMethod.D);
}

extend our process set:

internal Feature(FeatureList owner)
{
_owner = owner;
tempHandler += new EventHandler(DoSomethingA);
tempHandler += new EventHandler(DoSomethingB);
tempHandler += new EventHandler(DoSomethingC);
tempHandler += new EventHandler(DoSomethingD);
}

And in DoSomething(DoSomethingMethod caller) just add a new response for case DoSomethingMethod.D.

 

In all. . . two copy/pastes, four characters typed, and two lines of code in the DoSomething(DoSomethingMethod caller) method. Nothing changes in Feature.Process!!!

 

Does that make sense?

Edited August 2, 2004 by Joe Mamma

[Joe Mamma]

I am working on making an IEnumerable wrapper for the esri IFeatureCursor, do you call NextFeature after initializing a cursor to get the first IFeature?

 

ala

IFeature myFeature = myCursor.NextFeature();
while (myFeature != null)
{
 DoSomething(myFeature);
 myFeature = myCursor.NextFeature();
}

[VBAHole22]

Correct. A cursor is used on the Feature Class. It all stems from a feature dataset.

 

dim pDS as IFeatureDataset
Dim pFeature as IFeature
Dim pFCC as IFeatureClassContainer
Dim pEnumFc As IEnumFeatureClass
Dim pInFeatureClass As IFeatureClass
Dim PSearchFeatureCursor as IFeatureCursor

pFCC=CType(pDS, IFeatureClassContainer)
pEnumFc=pFCC.Classes
PInFeatureClass = pEnumFC.Next
pSearchFeatureCursor = pInFeatureClass.Search(Nothing, False)
pFeature = pSearchFeatureCursor.NextFeature

Do While Not pFeature is Nothing

pFeature = pSearchFeatureCursor.NextFeature
Loop

 

Thank you for the insight. I am going to be taking this procedural code and turn it into a class at some point. My biggest obstacle right now is that this code is in a web service and i am having difficulty understanding how objects can be passed to a web service. But that is for another thread.

[Joe Mamma]

Totally refactored - Zero Carb Diet

 

boy am I a geek. . . :D

 

There was a little spaghetti in my previous specifically

if (OnProcess = null) return

and all that combining of EventHandlers was a little too much overhead.

 

I replaced the event handler with an ArrayList of delegates, SubProcesses, if DoSomething fails, SubProcesses is cleared.

 

With a little creativity, the wrapper and the Adapter can be coereced into easily interfacing all the ESRI IFeature/IFeatureCursor implementations by attaching a method that calls a specific FeatureList initializer to the FeatureCursorAdapter's on reset event.

 

I used DummyFeature and DummyFeatureCursor to stand in for actual ESRI instances. These classes implement an extremely limited version of their respective ESRI interfaces.

 

Damn I want to get back into GIS. Got any positions there???

 

BTW, visio's reverse engineering sux!!!

I really want to see a UML for this.

 

using System;
using System.Collections;
namespace EventNoSpaghetti
{
// these interfaces are actually defined by esri
public interface  IFeature
{
 // I only need the OID property defined 
 int OID {get;}
}
public interface  IFeatureCursor
{
 // I will only declare the method I need,
 // Your esri interface has more
  IFeature NextFeature();
}
internal class FeatureWrapper
{
 delegate void DoSomethingDelegate();
 internal enum DoSomethingMethod
 {
  A, B, C
 }
 ArrayList SubProcesses = new ArrayList();
 protected FeatureCursorAdapter _owner; 
 protected IFeature _interop;
 
 protected void LoadSubProcesses()
 {
  SubProcesses.Add(new DoSomethingDelegate(DoSomethingA));
  SubProcesses.Add(new DoSomethingDelegate(DoSomethingB));
  SubProcesses.Add(new DoSomethingDelegate(DoSomethingC));
 }
 internal FeatureWrapper(FeatureCursorAdapter owner, IFeature interop)
 {
  _owner = owner;
  _interop = interop; 
 }
 protected void DoSomething(DoSomethingMethod caller)
 {
 
  Random rdm = new
Random(unchecked((int)DateTime.Now.Ticks));
  bool result = rdm.Next(2) == 1;
  System.Windows.Forms.MessageBox.Show(
string.Format("Feature Process \"{0}\" -> {1}.{2}: {3}",
_interop.OID,
caller.GetType().Name,
caller.ToString(), 
result ? "Success!" : "Failure!"));
 
  if (!result)
SubProcesses.Clear();
 }
 protected  void DoSomethingA()
 {
  DoSomething(DoSomethingMethod.A);
 }
 protected  void DoSomethingB()
 {
  DoSomething(DoSomethingMethod.B);
 }
 protected void DoSomethingC()
 {
  DoSomething(DoSomethingMethod.C);
 }
 internal void Process()
 {
  LoadSubProcesses();
  for (int i = 0; i  < SubProcesses.Count;  i++)
((DoSomethingDelegate)SubProcesses[i])();
 }
 internal IFeature Feature
 {
  get { return _interop;}
 }
}
public class FeatureCursorAdapter:IEnumerable
{
 IFeatureCursor _featureCursor;
 internal bool isNew = false;
 
 public FeatureCursorAdapter(IFeatureCursor featureCursor)
 {
  _featureCursor = featureCursor; 
 }

 public void Process()
 {
  IEnumerator features = this.GetEnumerator();
  features.Reset();
  while (features.MoveNext())
((FeatureWrapper) (features.Current)).Process();
 }
 internal IFeatureCursor Cursor
 {
  get { return _featureCursor; }
 }
 public EventHandler OnReset; 
 public virtual void Reset()
 {
  if (OnReset != null) 
OnReset(this, EventArgs.Empty);
  else
throw new Exception("FeatureWrapper must be reinitialized");
 }
 public IEnumerator GetEnumerator()
 {
  return new FeatureCursorAdapterEnumerator(this);
 }
}
public class FeatureCursorAdapterEnumerator:IEnumerator
{
 FeatureCursorAdapter _list;
 FeatureWrapper _current;
 internal FeatureCursorAdapterEnumerator(FeatureCursorAdapter list)
 {
  _list = list;
  _list.Reset();
 }
 public object Current
 {
  get 
  {
if (_current!= null) 
 return _current;
else
 throw new Exception("FeatureWrapper must be reinitialized");
  }
 }
 public bool MoveNext()
 {
  IFeature _feature = _list.Cursor.NextFeature();
  if (_feature == null) 
  {
_current = null;
return false;
  }
  else 
  {
_current = new FeatureWrapper(_list, _feature);
return true;
  }
 }
 public void Reset()
 {
  _current = null;
  _list.Reset();
 }
}
}

EventNoSpaghetti.zip

[VBAHole22]

Thanks. I'll digest this.

No positions at the moment, but we would sure like to have you on the team. This project I am on now is going to me a template for future customers so the OOP part is so important. But like they always say, bosses want results and mentioning OOP gives some folks the chills. They just see more lines of code that don't 'do' anything.

 

Are you using Visio2003 or a newer version? I heard it got better.

Try Posieden.

[Nerseus]

<Off topic a bit>

If a boss thinks OOP is just code-bloat then they need to be informed (if you're going to attempt it, make sure you know what you're talking about first!). There are more and more books coming out that explain OOP a LOT better than most universities teach. Meaning, it's more than just objects that encapsulate stuff. Sure, that's what OOP is but until you've seen a *good* OOP design, it's hard to showoff what it does.

 

From my experience, OO code (when done right, which is VERY hard to teach) is MUCH easier to maintain and enhance. I've tried adding features to non-OO code and the first 2 or 3 enhancements come "easy". After that, they get harder and harder and I generally end up breaking stuff while trying to fix or add something else. With OO code that rarely happens - ie, a good thing :)

 

-ner

[Joe Mamma]

this 'smell' kept me awake last night. . .

 

the line:

 

throw new Exception("FeatureWrapper must be reinitialized");

 

is duplicated in

FeatureCursorAdapter.Reset()

 

and

 

FeatureCursorAdapterEnumerator.Current

My gut told me this needs more refactoring.

I have a really neat idea I will implement tonight. . .

[samsmithnz]

<Off topic a bit>

 

From my experience, OO code (when done right, which is VERY hard to teach) is MUCH easier to maintain and enhance. I've tried adding features to non-OO code and the first 2 or 3 enhancements come "easy". After that, they get harder and harder and I generally end up breaking stuff while trying to fix or add something else. With OO code that rarely happens - ie, a good thing :)

 

-ner

 

You're right, generally OO code is better. But recent studies sugguest that in many situations people OVER-engineer their code now... Everything and anything in moderation is the key, even Gotos are ok, as long as you use them in Moderation...

[Nerseus]

I don't know if Goto's are ever acceptible :p

 

In all honesty, a Goto in Refactoring terms would be a smell. It points that you are skipping around some code - maybe a method to check first would get you out, or the code your skipping needs to be in a method so that you don't mind skipping over it. Generally, a Goto is used as a convenience to skip over a LOT of code or to get out of a bunch of nested code. In either case (lots of code or big nests), the code is probably somewhat unreadable and a good dose of refactoring would make the goto unnecessary.

 

But, a bad smell doesn't mean anything needs to change. If it works and is easy to understand then keep it - even a Goto.

 

-ner

[JRichmond]

It seems that pretty soon you'll be able to use "Continue":

 

http://blogs.vbcity.com/mcintyre/archive/2004/08/03/151.aspx