Ammo count calculation

Help and Feedback Code Review
#1

I have this really redundant and inefficient code right here which updates 2 weapons’ ammo count:

function UpdateAmmoText()
	
	GameUI.Weapons.Weapon1.Ammo.Text = _G.Weapon1.WeaponStates.Ammo.Value.."/".. _G.Weapon1.WeaponConfigs.MaxAmmo.Value
	if _G.Weapon1.WeaponStates.CanShoot.Value == true then
		GameUI.Weapons.Weapon1.Ready.Text = "Ready"
		GameUI.Weapons.Weapon1.Ready.TextColor3 = Color3.new(0,170,0)
	else
		GameUI.Weapons.Weapon1.Ready.Text = "Unready"
		GameUI.Weapons.Weapon1.Ready.TextColor3 = Color3.new(255,0,0)
	end
	
	GameUI.Weapons.Weapon2.Ammo.Text = _G.Weapon2.WeaponStates.Ammo.Value.."/".. _G.Weapon2.WeaponConfigs.MaxAmmo.Value
	
	if _G.Weapon2.WeaponStates.CanShoot.Value == true then
		GameUI.Weapons.Weapon2.Ready.Text = "Ready"
		GameUI.Weapons.Weapon2.Ready.TextColor3 = Color3.new(0,170,0)
	else
		GameUI.Weapons.Weapon2.Ready.Text = "Unready"
		GameUI.Weapons.Weapon2.Ready.TextColor3 = Color3.new(255,0,0)
	end
	
	GameUI.Weapons.TotalAmmo.Text = _G.Weapon1.WeaponStates.Ammo.Value + _G.Weapon2.WeaponStates.Ammo.Value .."/".._G.Weapon1.WeaponConfigs.MaxAmmo.Value + _G.Weapon2.WeaponConfigs.MaxAmmo.Value
	
	if _G.Weapon1.Parent == char then
		GameUI.Weapons.Weapon1.BackgroundTransparency = 0
		GameUI.Weapons.Weapon1.ViewportFrame.ImageTransparency = 0
	else
		GameUI.Weapons.Weapon1.BackgroundTransparency = 0.5
		GameUI.Weapons.Weapon1.ViewportFrame.ImageTransparency = 0.5
	end

	if _G.Weapon2.Parent == char then
		GameUI.Weapons.Weapon2.BackgroundTransparency = 0
		GameUI.Weapons.Weapon2.ViewportFrame.ImageTransparency = 0
	else
		GameUI.Weapons.Weapon2.BackgroundTransparency = 0.5
		GameUI.Weapons.Weapon2.ViewportFrame.ImageTransparency = 0.5
	end
	
end

Is there anyway to eliminate the redundant act and improve it overall? This is definitely something inefficient, for instance, it looks terrible. Weapon1 and Weapon2 are both instance variables, it refers to a tool.

Here’s the output result of this:
https://gyazo.com/d29ea65f6cf83a1be593146f2d968399

5 Likes
#2

Looking through your code, it seems to be responsible for communicating three things;

  • the weapons ammo and clip sizes.
  • whether it is ready to be used or not.
  • whether it is equipped or not

Instead of one method that checks all these variables whenever one of them changes, consider making three methods that are then triggered by an event. This event can be fired in the code that handles the variables these feedback methods are based on.

For example; instead of setting a weapon’s CanShoot variable through the weapon code and then running this method on the client to update the Gui, have the weapon code fire an event to the client that it’s CanShoot variable was changed. This will then execute only the code that is required to update that specific label.

6 Likes
#3

Well this is run inside a RenderStepped, despite on the way to fire it, is there any other improvements can be made?

#4

It is generally good practice to stay away from Global Variables. They can be convenient but can get messy. There are plenty of posts on the dev forum of better ways than _G variables.

#5

It can be exploited by exploiters injecting script right?

#6

I mean sure, that’s one way if they figure out what the variables are called and how to exploit them. Apart from that though, global variables are typically bad practice because of how easily they can cause hard-to-track issues. If one script changes the global to something another script wasn’t expecting, things could get interesting (or very uninteresting) very quickly. It’s best to keep all variables self-contained in the script. Just define Weapon1 and Weapon2 before the function. I’m sure it’s something as simple as
“Weapon1 = game.Players.LocalPlayer.Backpack:findFirstChild(“Weapon1”)” (excuse the formatting, I’m on mobile)

On the same note as variables, the code will look infinitely better as soon as you stop using fully qualified paths for everything. What I mean is, add variables for the long repeated paths you have going on like “GameUI.Weapons.Weapon1”
It could easily be turned into simply “UIWeapon1” and you’ll save a ton of room and improve readability.

tl;dr - it’s all in the variables bro.