Code Efficiency

:information_source: Attention Topic was automatically imported from the old Question2Answer platform.
:bust_in_silhouette: Asked By colonelkurtz

I’m not an experience programmer, and I have two questions, the primary being if the code could be made more consice. I’m always looking to do the work in fewer lines, but I’m not experienced enough in either programming or Godot to know if it’s efficient. (I followed a tutorial for the below code and made some changes to fit what I needed.)

Description:
This code is attached to a Polygon2D with a child RichTextLabel. When my character interacts with the object, print_dialogue() is called, which makes the dialogue window appear and start printing. I use arrays to store my dialogue (var count and two extra arrays let the NPC say different things when I return). The _physics_process checks if I’m done printing, and if not, to continue printing the dialogue, letter by letter, using var timer and speed to give it that typewriter feel. Once it’s done printing, it sets the appropriate variables, moves to the next array (for future encounters) and allows my player character to move again.

Second question: does this code allow me to implement a feature that allows the player to press the select key while it’s typing to skip the timer and type the whole sentence instantly? I’ve been trying an if statement inside the if !donePrinting, but that just causes it to print twice, ex: “Hello personHello person.” It feels like I’d have to rewrite the whole code for this little feature, but I’m not convinced.

extends Polygon2D

var printing = false
var donePrinting = false
var pressed = false

var timer = 0
var count = 0
var textToPrint = ["Hello person", "Goodbye person"]
var nextText = ["Oi, you're back?", "Bugger off"]
var finalText = ["Bugger off"]

var currentChar = 0
var currentText = 0

const Speed = 0.05

func _input(event):
	if event.is_action_pressed("ui_select"):
		pressed = true

func print_dialogue():
	set_visible(true)
	printing = true

func _physics_process(delta):
	if printing:
		if !donePrinting:
			timer += delta
			if timer > Speed:
				timer = 0
				get_node("RichTextLabel").set_bbcode(get_node("RichTextLabel").get_bbcode() + textToPrint[currentText][currentChar])
				currentChar += 1
			if currentChar >= textToPrint[currentText].length():
				currentChar = 0
				timer = 0
				donePrinting = true
				currentText += 1
		elif pressed:
			donePrinting = false
			get_node("RichTextLabel").set_bbcode("")
			if currentText >= textToPrint.size():
				currentText = 0
				count += 1
				if count == 1:
					textToPrint = nextText
				else:
					textToPrint = finalText
				printing = false
				set_visible(false)
				get_node("../../Player/KinematicBody2D").canMove = true
	pressed = false

There’s a lot to comment on here… One obvious thing to clean up (for both code length and performance) is the multiple get_node() calls to access the same RichTextLabel node. You’d be much better off storing a reference to that node once and then simply using that reference as needed. So…

onready var rtLabel = $RichTextLabel

Then, replace all your get_node("RichTextLabel") references with rtLabel

There’s a coding concept called “DRY” (Don’t Repeat Yourself). Any time you find yourself repeating the same code over and over, that’s usually a sign of code-smell and you should consider a different approach…

jgodfrey | 2020-05-10 15:13

:bust_in_silhouette: Reply From: jluini

Your code could and should be made more concise and clear. That is always the case when you code, and may be difficult to achieve if you are not experienced.

Your question should be more concise and clear aswell. Try to realize why it is printed twice, and ask it with a minimum version to show your problem if it’s still there. Otherwise close the question.

Changing the whole code is something that you do all the time when programming and should not be a problem.