HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Bus Announcement System in ROBLOX Lua

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
robloxsystembusannouncementlua

Problem

I got bored and decided to create a lua announcement system with the ROBLOX API.

One part is the display code, and the other part is the announcement code.

DISPLAY CODE

```
while true do
wait(0.2)
if script.Parent.Parent.An.Value == 0 then
script.Parent.An1.Text = "This bus terminates here"
script.Parent.An2.Text = "Bus is on diversion"
script.Parent.Selected.Text = "The destination has changed"
script.Parent.An3.Text = "Driver change"
script.Parent.An4.Text = "Next stop is closed"
script.Parent.An5.Text = "Move Down"

elseif script.Parent.Parent.An.Value == 1 then
script.Parent.An1.Text = "Next stop is closed"
script.Parent.An2.Text = "This bus terminates here"
script.Parent.Selected.Text = "Bus is on diversion"
script.Parent.An5.Text = "Move Down"
script.Parent.An3.Text = "The destination has changed"
script.Parent.An4.Text = "Driver change"

elseif script.Parent.Parent.An.Value == 2 then
script.Parent.An5.Text = "Move Down"
script.Parent.An1.Text = "Driver change"
script.Parent.An2.Text = "Next stop is closed"
script.Parent.Selected.Text = "This bus terminates here"
script.Parent.An3.Text = "Bus is on diversion"
script.Parent.An4.Text = "The destination has changed"

elseif script.Parent.Parent.An.Value == 3 then
script.Parent.An1.Text = "The destination has changed"
script.Parent.An2.Text = "Driver change"
script.Parent.Selected.Text = "Next stop is closed"
script.Parent.An3.Text = "This bus terminates here"
script.Parent.An4.Text = "Bus is on diversion"
script.Parent.An5.Text = "Move Down"

elseif script.Parent.Parent.An.Value == 4 then
script.Parent.An1.Text = "Bus is on diversion"
script.Parent.An2.Text = "The destination has changed"
script.Parent.Selected.Text = "Move Down"
script.Parent.An3.Text = "Next stop i

Solution

DRY

You are repeating yourself a lot in your code. Here is just one example:

if script.Parent.Parent.An.Value == 0 then
    script.Parent.An1.Text = "This bus terminates here"
    script.Parent.An2.Text = "Bus is on diversion"
    script.Parent.Selected.Text = "The destination has changed"
    script.Parent.An3.Text = "Driver change"
    script.Parent.An4.Text = "Next stop is closed"
    script.Parent.An5.Text = "Move Down"

elseif script.Parent.Parent.An.Value == 1 then
    script.Parent.An1.Text = "Next stop is closed"
    script.Parent.An2.Text = "This bus terminates here"
    script.Parent.Selected.Text = "Bus is on diversion"
    script.Parent.An5.Text = "Move Down"
    script.Parent.An3.Text = "The destination has changed"
    script.Parent.An4.Text = "Driver change"

elseif script.Parent.Parent.An.Value == 2 then
    script.Parent.An5.Text = "Move Down"
    script.Parent.An1.Text = "Driver change"
    script.Parent.An2.Text = "Next stop is closed"
    script.Parent.Selected.Text = "This bus terminates here"
    script.Parent.An3.Text = "Bus is on diversion"
    script.Parent.An4.Text = "The destination has changed"
...


A big reason I see for this is these AnN things. If possible, try grouping things like these into arrays so you can easily iterate through them with loops, giving you the ability to work on the entire array with only a few lines, rather than several groups of a few lines.

DRY... again!

Along with those AnN variables, I see this in all places of your code:

if thing == 0 then

elseif thing == 1 then

elseif thing == 2 then

...


Then, in each block, very similar code is posted. This is, again, repeating yourself and should be avoided.

Try to fit the code in the blocks into a loop. If that means also creating an array of the different "bus messages", then okay. Then, as you are looping, you can use the index to compute which bus message to use.

Overall, you are really doing a lot of repeating in this code and that is not good.

Save your stuff

This ignores the above tip for now.

You are accessing this a lot:

script.Parent.AnN


This type of access, when overused as it is here, slows down your code. Store these sorts of things in variables:

local parent = script.Parent
local An1 = parent.An1
...


(if you follow the above tip, these AnN variables will probably be in an array instead).

Now, you are doing much less constant field access, overall speeding up your code.

Code Snippets

if script.Parent.Parent.An.Value == 0 then
    script.Parent.An1.Text = "This bus terminates here"
    script.Parent.An2.Text = "Bus is on diversion"
    script.Parent.Selected.Text = "The destination has changed"
    script.Parent.An3.Text = "Driver change"
    script.Parent.An4.Text = "Next stop is closed"
    script.Parent.An5.Text = "Move Down"

elseif script.Parent.Parent.An.Value == 1 then
    script.Parent.An1.Text = "Next stop is closed"
    script.Parent.An2.Text = "This bus terminates here"
    script.Parent.Selected.Text = "Bus is on diversion"
    script.Parent.An5.Text = "Move Down"
    script.Parent.An3.Text = "The destination has changed"
    script.Parent.An4.Text = "Driver change"

elseif script.Parent.Parent.An.Value == 2 then
    script.Parent.An5.Text = "Move Down"
    script.Parent.An1.Text = "Driver change"
    script.Parent.An2.Text = "Next stop is closed"
    script.Parent.Selected.Text = "This bus terminates here"
    script.Parent.An3.Text = "Bus is on diversion"
    script.Parent.An4.Text = "The destination has changed"
...
if thing == 0 then

elseif thing == 1 then

elseif thing == 2 then

...
script.Parent.AnN
local parent = script.Parent
local An1 = parent.An1
...

Context

StackExchange Code Review Q#115852, answer score: 2

Revisions (0)

No revisions yet.