0
\$\begingroup\$

I am creating an Idle game right now (tycoon style) which has 9 trains, (8 of them needed to be purchased), and 1 is always active, no matter if you just started the game.

I was trying to create a simple code, but it got really really messy, so there's 8 buttons, that each one, should "instantiate" the train and the trail of it, and after you press the first button (the most left one) the one next to it (in the right side) should be interactable, and when they press that button the train&mine on it should appear (right now I'm just using gameobject.active = true), after they "purchased" the trail&train it should be always active (Using right now the playerprefs).

public class GameManager : MonoBehaviour
{
    public Button[] Buttons;
    public GameObject  TrainB, TrainC, TrainD, TrainE, TrainF, TrainG, TrainH, TrainI;
    public GameObject  MineB, MineC, MineD, MineE, MineF, MineG, MineH, MineI;
    public Text text;
    public int CurrentTrainToSpawn = 0;
    private void Start()
    {

    }
    private void Update()
    {
        UpdateTrainPreFab();
    }
    public void ButtonA()
    {
        int ButtonAChoosed = PlayerPrefs.GetInt("PressedButtonA", 0);
        if (ButtonAChoosed == 0)
        {
            Buttons[0].gameObject.active = false;
            TrainB.gameObject.active = true;
            MineB.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonA", 1);
        }
    }
    public void ButtonB()
    {
        int ButtonBChoosed = PlayerPrefs.GetInt("PressedButtonB", 0);
        if (ButtonBChoosed == 0 && PlayerPrefs.GetInt("PressedButtonA", 0) == 1)
        {
            Buttons[1].gameObject.active = false;
            TrainC.gameObject.active = true;
            MineC.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonB", 1);
        }
    }
    public void ButtonC()
    {
        int ButtonCChoosed = PlayerPrefs.GetInt("PressedButtonC", 0);
        if (ButtonCChoosed == 0 && PlayerPrefs.GetInt("PressedButtonB", 0) == 1)
        {
            Buttons[2].gameObject.active = false;
            TrainD.gameObject.active = true;
            MineD.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonC", 1);
        }
    }
    public void ButtonD()
    {
        int ButtonDChoosed = PlayerPrefs.GetInt("PressedButtonD", 0);
        if (ButtonDChoosed == 0 && PlayerPrefs.GetInt("PressedButtonC", 0) == 1)
        {
            Buttons[3].gameObject.active = false;
            TrainE.gameObject.active = true;
            MineE.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonD", 1);
        }
    }
    public void ButtonE()
    {
        int ButtonEChoosed = PlayerPrefs.GetInt("PressedButtonE", 0);
        if (ButtonEChoosed == 0 && PlayerPrefs.GetInt("PressedButtonD", 0) == 1)
        {
            Buttons[4].gameObject.active = false;
            TrainF.gameObject.active = true;
            MineF.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonE", 1);
        }
    }
    public void ButtonF()
    {
        int ButtonFChoosed = PlayerPrefs.GetInt("PressedButtonF", 0);
        if (ButtonFChoosed == 0 && PlayerPrefs.GetInt("PressedButtonE", 0) == 1)
        {
            Buttons[5].gameObject.active = false;
            TrainG.gameObject.active = true;
            MineG.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonF", 1);
        }
    }
    public void ButtonG()
    {
        int ButtonGChoosed = PlayerPrefs.GetInt("PressedButtonG", 0);
        if (ButtonGChoosed == 0 && PlayerPrefs.GetInt("PressedButtonF", 0) == 1)
        {
            Buttons[6].gameObject.active = false;
            TrainH.gameObject.active = true;
            MineH.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonG", 1);
        }
    }
    public void ButtonH()
    {
        int ButtonHChoosed = PlayerPrefs.GetInt("PressedButtonH", 0);
        if (ButtonHChoosed == 0 && PlayerPrefs.GetInt("PressedButtonG", 0) == 1)
        {
            Buttons[7].gameObject.active = false;
            TrainI.gameObject.active = true;
            MineI.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonH", 1);
        }
    }
    public void UpdateTrainPreFab()
    {
        int ButtonAChoosed = PlayerPrefs.GetInt("PressedButtonA", 0);
        if (ButtonAChoosed == 1)
        {
            Buttons[0].gameObject.active = false;
            TrainB.gameObject.active = true;
            MineB.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonA", 1);
        }
        int ButtonBChoosed = PlayerPrefs.GetInt("PressedButtonB", 0);
        if (ButtonBChoosed == 1 && PlayerPrefs.GetInt("PressedButtonA", 0) == 1)
        {
            Buttons[1].gameObject.active = false;
            TrainC.gameObject.active = true;
            MineC.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonB", 1);
        }
        int ButtonCChoosed = PlayerPrefs.GetInt("PressedButtonC", 0);
        if (ButtonCChoosed == 1 && PlayerPrefs.GetInt("PressedButtonB", 0) == 1)
        {
            Buttons[2].gameObject.active = false;
            TrainD.gameObject.active = true;
            MineD.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonC", 1);
        }
        int ButtonDChoosed = PlayerPrefs.GetInt("PressedButtonD", 0);
        if (ButtonDChoosed == 1 && PlayerPrefs.GetInt("PressedButtonC", 0) == 1)
        {
            Buttons[3].gameObject.active = false;
            TrainE.gameObject.active = true;
            MineE.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonD", 1);
        }
        int ButtonEChoosed = PlayerPrefs.GetInt("PressedButtonE", 0);
        if (ButtonEChoosed ==1 && PlayerPrefs.GetInt("PressedButtonD", 0) == 1)
        {
            Buttons[4].gameObject.active = false;
            TrainF.gameObject.active = true;
            MineF.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonE", 1);
        }
        int ButtonFChoosed = PlayerPrefs.GetInt("PressedButtonF", 0);
        if (ButtonFChoosed == 1 && PlayerPrefs.GetInt("PressedButtonE", 0) == 1)
        {
            Buttons[5].gameObject.active = false;
            TrainG.gameObject.active = true;
            MineG.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonF", 1);
        }
        int ButtonGChoosed = PlayerPrefs.GetInt("PressedButtonG", 0);
        if (ButtonGChoosed == 1 && PlayerPrefs.GetInt("PressedButtonF", 0) == 1)
        {
            Buttons[6].gameObject.active = false;
            TrainH.gameObject.active = true;
            MineH.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonG", 1);
        }
        int ButtonHChoosed = PlayerPrefs.GetInt("PressedButtonH", 0);
        if (ButtonHChoosed == 1 && PlayerPrefs.GetInt("PressedButtonG", 0) == 1)
        {
            Buttons[7].gameObject.active = false;
            TrainI.gameObject.active = true;
            MineI.gameObject.active = true;
            PlayerPrefs.SetInt("PressedButtonH", 1);
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ If you need to have that many named objects of the same type, used in repetitive code like that, you should rethink how you're doing things. Perhaps arrays would be helpful? \$\endgroup\$ – 1201ProgramAlarm Oct 6 '18 at 4:24
1
\$\begingroup\$

I had take a look on your source code, and my comments are bellow:

Readable:

  1. You should follow coding conventions. Such as: add an empty line between brackets( {} ). Ex:
int ButtonAChoosed = PlayerPrefs.GetInt("PressedButtonA", 0);
if (ButtonAChoosed == 0)
{
    .....your logic code
}

then you should add an empty line between int ButtonAChoosed ... and if (ButtonAChoosed == 0):

int ButtonAChoosed = PlayerPrefs.GetInt("PressedButtonA", 0);

if (ButtonAChoosed == 0)
{
    .....your logic code
}
  1. You are using some magic numbers, values. Ex: 0,1,..,7 are magic numbers. You should define constants or something else to make it more meaning.

  2. Redundant properties: There are some redundant properties in your source code. Ex: I don't see where are you using bellow properties. In case you don't use it, you should delete it.

public Text text;    
public int CurrentTrainToSpawn = 0;

OOP:

Don't public members of class if you don't use it out side the class. I found that allmost of your class members are public. Ex:

public Button[] Buttons;    
public void ButtonA()

Then please consider to private it if you don't use them out side of the class.

DRY:

Don't Repeat Yourselft: Almost of your methods( ButtonA(), ButtonB(),...) have similar logic. Then you can consider group similar logic, define a method for the group of logic. And then you can reuse it:

As my quickly review, I changed bellow properties to 2 list of objects:

public GameObject TrainB, TrainC, TrainD, TrainE, TrainF, TrainG, TrainH, TrainI;

public GameObject MineB, MineC, MineD, MineE, MineF, MineG, MineH, MineI;

And then, I grouped your logic as a ButtonCommand() method:

private List<GameObject> Trains;
private List<GameObject> Mines;

private void ButtonCommand(string buttonName, int buttonIndex, string checkedButtonName = "")
{
    int buttonChoosed = PlayerPrefs.GetInt(buttonName, 0);
    if (buttonChoosed == 0 && (string.IsNullOrEmpty(checkedButtonName) || layerPrefs.GetInt(checkedButtonName, 0) == 1))
    {
        Buttons[index].gameObject.active = false;   
        Trains[index + 1].gameObject.active = true;
        Mines[index + 1].gameObject.active = true;
        PlayerPrefs.SetInt(buttonName, 1);
    }
}

And then you can reuse it for ButtonA(), ButtonB(),...methods. The code become more shorter and easier to maintain:

    public void ButtonA()
    {
        ButtonCommand("PressedButtonA", 0);
    }

    public void ButtonB()
    {
        ButtonCommand("PressedButtonB", 1, "PressedButtonA");
    }

    //............

    public void ButtonH()
    {
        ButtonCommand("PressedButtonH", 7, "PressedButtonI");
    }

    public void UpdateTrainPreFab()
    {
        ButtonCommand("PressedButtonA", 0);
        ButtonCommand("PressedButtonB", 1, "PressedButtonA");
        //........
        ButtonCommand("PressedButtonH", 7, "PressedButtonI");
    }

Design patterns:

I think you can consider to apply command patterns.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.