Learning to code

Help and Feedback Code Review
,
#1

Hello Developers I would like to ask you to review my code and tell me what mistakes I have made and how to fix them.

The way the code works is based on an infinite loop that changes the colours of the blocks very similar to the floor of a discotheque where the floor changes colour every few seconds in this case I chose to change every 1 second and the code only works on the client side and not on the server.

local module = {script.Parent}

local Work = game:GetService("Workspace")

local test = game:GetService("TestService")

local Block1 = Work.Animation1.Block1

local block2 = Work.Animation1.Block2

local color1 = test.Colors.Color1.Value

local color2 = test.Colors.Color2.Value

local color3 = test.Colors["Color 3"].Value

local color4 = test.Colors.Color4.Value

local Alternative1 = test.Colors["Alternative Colours"].Alternative1.Value

local Alternative2 = test.Colors["Alternative Colours"].Alternative2.Value

local Alternative3 = test.Colors["Alternative Colours"].Alternative3.Value

local Alternative4 = test.Colors["Alternative Colours"].Alternative4.Value

while true do
	
	Block1.BrickColor = color1
	
	wait(1)
	
	
	block2.BrickColor = color2
	
	wait(1)
	
	Block1.BrickColor = color3
	
	wait(1)
	
	block2.BrickColor = color4
	
	wait(1)
	
	Block1.Color = Alternative1
	
	wait(1)

	block2.Color = Alternative2
	
	wait(1)
	
	Block1.Color = Alternative3
	
	wait(1)
	
	block2.Color = Alternative4
	
	warn("this script is the second one i've made")
	

	local block3 = Work.Animation2.Block3
	local block4 = Work.Animation2.Block4
	local block5 = Work.Animation2.Block5
	
	while true do
		
		block3.BrickColor = color1
		
		wait(1)
		
		block3.BrickColor = color2
		
		wait(1)
		
		block4.BrickColor = color3
		
		wait(1)
		
		block5.BrickColor = color4
		wait(1)
		Block1.Color = Alternative1
		wait(1)
		block2.Color = Alternative2
		wait(1)
		block3.Color = Alternative3
		wait(1)
		block4.Color = Alternative4
		
		
	end
		
	end

return module
1 Like
#2

Why is are these lines in your code?

local module = {script.Parent}

return module

If you use a normal serverScript you don’t need that normally.

#3

Is a ModuleScript no a server script

#4

The script you provided appears to be intended to change the colors of two objects in the game periodically. At the beginning of the script, a number of local variables are defined to store references to objects in the game and to some color values. The script then enters an infinite loop, which consists of a series of statements that change the colors of the two objects, wait for a second, and repeat the process. The script also defines another infinite loop nested inside the first one, which changes the colors of three additional objects. However, it is not clear what the purpose of the nested loop is, or what the block3 , block4 , and block5 variables are used for. It may be a good idea to add comments to the script explaining its purpose and how it works.

#5

If you want to change the color of a part every 1 second in an infinite loop, you can just do this:

local part = script.Parent -- Replace with the actual part you want to change colors

while true do
	part.BrickColor = BrickColor.Red()
	task.wait(1)
	part.BrickColor = BrickColor.Orange()
	task.wait(1)
	part.BrickColor = BrickColor.Yellow()
	task.wait(1)
end

You can also use BrickColor.random() to make it change into a random color:

local part = script.Parent -- Replace with the actual part you want to change colors

while true do
	part.BrickColor = BrickColor.random()
	task.wait(1)
end
#6

Here are a few suggestions for improving the code:

1. Add comments to explain what the code is doing. This will make it easier for others (and yourself) to understand the code and its purpose.
2. Use proper naming conventions for variables. In general, it is good practice to use camelCase or snake_case for variable names, rather than using all caps or camelCase with spaces.
3. Instead of having a series of wait(1) statements, you can use a for loop to iterate over an array of colors or alternatives. This will make the code easier to read and modify.
4. Consider using functions to organize the code and make it more modular. For example, you could create a function to handle the color changes for Block1 and Block2, and another function to handle the color changes for Block3, Block4, and Block5.
5. You may want to consider using a different way to achieve the desired animation, such as using a tween or a sequence of tweens. This can make the code more efficient and easier to modify.
6. Use local variables whenever possible, to minimize the scope of the variables and avoid unintended consequences.

I took your code and asked https://chat.openai.com/chat for improvements, hope they help.

#7

There are a few issues with this code:

  1. It has an infinite loop that runs within another infinite loop. This means that the inner loop will never end and the program will never terminate.
  2. The use of the wait function inside an infinite loop can cause performance issues and make the game unresponsive.
  3. It is not recommended to use a global variable module as it can cause conflicts with other variables with the same name.

Here is a revised version of the code that addresses these issues.

local Work = game:GetService("Workspace")
local Test = game:GetService("TestService")
local Block1 = Work.Animation1.Block1
local Block2 = Work.Animation1.Block2
local Block3 = Work.Animation2.Block3
local Block4 = Work.Animation2.Block4
local Block5 = Work.Animation2.Block5

local colors = {
	Test.Colors.Color1.Value,
	Test.Colors.Color2.Value,
	Test.Colors["Color 3"].Value,
	Test.Colors.Color4.Value
}

local alternativeColors = {
	Test.Colors["Alternative Colours"].Alternative1.Value,
	Test.Colors["Alternative Colours"].Alternative2.Value,
	Test.Colors["Alternative Colours"].Alternative3.Value,
	Test.Colors["Alternative Colours"].Alternative4.Value
}

while true do
	for i, color in ipairs(colors) do
		Block1.BrickColor = color
		Block2.BrickColor = colors[(i % #colors) + 1]
		wait(1)
	end
	
	for i, color in ipairs(alternativeColors) do
		Block1.Color = color
		Block2.Color = alternativeColors[(i % #alternativeColors) + 1]
		Block3.Color = alternativeColors[(i % #alternativeColors) + 1]
		Block4.Color = alternativeColors[(i % #alternativeColors) + 1]
		wait(1)
	end
end

This code will cycle through the colors in the colors and alternativeColors tables, changing the colors of Block1 and Block2 every second. It will also change the colors of Block3 , Block4 , and Block5 every second using the alternativeColors table. This revised version of the code will not have the performance issues and infinite looping of the original code.

#8

Here are the issues with the original code:

  1. It has an infinite loop that runs within another infinite loop. This means that the inner loop will never end and the program will never terminate.
  2. The use of the wait function inside an infinite loop can cause performance issues and make the game unresponsive.
  3. It is not recommended to use a global variable module as it can cause conflicts with other variables with the same name.

The infinite loops cause the program to never end, which can lead to performance issues and make the game unresponsive. The wait function should be used with caution inside infinite loops, as it can cause the game to become unresponsive if the wait time is too long or if the loop runs for too long.

Using a global variable module is generally not recommended as it can cause conflicts with other variables that have the same name. It is better to use local variables or to use a more descriptive and unique variable name.

#9

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.