How do I make this map picker use less lines of code?

Help and Feedback Code Review
, ,
#1

  • What does the code do and what are you not satisfied with? - The code is supposed to pick a map with map voting.

  • What potential improvements have you considered? - Using something else than using a lot of if and elseif statements.

  • How (specifically) do you want to improve the code? - Make the code more efficient and take up less space.

local MapModule = {}

MapModule.Maps = {}

function MapModule.PickMap()
	for I, V in pairs(MapModule.Maps) do
		table.remove(MapModule.Maps, I)
	end
	
	local Rand = Random.new()
	local Maps = game:GetService("ServerStorage").Maps:GetChildren()
	local RandomMap1 = Maps[Rand:NextInteger(1, 3)]
	local RandomMap2 = Maps[Rand:NextInteger(4, 5)]
	local RandomMap3 = Maps[Rand:NextInteger(6, 7)]
	
	table.insert(MapModule.Maps, 1, RandomMap1:Clone())
	table.insert(MapModule.Maps, 2, RandomMap2:Clone())
	table.insert(MapModule.Maps, 3, RandomMap3:Clone())
	
	game:GetService("ReplicatedStorage").MapPick:FireAllClients(RandomMap1.Name, RandomMap2.Name, RandomMap3.Name)
	
	return RandomMap1, RandomMap2, RandomMap3
end

