Overcomplicated letter reveal code?

Help and Feedback Code Review
#1

Felt like I really overcomplicated this, but basically I wanted to have a random letter of a word be revealed every 30 seconds so I created a function that does so. (Example: The word to be guessed is “apple” and its shown as “_____” then a random letter is given as a hint and becomes “A____” and so on.) Any tips or ways I could’ve done this better? (I coded this super late at night so cut me some slack)

function replace_char(pos, str, r) -- replaces a character in a string given an index
	return str:sub(1, pos-1) .. r .. str:sub(pos+1)
end

function Randomizer.RevealLetter(word, str)
	local randomLetterIndex = math.random(1,#word)
	local randomLetter = ""
	local pass = false -- variable becomes true once a letter isn't being repeated
	repeat
		wait()
		for i, letter in ipairs(str:split("")) do -- loop through each character in string
			if i == randomLetterIndex then
				if letter ~= "_" then
					print('REPEATED, CHOOSE A DIFFERENT LETTER')
					randomLetterIndex = math.random(1,#word)
				else
					pass = true
				end
			end
		end
	until pass
	for index, v in ipairs(word:split("")) do -- loop through each character in string
		if index == randomLetterIndex then -- checks if index is equal to the chosen random index
			randomLetter = v
		end
	end
	return "The word is " .. replace_char(randomLetterIndex, str, randomLetter) -- replace character in the string
end
#2

Your loops are not necessary. What you’re doing is:

for i, v in ipairs(collection) do
    if i == specific_index then
        -- code
    end
end

When this can be simplified to:

local v = collection[specific_index]
-- code

Additionally, you’re doing str:split("") to separate your letters into a table and then grabbing from it. You could just use string.sub for that.

Then, your overall check loop doesn’t require a wait() as it will never be a performance issue unless your word has millions of letters and only a single underscore hidden somewhere. The code before the loop also already encompasses an iteration of it, so you can merge that in too. Something like:

repeat
    local randomIndex = math.random(1, #word)
    -- code
until -- cond

After all those changes and some more polishing, here’s what you could end up with:

function Randomizer.RevealLetter(word, shown)
	local randomIndex
	local randomLetter

	repeat
		randomIndex = math.random(1, #word)
		randomLetter = string.sub(word, randomIndex, randomIndex)
	until string.sub(shown, randomIndex, randomIndex) == "_"

	return "The word is " .. ReplaceChar(randomIndex, shown, randomLetter)
end
3 Likes
#4

Would this be simpler?

function replace_char(pos, str, r) -- replaces a character in a string given an index
	return str:sub(1, pos-1) .. r .. str:sub(pos+1)
end

function Randomizer.RevealLetter(word, str)
	return "The word is " .. replace_char(math.random(1,#word), str, word:split("")[math.random(1,#word)])
end

In application:

local word = "Hello"
local str = "The word is _ _ _ _ _"
local r = Randomizer(word)
print(r:RevealLetter(str))

The above code will print:
The word is H e l l o

#5

Wouldn’t work because it doesn’t account for letters that are already revealed and you are revealing the letter in a random position, not the original.