Code cleanup reccomendations

Hello, I currently have a script written to execute when each button on my nav is clicked on to logon, verify user security level, and redirect to appropriate fall back pages or page requested. It works great. But I have to copy and past this into each button. Then I must change the security levels and relevant page to open. Seems like a lot of code in a a lot of places. and makes it a pain if i need to adjust the script. I wonder how I could make better use of the code, and possibly passing in some variables to lessen the lines needed to do this.

Any ideas would be greatly appreciated, the code is pasted below:

# This script controls the badge access feature
# It allows the user to log into a page, or 
# redirects them to the invalid login, or access denied page
# it also resets and sets the timer to log out user after set time 
                                          
#Displays keyboard to scan badge or enter emergency use password
                                          
password = system.gui.showTouchscreenKeyboard("Scan Badge or Enter Emergency Password")
                                          
# opens and scans user table to cross reference password to username to provide login credentials.
system.nav.openWindow("Main/TImport")
window = system.gui.getWindow("Main/TImport")
data = window.rootContainer.getComponent("User Data").data
system.nav.closeWindow("Main/TImport")
user_name = ""

for row in range(data.rowCount):
	index_row = data.getValueAt(row, 3)
	if index_row == password:
		user_name = data.getValueAt(row, 1)
		break

# Allows successful access to page if user role has security clearance, enable and resets timer
# sends them to access denied page if user does not have valid security clearance
success = system.security.switchUser(user_name, password)
roles = system.security.getRoles()
if u'Administrator' in roles or u'LVL_2' in roles or u'LVL_4' in roles:
	system.nav.swapTo('P1 Main')
	system.tag.writeToTag("System3/BadgeSystem/LogoutTimerValue", 0)
	system.tag.writeToTag("System3/BadgeSystem/DisplayCountdown", 1)
	system.tag.writeToTag("System3/BadgeSystem/LogoutTimerRunning", 1)
else:
	system.nav.swapTo('Access Denied')
	system.tag.writeToTag("System3/BadgeSystem/LogoutTimerValue", 0)
	system.tag.writeToTag("System3/BadgeSystem/DisplayCountdown", 0)
	system.tag.writeToTag("System3/BadgeSystem/LogoutTimerRunning", 0)
    
# Sends user to failed login page if badge scan does not work
if not success:
	system.nav.swapTo('Failed Login')
	system.tag.writeToTag("System3/BadgeSystem/LogoutTimerValue", 0)
	system.tag.writeToTag("System3/BadgeSystem/DisplayCountdown", 0)
	system.tag.writeToTag("System3/BadgeSystem/LogoutTimerRunning", 0)                                                                                            

You could make a template for the button, then pass each of the different button the appropriate security level. You would have a single instance of the base code.

1 Like

Interesting idea but not sure how that would work. How would I pass the security and correct page.

I kinda get it. But it has to execute in that page which is already open. So how would I pass something like a param?

Would it have to execute after separately?

You are currently hard coding the security level and pages right? Instead of doing that create template parameters, and reference the template parameters in your script. You can then set the custom properties for each button instance any way you like. With that you then don’t have to modify your code, just the custom properties.
Heres a very basic example, this is a templated button to open another view: i have three template parameters that i pass to each instance of the templated button.

The following code refactoring is considering you’re either running Ignition 8 or 8.1

  1. Extract the tag write calls into a single function and call writeBlocking
    • If you’re running an older version like 7.8 or 7.9, you may use system.tag.writeAll instead
  2. Change the way you check if the user has any of the roles by using any. This way you could pass the roles by setting a custom parameter of the component
# This script controls the badge access feature
# It allows the user to log into a page, or 
# redirects them to the invalid login, or access denied page
# it also resets and sets the timer to log out user after set time 
                                          
# Displays keyboard to scan badge or enter emergency use password
                                          
password = system.gui.showTouchscreenKeyboard("Scan Badge or Enter Emergency Password")
                                          
