How can I get rid of some of these nested case structures?

[Shiv0921]

My system is doing various things and one is acquiring data, including water level. The system will slowly empty its own water and I need to keep it topped up by opening a valve when it gets too low. But if it’s too low or too high I don’t want to bother with the valve, I just want everything to shut down. I seem to have ended up with a 5 deep nested case structure and it seems like a bit of a mess. I've thought about using a producer consumer set up but I wasn't sure that actually made things any more elegant. Can anyone advise on what might be a better way?

The process is:

If the system is acquiring, check the data

Check this data is between 10 and 30

if not, stop the program and exit.

If the data is between those major levels and the valve is closed (no water filling) check that the level is above 15

If not, open the valve to increase the level.

If the data is between those major levels and the valve is open (water is refilling) check the level is below 20.

If it exceeds 20, close the valve as the level is now good.

Few things.

• Time is not critical. If the system takes several seconds to realise the level is too low/high that’s fine. (Though I still don't want unnecessarily slow code as I'm trying to learn best practices)  
• Lossy data checking is fine, again because it doesn’t need to react quickly.
• I think the notifier might need to be replaced with a queue anyway, so the second load of case structures can access the data? Not sure on that, but I can work on that one.

Any input would be much appreciated, I’d like to learn to write good clean code, rather than just stuff that works, but I don’t work in software so am still very much learning

.

[GerdW]

Hi Shiv,

---

@Shiv0921 wrote:

Any input would be much appreciated, I’d like to learn to write good clean code, rather than just stuff that works, but I don’t work in software so am still very much learning

---

You didn't ask specifically for this, but you may start by removing those FOR loops in your image. Use polymorphism instead!

The left FOR loop might be replaced by an InRangeAndCoerce function…

[altenbach]

Sorry, I don't have your toolkits and cannot really inspect the code.

Another odd construct is to base the wait on "2 seconds + an error code". Error codes are I32 and can be gigantic (positive and negative). Does not seem reasonable code. There is a chance that the code will stall almost forever if an error happens to occur.

[Shiv0921]

---

The left FOR loop might be replaced by an InRangeAndCoerce function…

---

Thanks, that's a great point! No problem that it's not what I asked about, any feedback is really appreciated

[Shiv0921]

---

@altenbach wrote:

Sorry, I don't have your toolkits and cannot really inspect the code.

Another odd construct is to base the wait on "2 seconds + an error code". Error codes are I32 and can be gigantic (positive and negative). Does not seem reasonable code. There is a chance that the code will stall almost forever if an error happens to occur.

---

Ah yeah I thought that might be a bit of a weird one, it was on my list to double check. I didn't realise error codes could be enormous (or negative), thank you for informing me! I just wanted something that would force the execution of the wait to be after the message had sent, as it's important the state gets a chance to change before it runs again.

[altenbach]

Just place the wait after the inner case, with a 2000 wired from one case and "use default if unwired" for the other case. In fact the wait could probably go all the way to the diagram of the much big case structure.

[Shiv0921]

Thank you altenbach, I appreciate that. In a similar vein, is there a way to be sure that the 'open/closed' state here is updated before the 'check level/exit' message is sent? I almost feel like this is where a flat sequence structure might come in, but I'm aware they're normally a cheap get-out and to be avoided.

(I'm trying to make sure that the valve is always closed before the system can exit, just in case the user asks to exit while the water is still filling.)

I realise this is perhaps very straightforward, it has been some months since I worked with labview/did any courses, and I now realise it may be time to go back over the courses. 

[Mark_Yedinak]

I'll start by saying that I haven't looked at your code. But I suspect that you are doing too many unrelated things in your actions. The actions of teh state machine should be very minimal. For example, in the picture you posted you are aquiring the data and chekcing limits within the same state. That makes it much more dificult to alter your flow of execution since a sigle action is performing many different tasks. By breaking your actions into more discreet  By doing so it is easier for you to control what gets executed. For example, I would have an action called "Acquire Data" and another action called "Check Limits". Other actions would include "Open Valve" and "Close Valve". Depending on what state your system is in, you would queue up the necessary actions to be perform.

The reason you have so many nested case structures is that you are trying to do too much in each action. I also recommend that you separate your state control from your actions. Think of two case structures next to each other. These case structures are wired together so each will execute. The left case structure wold be your discreet actions/tasks. The right case structure manages the state and queues up the actions to be performed. Norm gave a great presentation on this type of architecture. He calls it the TLB framework. An approach like this gives you much better control over your operations and makes it easier to adjust what actions are performed and when. It also makes it easier for you to reuse those actions since they perfomr a single task/operation.

[dsb@NI]

It may also be worthwhile to review this older post: Alternate to Nested case structures ?

[Shiv0921]

Thanks Doug, I had looked at that before but initially didn't see how I could relate it to my case. Now with some further effort I think I understand and have now incorporated that, which got rid of 1 of the case structures at least! 

I could have used it to remove the acquisition on/off case too, but I didn't want the vi perma running in the background attempting to check all the other booleans when acquisition is off anyway.