CLD Peanut Example - Code Review Request

[Tex85]

I am studying for the CLD exam using the success package prep materials. Example 16 is a peanut hopper processing plant simulation. Anywho, I'm comparing my block diagram to the example solution. I understand that many solutions can get the job done, but the example solution seems overly complicated. I'm wondering if my solution is improper or using techniques that would get dinged on the exam.

I've attached the example solution and my solution.

Mostly interested if anything in my solution jumps out as "DONT DO THAT". Such as overuse of property nodes or underuse of references.

Thanks for any help you can give 🙂

[Kyle97330]

I'm not a CLD so I never took the test (my company doesn't pay for it...), so I don't know what they would "officially" look for, but I have been using LabVIEW professionally for 10 years, so I have to hope I'm qualified.

Items of note:

• Your initial invoke of "Default vals.Reinit All" is not connected to your main loop via dataflow.  As such, it could theoretically run much later than expected and create unexpected results (like if it doesn't reset the "Stop" button to False before it reads the first time).
• There is no explicit error handling at all in your code.  It's very unlikely that the property nodes you are using would generate any errors, but relying on "Automatic error handling" is poor practice.  I would connect everything with error wires and run that to a Simple Error Handler, and set it to stop when an error happens.
• You're using a lot of "Value" property nodes that are hard-linked to a control, and don't use the error inputs/outputs for dataflow.  Local variables are recommended to be used in this case as they have less overhead.  (Obviously doesn't apply to value(signaling) nodes)
• There's a "Default" case for your state enum that can never possibly run that you can just delete
• Your event triggers are backwards.  Typically, you use "Value change" on most controls, but certain controls (knobs and sliders, mostly) it's common to use "Mouse up" so changing from 1 to 9 doesn't trigger a change on every value in between.  You have everything the other way around, where your knobs are the only ones to use "Value change".  
• You can delete the "close reference" nodes in your init case because "close reference" is a no-op on any control reference
• Your init case sets 7 values that should have just been reset to default with the earlier invoke node so they're all unnecessary
• The point of a state machine is usually to be able to change between states.  Yours goes to init and never changes, so you have a useless wire going through everything else.  You could get the same effect by just wiring up the Iteration counter and having "0" be your init case and "Default" for your event handling case.  Handling anything in the event structure is usually not recommended, especially if it takes a nontrivial amount of time, so for the cases where it requires a delay (the "Use" cases) switching to a 3rd state like the example solution does is a better way to go.

There's more that I would personally do to neaten things up (straighten some wires, make certain wires don't go under text, line things up, put the unused controls somewhere) but I don't think they grade too hard on style.

[billko]

---

@Kyle97330 wrote:

I'm not a CLD so I never took the test (my company doesn't pay for it...), so I don't know what they would "officially" look for, but I have been using LabVIEW professionally for 10 years, so I have to hope I'm qualified.

Items of note:

• Your initial invoke of "Default vals.Reinit All" is not connected to your main loop via dataflow.  As such, it could theoretically run much later than expected and create unexpected results (like if it doesn't reset the "Stop" button to False before it reads the first time).
• There is no explicit error handling at all in your code.  It's very unlikely that the property nodes you are using would generate any errors, but relying on "Automatic error handling" is poor practice.  I would connect everything with error wires and run that to a Simple Error Handler, and set it to stop when an error happens.
• You're using a lot of "Value" property nodes that are hard-linked to a control, and don't use the error inputs/outputs for dataflow.  Local variables are recommended to be used in this case as they have less overhead.  (Obviously doesn't apply to value(signaling) nodes)
• There's a "Default" case for your state enum that can never possibly run that you can just delete
• Your event triggers are backwards.  Typically, you use "Value change" on most controls, but certain controls (knobs and sliders, mostly) it's common to use "Mouse up" so changing from 1 to 9 doesn't trigger a change on every value in between.  You have everything the other way around, where your knobs are the only ones to use "Value change".  
• You can delete the "close reference" nodes in your init case because "close reference" is a no-op on any control reference
• Your init case sets 7 values that should have just been reset to default with the earlier invoke node so they're all unnecessary
• The point of a state machine is usually to be able to change between states.  Yours goes to init and never changes, so you have a useless wire going through everything else.  You could get the same effect by just wiring up the Iteration counter and having "0" be your init case and "Default" for your event handling case.  Handling anything in the event structure is usually not recommended, especially if it takes a nontrivial amount of time, so for the cases where it requires a delay (the "Use" cases) switching to a 3rd state like the example solution does is a better way to go.

There's more that I would personally do to neaten things up (straighten some wires, make certain wires don't go under text, line things up, put the unused controls somewhere) but I don't think they grade too hard on style.

---

Style, Functionality, and Documentation are all considered equally IIRC.

[Tex85]

Awesome! Thanks for the thorough reply.

• Re-init to default - Data flow should be explicitly routed.
• Error handling needs implementation, definelty.
• Ref vs value property nodes vs local variables - This is where I was thrown off by their example. They use a mix of refs and value property nodes but I don't see them use local variables. I use labview daily at work for production test systems and use exclusively value property nodes (not arguing that this is correct). I've never run into issues. But I do understand they can cause issues in other environments that I do not delve into like FPGA. But I'm study to pass the test and agree with the points you made.
• Great point about value change vs mouse up.
• When to close refs is somewhat nebulous to me. In their example they close the refs. I typically close refs where it seems logical to do so and test the code to make sure the ref closing doesn't cause bugs. I'll study ref closing more.
• Init to default vs explicit initialize - I feel like the test may want to see explicitly setting to a default value. In my experience, someone could go into my code and accidentally do a "make current value default".
• Great point on the event structure.

Thanks again

[Kyle97330]

---

@Tex85 wrote:

• Ref vs value property nodes vs local variables - This is where I was thrown off by their example. They use a mix of refs and value property nodes but I don't see them use local variables. I use labview daily at work for production test systems and use exclusively value property nodes (not arguing that this is correct). I've never run into issues. But I do understand they can cause issues in other environments that I do not delve into like FPGA. But I'm study to pass the test and agree with the points you made.
• When to close refs is somewhat nebulous to me. In their example they close the refs. I typically close refs where it seems logical to do so and test the code to make sure the ref closing doesn't cause bugs. I'll study ref closing more.

Thanks again

---

This is a video by Darren Nattinger, somtimes seen as DNatt or Darren on the forums and other places, who is a long-time NI employee.  

https://youtu.be/pS1UBZzKl9k?t=2010

I've linked to a timestamp in the video where he talks about references to close.  A lot of what he has in the video is a matter of opinion on LabVIEW design, most of which I agree with (but not all), but the references section is objectively helpful to know about from someone who has much better information on the subject.

For the value property node vs local variable, check out this thread:

https://forums.ni.com/t5/LabVIEW/Difference-between-local-variable-and-property-node-value/td-p/4274...

That covers it fairly well as well as linking to other previous threads on the topic.