# opens and scans user table to cross reference password to username to provide login credentials.
system.nav.openWindow("Main/TImport")
window = system.gui.getWindow("Main/TImport")
data = window.rootContainer.getComponent("User Data").data
system.nav.closeWindow("Main/TImport")
user_name = ""

for row in range(data.rowCount):
	index_row = data.getValueAt(row, 3)
	if index_row == password:
		user_name = data.getValueAt(row, 1)
		break

def write_to_tag(logout_timer_value, display_countdown, logout_timer_running):
	# system.tag.writeAll( # if Ignition version < 8
	system.tag.writeBlocking(
		[
			"System3/BadgeSystem/LogoutTimerValue",
			"System3/BadgeSystem/DisplayCountdown",
			"System3/BadgeSystem/LogoutTimerRunning",
		],
		[logout_timer_value, display_countdown, logout_timer_running],
	)


# Allows successful access to page if user role has security clearance, enable and resets timer
# sends them to access denied page if user does not have valid security clearance
success = system.security.switchUser(user_name, password)
roles = system.security.getRoles()
allowed_roles = [u'Administrator', u'LVL_2', u'LVL_4'] # Here you could use the custom parameter

# Sends user to failed login page if badge scan does not work
if not success:
	system.nav.swapTo('Failed Login')
	write_to_tag(0, 0, 0)
elif any(role in roles for role in allowed_roles:
	system.nav.swapTo('P1 Main')
	write_to_tag(0, 1, 1)
else:
	system.nav.swapTo('Access Denied')
	write_to_tag(0, 0, 0)

Move the script into a project script, then call the script with appropriate parameters as needed.

As @thecesrom.git has shown there are ways to improve/clean up the script itself.

2 Likes

As a general rule of thumb recommendation I have for Ignition development - when you get told to implement a new piece of business logic (in this case logout logic) - write it in a project script first. And then call it from the GUI wherever you need. This helps keep your code DRY (Don't Repeat Yourself).

But I have to copy and past this into each button.

This would be the opposite of DRY code, appropriately named WET (Write Everything Twice). If you find yourself having to copy and paste the same lines of code all over the GUI know that you are engaging in an anti-pattern that will make future features and maintenance much harder.

There's a lot to be said about the styles and principles of good "clean" code some of which don't really apply to Ignition given how we are to design the GUI etc, but DRY code is one that is true pretty much in all scenarios.

Sorry for the rant, I'm working on a project that is completely WET, where there are 10 recipes so each window has 10 submit buttons all with the same code instead of a function call, and it's been a slog trying to make it clean. The best thing to do is to just keep it DRY from the start.

2 Likes

I don’t disagree. I was being rushed and did what I had to do. But I don’t really need the lecture , the whole point of my post was I realized my wrongs and was making an effort to fix them. Being new I had trouble grasping the implementation of it.

And while I feel good answers were provided I really have to look into them so I fully understand it. Once I fully understand what was said above it will make me a better programmer/integrator. I don’t just wanna “copy and paste” answers until I feel confident enough in my
Understanding that my next script will be written accordingly .

Thank you!

1 Like

I’m using 7.8.5

Didn’t mean it as a lecture, just trying to save you from future headaches the likes of which I have everyday from dealing with WET code :slight_smile:

1 Like

No worries. I gotta play around more. I really don’t get to script much where I work currently.

How do I write the param and how do I call script in scripting section of button. Or please link to documentation I’m having a hard time finding what I need to wrap this off.

Again thank you all

@thecesrom.git how would I call that as a whole in the button script. I see examples on documentation like script.def () to call function and what not in a script. Would I have to incase it in a def() would I have to pass params to script.def() or will it detect without doing that.

For your questions regarding calling scripts and passing parameters, I would definitely recommend looking at what Inductive University has to offer for 7.8

https://inductiveuniversity.com/courses/ignition-basics/7.8

As well as the docs.

https://docs.inductiveautomation.com/display/DOC

1 Like