function MapModule.PlaceMap()
	
	if game:GetService("ReplicatedStorage").Map1Vote.Value > game:GetService("ReplicatedStorage").Map2Vote.Value and game:GetService("ReplicatedStorage").Map3Vote.Value then
		for _, V in pairs(MapModule.Maps[1]:GetChildren()) do
			V.Parent = workspace.Props
		end

		return
	elseif game:GetService("ReplicatedStorage").Map2Vote.Value > game:GetService("ReplicatedStorage").Map1Vote.Value and game:GetService("ReplicatedStorage").Map3Vote.Value then
		for _, V in pairs(MapModule.Maps[2]:GetChildren()) do
			V.Parent = workspace.Props
		end	

		return
	elseif game:GetService("ReplicatedStorage").Map3Vote.Value > game:GetService("ReplicatedStorage").Map2Vote.Value and game:GetService("ReplicatedStorage").Map1Vote.Value then
		for _, V in pairs(MapModule.Maps[3]:GetChildren()) do
			V.Parent = workspace.Props
		end	

		return
	elseif game:GetService("ReplicatedStorage").Map1Vote.Value == game:GetService("ReplicatedStorage").Map2Vote.Value then
		local RandomNumber = math.random(1, 2)
		
		for _, V in pairs(MapModule.Maps[RandomNumber]:GetChildren()) do
			V.Parent = workspace.Props
		end		

		return			
	elseif game:GetService("ReplicatedStorage").Map2Vote.Value == game:GetService("ReplicatedStorage").Map3Vote.Value then
		local RandomNumber = math.random(2, 3)
		
		for _, V in pairs(MapModule.Maps[RandomNumber]:GetChildren()) do
			V.Parent = workspace.Props
		end	

		return		
	elseif game:GetService("ReplicatedStorage").Map1Vote.Value == game:GetService("ReplicatedStorage").Map3Vote.Value then
		local RandomNumber = math.random(0, 2)
		
		for _, V in pairs(MapModule.Maps[RandomNumber + 1]:GetChildren()) do
			V.Parent = workspace.Props
		end	

		return		
	end
	
	if game:GetService("ReplicatedStorage").Map3Vote.Value == 0 and game:GetService("ReplicatedStorage").Map1Vote.Value == 0 and game:GetService("ReplicatedStorage").Map2Vote.Value == 0 then
		local Maps = game:GetService("ServerStorage").Maps:GetChildren()
		local Rand = Random.new()
		local RandomMap = Maps[Rand:NextInteger(1, #MapModule.Maps)]
		
		for _, V in pairs(RandomMap:GetChildren()) do
			V.Parent = workspace.Props
		end	

		return
	else
		local Maps = game:GetService("ServerStorage").Maps:GetChildren()
		local Rand = Random.new()
		local RandomMap = Maps[Rand:NextInteger(1, #Maps)]
		
		for _, V in pairs(RandomMap:Clone():GetChildren()) do
			V.Parent = workspace.Props
		end			
	end
end

function MapModule.DeleteMap()
	for _, V in pairs(workspace.Props:GetChildren()) do
		V:Destroy()
	end	
end

return MapModule

Thanks for your time.

3 Likes
#2

Hey there. First and foremost, I recommend setting Replicated Storage to a specific variable. If you put a line such as this at the top of your code local RStorage = game:GetService(“ReplicatedStorage”)
You can do this for any variables that are used more than a single time. This should shave quite a few characters off of your code.

#3

Thanks. Is that the only improvement I can make?

#4

First and foremost, I want to preface this with the following: Just as much as code being too long and explicit is a sign of a greater problem, succinct code can cause the same issues when too extreme. Therefore, instead of assuming your code takes up “too much” space, look for ways it can be more readable, more maintainable, and more efficient.

This out of the way, I’ve made a rather drastic rewrite that’s heavily dynamic and more maintainable. I’ve also adopted a bit of a functional paradigm, trying to eliminate as much implicit state as possible (Because who doesn’t love pure functions?). Also, I don’t believe any else or elseif statements exist to achieve exhaustiveness checking.

I also want to preface this further that this is not a prescriptive solution. Code is extremely subjective when it comes to styles, especially on such a decentralized platform like Roblox. The following snippet should just be used as a reference to help better inform future stylistic and design decisions. Without further ado, my rendition of a map manager in the context given:

-- NOTE: This script will not work drag-and-drop as certain stylistic changes
-- have occured that may be incompatible.

local ReplicatedStorage = game:GetService("ReplicatedStorage");
local ServerStorage = game:GetService("ServerStorage");

local rand = Random.new();

--[[
    A module that manages maps, their votes, and how they're loaded.
]]
local MapManager = {};

--[[
    Randomly polls all the maps in the game and selects a given amount of them,
    ensuring no back-to-back selections.

    @param {number} listingLength - Integer. The amount of random maps to fetch.
    @param {Instance?} [lastMap] - Optional. The last map that was played.
    @returns {Instance[]} - The current map listing to be voted on.
]]
function MapManager.getRandomMapListing(listingLength, lastMap)
    assert(type(listingLength) == "number");
    assert(listingLength > 0);
    assert(math.floor(listingLength) == listingLength);
    assert(type(lastMap) == "nil" or typeof(lastMap) == "Instance");

    local mapListing = {};
    local maps = ServerStorage.Maps:GetChildren();

    if lastMap ~= nil then 
        for index, map in pairs(maps) do
            if map == lastMap then
                table.remove(maps, index);
                break;
            end
        end
    end

    assert(#maps >= listingLength);

    while #mapListing < listingLength do
        local randomIndex = rand:NextInteger(1, #maps);
        local randomMap = maps[randomIndex];
        table.insert(mapListing, randomMap);
        table.remove(maps, randomIndex);
    end

    return mapListing;
end

--[[
    Communicate to the clients the names of the maps to be voted upon.

    @param {Instance[]} mapListing - The maps in question.
    @returns {nil}
]]
function MapManager.fireClientsMapListing(mapListing)
    assert(type(mapListing) == "table");
    assert(#mapListing > 0);

    local mapNames = {};

    for _, map in ipairs(mapListing) do
        assert(typeof(map) == "Instance");
        table.insert(mapNames, map.Name);
    end

    ReplicatedStorage.MapSelection:FireAllClients(mapNames);
end

--[[
    Create the relevant instances that hold the tally for map votes.

    @param {Instance[]} mapListing - The maps to be voted on.
    @returns {Folder} - The folder containing the `IntValue` instances.
]]
-- TODO: Investigate way of avoiding the use of `ValueBase` objects to tally
-- votes. I imagine tossing around a number literal is less intensive than
-- maintaining a reference to the instance. 
function MapManager.populateVoteValues(mapListing)
    assert(type(mapListing) == "table");
    assert(#mapListing > 0);

    local votesFolder = Instance.new("Folder");
    votesFolder.Name = "Votes";
    votesFolder.Parent = ReplicatedStorage;

    for _, map in ipairs(mapListing) do
        assert(typeof(map) == "Instance");
        local intValue = Instance.new("IntValue");
        intValue.Name = map.Name;
        intValue.Parent = votesFolder;
    end

    return votesFolder;
end

--[[
    Returns which map is the most voted on within the given listing.
    Resolves any sort of tie through random selection of the highest voted maps.

    @param {Instance[]} mapListing - The listing of maps.
    @param {Folder} votesFolder - The `IntValue` objects used in tallying.
    @param {boolean} [final=false] - Optional. If this is the last poll.
    @returns {Instance} - The selected map.
]]
function MapManager.getMostVotedMap(mapListing, votesFolder, final)
    assert(type(mapListing) == "table");
    assert(#mapListing > 0);
    assert(typeof(votesFolder) == "Instance");
    assert(votesFolder:IsA("Folder"));
    assert(type(final) == "boolean" or type(final) == "nil");

    local highestValue = -math.huge;
    local intValues = votesFolder:GetChildren();

    for _, intValue in ipairs(intValues) do
        assert(typeof(intValue) == "Instance");
        assert(intValue:IsA("IntValue"));

        if intValue.Value > highestValue then
            highestValue = intValue.Value;
        end
    end

    local possibleMapsNames = {};

    for _, intValue in ipairs(intValues) do
        if intValue.Value == highestValue then
            table.insert(possibleMapsNames, intValue.Name);
        end
    end

    local randomMapNameIndex = rand:NextInteger(1, #possibleMapsNames);
    local randomMapName = possibleMapsNames[randomMapNameIndex];

    for _, map in ipairs(mapListing) do
        if map.Name = randomMapName then
            return map;
        end
    end

    if final then
        votesFolder:Destroy();
    end

    -- Ergo, the point of no return:
    error("map listing and votes are unsynchronized", 2);
end

--[[
    Clones the given map and parents it to `workspace`.
    
    @param {Instance} map - The map to clone and load.
    @returns {Instance} - The reference to the cloned map.
]]
function MapManager.loadMap(map)
    assert(typeof(map) == "Instance");
    local mapClone = map:Clone();
    mapClone.Parent = workspace;
    return mapClone;
end

--[[
    Unloads the given map.

    @remarks 
    Ensure the map that's unloaded is not one of the template maps; rather, it
    should be the cloned map as retreived through `MapManager.loadMap`.

    @param {Instance} map - The map to unload.
    @returns {nil}
]]
-- TODO: Remove this function and any of its uses. It's more confusing than
-- simply invoking `:Destroy()` on the map's clone.
function MapManager.unloadMap(map)
    assert(typeof(map) == "Instance");
    map:Destroy();
end

return MapManager;

If you have any questions, feel free to ask! I’d be more than happy to explain my rationales and answer any follow-ups.

1 Like
#5

@TheEdgyDev
@MFCmaster1234

What order do I call the functions?

Thanks.

#6

I see EdgyDev ended up writing the script entirely for you. Good to hear. I prefer to lead towards an answer rather than give away. Otherwise you learn less. I see however that they added some teachable moments for you in the comments, so Im glad you were able to learn. Any other questions or issues with this map feel free to let me know.

#7

I imagine the general game loop works as follows in the context of a round-based game:

  1. Intermission begins.
  2. A listing of maps is randomly picked.
  3. Players are prompted the listing and vote accordingly.
  4. The votes are tallied and the map is selected.
  5. Deinitialization ensues for anything no longer needed.
1 Like
#8

So, basically:

local Listing = MapManager.getRandomMapListing(#game.ServerStorage.Maps:GetChildren())

MapManager.fireClientsMapListing(MapManager.getRandomMapListing(Listing))

MapManager.populateVoteValues(Listing)

local ChosenMap = MapManager.getMostVotedMap(Listing, game.ReplicatedStorage.Votes)

MapManager.loadMap(ChosenMap)

MapManager.unloadMap(ChosenMap)

Again, I have to say, thank you so much for writing a whole script for me!

I will add this after I read through it and understand how it works.

1 Like