GameAnalytics / GA-SDK-ROBLOX

Repository for GameAnalytics Roblox SDK
MIT License
68 stars 45 forks source link

Enum name lookup should use table (dictionary/map) instead of an if-statement #42

Closed Quenty closed 5 years ago

Quenty commented 5 years ago

Enum name lookup should use table (dictionary/map) instead of an if-statement

This happens in init.lua

the1schwartz commented 5 years ago

I am sure what part of the code you are referring to. Could you post a line number to where this is happening and what the code should look instead.

cristianbercu commented 5 years ago

Hello @Quenty We are very keen on fixing the SDK and in order to do so we would like to better understand the issue you are seeing. Is it possible to add more details to your comment? Thanks!

Quenty commented 5 years ago

Yup! Sorry for the delay in responses!

https://github.com/GameAnalytics/GA-SDK-ROBLOX/blob/master/GameAnalyticsSDK/GameAnalytics/Events.lua#L180 https://github.com/GameAnalytics/GA-SDK-ROBLOX/blob/master/GameAnalyticsSDK/GameAnalytics/init.lua#L2

Lines like this should be rewritten using hashmaps to look things up. Lua only has Hashmaps! See something like this:

local PROGRESSION_STATUS = {
     Source = 1;
     Sink = 2;
}

-- This can be automatically generated if your keys are the same.
local PROGRESSION_STATUS_LOOKUP = {
     [1] = "Source";
     [2] - "Sink";
}

Here's what my GAFlowType.lua looks like in my rewrite.

--- Flow type for resource events
-- @module GAFlowType
-- @author Quenty

local require = require(game:GetService("ReplicatedStorage"):WaitForChild("Nevermore"))

local Table = require("Table")

return Table.ReadOnly({
    SINK = "Sink";
    SOURCE = "Source";
})
Quenty commented 5 years ago

The reason you want to do this is performance! You can almost certainly reduce lines of code, keep things simple, and also have faster code.

It's not strictly necessary (not a bug), but it's a good idea. If done properly will unify your enums.

the1schwartz commented 5 years ago

Thanks again for pointing me in the right direction. This should be fixed in the latest version (v.1.2.8)

the1schwartz commented 5 years ago

local Table = require("Table")

Doesn't load, I guess this is because it is a user defined script in our project?