From 04:00 PM CDT – 08:00 PM CDT (09:00 PM UTC – 01:00 AM UTC) Tuesday, April 16, ni.com will undergo system upgrades that may result in temporary service interruption.
We appreciate your patience as we improve our online experience.
LabVIEW
How's My Coding?
Solved!
Hey all, I'm currently trying to get better with LabVIEW and was reciently given the following task to test my skills:
1. Have a button on the screen that when clicked on will generate a random number between 1 and 100.
2. If the number is odd, display a message on the screen that lists the number and the string “Odd Number” next to it.
3. If the number is even, log the number to a text file by saving the number and the string “Even Number” next to it.
4. In either the odd or even case, if the user clicks the button to generate another random number, append the new number and string to the previous.
Currently I believe my code does what's needed and I think I've used all the best practicies/optomizations I'm aware of. However, I was wondering if there was anything I could do to improve on the code or anything I missed.
Wow it actually looks pretty decent and clean. Good work.
All the suggestions I have are pretty minor but since you are asking for suggestions here are mine, some are opinions and others could disagree.
- Terminals as Icons
- NI says new users like seeing large terminals as their icons, I (and others) see it as wasted space with no real benefit. I'd prefer seeing small terminals. This default can be changed to false in the Tools >> Options. Otherwise you can right click each one.
- Panel Close? Event
- This is so that if you click the red X in the upper right corner of the window, your program goes through the clean up and finishes properly. Anyone familiar with Windows will want to click that button to exit, and in professional software it should be handled. Then we can remove the Stop button since most users of Windows software don't know what that should do, but they do know closing the window should close it.
- Update History in Evaluate?
- I think history should be updated in the Evaluate state, so that the value it generated just now is updated, instead of having to come back around.
- Arrays Constants Show Size
- Very minor, but I prefer array constants that show 1 more item than are in them so that at a glance you can tell that the number of items are the number you expect. In your Generate Value Change you have an array constant showing 2 items, but visually I couldn't tell you how many values are in that array.
- Default if Unwired Could be used.
- Default if Unwired is not a bad feature it is just one that can be abused, and cause issues. In cases like the stop boolean in your event structure I wouldn't mind just seeing a constant of True in the cases that exit, and then a default of false in the others. We know only if a true is wired will it exit, and we know that a default will be false.
- Type Def Enums
- Basically all Enums, and Clusters you make should be type defed. This is so when you update them in one place, all the places that call it get updated. Otherwise you are updating all the constants and controls when it needs updating.
- If you are using an Enum on a case get rid of the Default
- Along the same lines of the enum adding new states. If later I added a "Init" state then that would go to the Default case which at the moment is Generate. If you remove the default, and explicity handle each value the enum allows then the VI won't be broken. But if you then add a new state like "Init" the VI will be broken, forceing the developer to handle it instead of making the new "Init" value, and then having the code not be broken which might make the developer think all is good.
- Error Handling
- There isn't a ton that can be done, but if you try to write your Even Out.txt and the file is read-only, or in a place where the user doesn't have permission to write, then an error will be generated but the user won't know about it (unless automatic error handling is on and that isn't true in an EXE). Just a simple error handler in cmall cases like this.
- Named References
- Generally a named reference isn't good. No need to name your Notifier since you should be passing around the refernce to it, and not referencing it by name.
- System Like UI
- I prefer using system controls and setting up with the system contrast levels. This makes programs look less like LabVIEW programs and more like normal programs. This also involves setting the Window Appearance.
- This VI Reference
- No need to wire a reference to a Property or Invoke Node if it is for the This VI, and no need closing it. The reference can't be closed because the VI is still open so it is like a no-op. Generally you need to close references you open.
- Error Constant
- I wouldn't wire an error constant at the start of the program. The invoke node will default to no error.
- No Need for Strip Path
- If you build a path with a path constant of ".." then it behaves like a strip path. Using this you can create the path with a single function call.
- Format Into String
- Format Into String could be used instead of some basic string stuff combining a couple functions into one.
Hooovahh wrote:
- Arrays Constants Show Size
- Very minor, but I prefer array constants that show 1 more item than are in them so that at a glance you can tell that the number of items are the number you expect. In your Generate Value Change you have an array constant showing 2 items, but visually I couldn't tell you how many values are in that array.
I have gotten in the habit of showing the Scroll Bar with my array constants and get the same effect. Also easier to see if you are near the end of the array if you have a large one.
crossrulz wrote:I have gotten in the habit of showing the Scroll Bar with my array constants and get the same effect. Also easier to see if you are near the end of the array if you have a large one.
If there were a comment about array constants I didn't say, it would be that. In this case there are only two values so showing 3 elements seems fine to me. If there are 20 elements yeah I'll make the constant show some, maybe 5 and then have a vertical scroll bars showing anyone reviewing the code that it needs extra attention.
GentlemanS wrote:Also, where did the "Discard?" boolean come from in the event structure? I've never seen that before.
Well for others that stumble on this post. The event type is a filter event which is why it has the question mark in it. This means that the event to close the panel was requested, but the discard allows us to ignore the request to close the panel and do other things instead. The "Panel Close" event (without filtering) is fired when the panel is closed, and it has already closed with no option to discard the request to close. There are a few other filter events that are useful like Key Down? or Mouse Down? which can ignore presses.
A few more random points for the originally posted code:
- Don't maximize the diagram to the screen. Annoying.
- Your notifier references don't really need to go through shift registers (unless there is a possibility that the FOR loop iterates zero times).
- Notifiers are lossy. Since you are rapidly adding two elements to it, it is possible that one gets discarded. You should use queues instead.
- If you do the "to U8" before the "+1", the math is simpler.
- The only purpose of the defaults invoke node is to clear the string. Complete overkill. Simply place the string indicator right after the left string shift register in the lower loop and it will clear right after start. Eliminate the invoke node. It is typically a good idea not to bury indicators deep inside structures for simpler code maintenance.
- To tell if an interger is odd or even, simply do a bitwise AND with 1 and wire the output to the case selector. Make one case 0, default (even) and the other 1 (odd). No need to use expensive Q&R operations.
- Since the two notifications entries always occur together, it would be sufficient to combine them into one. (eliminates FOR loop and eliminates deeply stacked structures).
- You should open the file once outside the loop, append inside the loop and close when the program stops. No need to use high-level file IO that constanty opens and closes the same file and has extra formatting overhead. Then there is also no need to built an array with one element before saving. And yes, as has been mentioned, most of your string building can be combined into a simple formatting primitive.
- I am not sure why the error case generates a 200. This sentinel value (?) is never used anywhere.
- And yes, as has been mentioned, your enum must be typedef'd.
- There should be an empty default case for the state. Having "generate" as default seems incorrect.
Sweet! I would have never thought of the bitwise and, I'm sure I'll use that in the future. A few questions though:
For this code, you combined the "Generate," and "Evaluate" cases. When we do that, should we go back to using a notifier instead of a queue structure? If not, when would one want to use a notifier?
GentlemanS wrote: If not, when would one want to use a notifier?
A Notifier is a lossy communication scheme. What this means is that a listener for the notification could miss one if the sender sends multiple before the reader can come back and check for the notification. So the general rule of thumb is that if you only care about the latest data being sent, use a Notifier. If you need to process every message, use a Queue.
