How can I make my code look, better?

Help and Feedback Code Review
1

My code, looks like absolute dookie to me ig.
I can’t name stuff good and can’t make my code look good

If you want code examples feel free to ask

1 Like
2

Can’t exactly give ya advice without seeing your code first

1 Like
3 
local function check(val, type)
	local t = typeof(val)
	return t == type, t;
end

local function lookupify(t)
	local l = {};
	for i,v in next, t do
		l[v] = i;
	end

	return l;
end

local function setAttributes(attributes, object)
	if not attributes then return end;
	for i,v in next, attributes do
		local success = pcall(object.SetAttribute, object, i, v);
		if not success then
			warn(string.format("Error on setting attributes, Name %s as %s into %s", i, tostring(v), object.Name))
		end
	end
end

local function removeInstanceOfName(instanceName, parent)
	if parent:FindFirstChild(instanceName) then
		repeat if parent:FindFirstChild(instanceName) then
				parent[instanceName]:Destroy()
			else 
				break
			end
		until false
	end
	return
end

local function setup(instanceData, object, parent)
	local nameFound;
	for i, v in ipairs(instanceData) do
		if check(v, "Instance") and object.Parent == nil and object.Parent ~= v then
			object.Parent = v;
		elseif not nameFound and check(v, "string") and object.ClassName ~= v then
			nameFound = true;
			object.Name = v;
		end
	end
end

local function addIntoDescendants(descendants, instance, order)
	if order == 1 then
		descendants[#descendants + 1] = instance;
	elseif descendants[instance.Name] then
		local index = 1;
		local key = instance.Name
		repeat task.wait() 
			index += 1;
			key = string.format("%s#%s", instance.Name, index);
			if not descendants[key] then
				break
			end
		until false;
		descendants[key] = instance
	else
		descendants[instance.Name] = instance
	end	
end

local function instanceNew2(instanceData)
	for i,v in next, instanceData do
		local success, object = pcall(Instance.new, instanceData[1])
		if success then
			return object
		end
	end
	
	error("Cannot create instance! You must add a valid Class!");
end

function createInstance(instanceData : {Instance | string}, listDescendants, parent)
	local object = instanceNew2(instanceData);
	local descendants = {};

	if parent then object.Parent = parent; end;
	setup(instanceData, object, parent)

	local function setProp(property, value)
		if property == "Parent" and parent then return end;

		local propertyExists, typeof = pcall(function() return typeof(object[property]) end);
		if not propertyExists then return end;

		if check(value, typeof) or typeof == "nil" then
			object[property] = value;
		else
			warn("Wrong value type!", value);
		end
	end

	local function createChild(instanceData)
		local instance, descendants2 = createInstance(instanceData, listDescendants, object);
		if not listDescendants then  return end
		addIntoDescendants(descendants, instance, listDescendants)
		
		for _, object in next, descendants2 do
			addIntoDescendants(descendants, object, listDescendants)
		end
	end

	for key, value in next, instanceData do
		if forbidden[key] then continue end
		if typeof(value) == "table" then
			createChild(value);
		else
			setProp(key, value)
		end
	end
	
	if instanceData.attributes then
		setAttributes(instanceData.attributes, object);
	end

	return object, descendants;
end

forbidden = lookupify{"attributes", "Attributes"};

return {
	createInstance = createInstance,	
	removeInstanceOfName = removeInstanceOfName,
};
1 Like
4

A major thing i see is one letter variables, if you’re fine with that then keep it but to me they usually look ugly. Sometimes I find if i want to make a temporary variable I usually name it temp instead of a one letter string. Also, I’m a little confused on what this code does, could you explain please?

local function removeInstanceOfName(instanceName, parent)
	if parent:FindFirstChild(instanceName) then
		repeat if parent:FindFirstChild(instanceName) then
				parent[instanceName]:Destroy()
			else 
				break
			end
		until false
	end
	return
end
1 Like
5

This code is used to make instance[s], my fingers kept aching from typing out “Instance.new” too much. So if I wanted to make like a part in Workspace I would type

create{"Part", workspace}
or if I wanted to make a Part called anything I would type out
create{"Part", workspace, "anything"}
or if I wanted to bulid a tree of parts then

create{
	"Part",
	workspace,
	{
		"Part",
		"Branch",
		{
			"Part",
			"Branch",
		},

	},
	{
		"Part",
		"Branch",
		{
			"Part",
			"Branch",
		},
	},
	{
		"Part",
		"Branch",
		{
			"Part",
			"Branch",
		},

	},
	{
		"Part",
		"Branch",
		{
			"Part",
			"Branch",
		},
	},
	{
		"Part",
		"Branch",
		{
			"Part",
			"Branch",
		},
	},

}
1 Like
6

Don’t reinvent the wheel
Also generic iteration would be faster than next
next could only be faster if you use only 1 retrived argument inside loop.
Also why the hell do you put SetAttribute inside pcall?
Bro what?
There is already lightweight frameworks that are doing exactly that anyway [1.3.2] Hierarchy Builder - Lightweight D.S.L

7

When attempting to set an attribute to an unsupported type, an error will be thrown.

Plus I dont even use this, I was just bored.

8

If you want to get crafty with the function names there are some generally accepted “guidelines” (source = me):

  • “main” functions should be in PascalCase so they standout easier. For example:
    function Setup()

  • utility functions like function getColour() would be in camelCase.

  • in modules or tables private functions (functions only used by the table and it’s functions) should be prefixed with a “_”. For example: function _isActive() (This can be used in correlation with the utility function camelCase.

1 Like
9

Use comments. You can do nice comment customisations and plus it helps when you come back later on and understand what the code actually does.