How to make this efficient?

Help and Feedback Code Review
,
#1

How can I make this efficient without/with using math.random()?

local colors = require(script:WaitForChild("Color_Rarity"))
local Gui = script.Parent
local ColorButton = Gui.ColorButton
local function ChooseWhichColorCommon()
	local CommonRandom = math.random(1, 3)
	ColorButton.BackgroundColor3 = workspace.ColorChanger.Color
		if CommonRandom == 1 then
		colors.ChooseRandomCommonColor('Red')
	end
	
	if CommonRandom == 2 then
		colors.ChooseRandomCommonColor('Blue')
	end
	
	if CommonRandom == 3 then
		colors.ChooseRandomCommonColor('Green')
	end
	
	
end

local function MouseClicked()
	return ChooseWhichColorCommon()
	
end

ColorButton.MouseButton1Down:Connect(MouseClicked)
4 Likes
#2

Store the colors inside of an array.

local colors = {
	Color3.fromRGB(0, 0, 0);
	Color3.fromRGB(127.5, 127.5, 127.5);
	Color3.fromRGB(255, 255, 255)
}

local color = colors[math.random(1, #colors)]
1 Like
#3

Could I change the array to my module function?

1 Like
#4

You can do this inside of a function as well, just return the color once it’s randomized.

return color
1 Like
#5

Alright. I will test the script out!

1 Like
#6

It works but it gives me a error

1 Like
#7

It says that, “Players.Cxdious.PlayerGui.ColorGui.ColorChanger:14: invalid argument #2 to ‘random’ (interval is empty)” - Line 14

local colors = require(script:WaitForChild("Color_Rarity"))
local ColorChanger = workspace:WaitForChild("ColorChanger")
local Gui = script.Parent
local ColorButton = Gui.ColorButton

local CommonColors = {
	colors.ChooseRandomCommonColor("Red");
	colors.ChooseRandomCommonColor("Blue");
	colors.ChooseRandomCommonColor("Green")
}

local function ChooseWhichColorCommon()
	ColorButton.BackgroundColor3 = workspace.ColorChanger.Color
	local color = CommonColors[math.random(1, #CommonColors)]
	
	ColorChanger.BrickColor = BrickColor.new(color)
	return color
end

local function MouseClicked()
	return ChooseWhichColorCommon()
	
end

ColorButton.MouseButton1Down:Connect(MouseClicked)
#8

check that your colors.ChooseRandomCommonColor function returns a value.

this error insinuates that your table is empty.

1 Like
#9

I have these values. Sorry, I am new at doing modules and tables.

local CommonColors = {
	['Red'] = {
		['Color'] = BrickColor.new("Bright red")
		
	},
	
	['Green'] = {
		['Color'] = BrickColor.new("Bright green")
		
	},
	
	['Blue'] = {
		['Color'] = BrickColor.new("Bright blue")
		
	}
	
}
#10

did you just change the table, or is this in the module?

it should work if you changed the table.

if not, can you send the the ChooseRandomCommonColor function?

1 Like
#11

The values are in the module. Should I make the value into the table?

2 Likes
#12

I mean… you can, but you’d have to rewrite your code.

can you send the the ChooseRandomCommonColor function?
maybe I can help fix it.

1 Like
#13

Sure thing!

local ColorButton = script.Parent.Parent.ColorButton
local RS = game:GetService("ReplicatedStorage")
local PointEvent = RS.Color

local CommonColors = {
	['Red'] = {
		['Color'] = BrickColor.new("Bright red")
		
	},
	
	['Green'] = {
		['Color'] = BrickColor.new("Bright green")
		
	},
	
	['Blue'] = {
		['Color'] = BrickColor.new("Bright blue")
		
	}
	
}

function CommonColors.ChooseRandomCommonColor(color)
	local function rando()
	local random = math.random(1,4)
		if CommonColors[color] then
	
			if random == 2 then
	
				workspace:WaitForChild("ColorChanger").BrickColor = CommonColors[color].Color
				PointEvent:FireServer()
			end
	 
		end
	end

	ColorButton.MouseButton1Down:Connect(rando)
	print(workspace.ColorChanger.BrickColor)
end



return CommonColors
2 Likes
#14

that’s your issue.
the function does not return any value, and so the table in the first script does not work.

first off, you’ve put a function into a data table, so you should probably replace CommonColors with:

local CommonColors = {
    ["data"] = {
        ['Red'] = {
            ['Color'] = BrickColor.new("Bright red")
            
        },
        
        ['Green'] = {
            ['Color'] = BrickColor.new("Bright green")
            
        },
        
        ['Blue'] = {
            ['Color'] = BrickColor.new("Bright blue")
            
        }
    }
}

and then
replace all of that code with:

function CommonColors.ChooseRandomCommonColor(color)
	local colours = CommonColors.data
    local random = math.random(1, #colours)
    local final = random[math.random(1, #random)]
    return final
end

the only issue is that I have no idea what the remote event does.

The code above chooses one of the colours from the rom.

edit -lmk If you need any explaining on what I did.

1 Like
#15

How do I replace the mousebuttondown function?

2 Likes
#16

I was thinking that the main script would handle that,

and im pretty sure it does?

give me a minute to check it.

1 Like
#17

ahh, right so you were setting some sort of part to a colour, and then changing the button to that.

you can fix the button by replacing the first script with:

local colors = require(script:WaitForChild("Color_Rarity"))
local ColorChanger = workspace:WaitForChild("ColorChanger")
local Gui = script.Parent
local ColorButton = Gui.ColorButton

local CommonColors = {
	colors.ChooseRandomCommonColor("Red");
	colors.ChooseRandomCommonColor("Blue");
	colors.ChooseRandomCommonColor("Green")
}

ColorButton.MouseButton1Down:Connect(function()
    local colour = CommonColors[math.random(1, #CommonColors)]
    ColorButton.BackgroundColor3 = colour
end)

there was a LOT of unnecessary code there.

1 Like
#18

Now it changed to the color intended. Thank you! But, it’s going to be annoying to say but, now it says “Players.Cxdious.PlayerGui.ColorGui.ColorChanger:13: invalid argument #2 to ‘random’ (interval is empty)”

Edit: Line 13

2 Likes
#19

So it changes the colour correctly… but the colour cannot be found…

maybe you have a duplicate script?

1 Like
#20

I’ve checked everywhere. I don’t have a duplicated script. This problem is weird

1 Like