I noticed in my code, I have a lot of the same strings hard coded, so decided to make a static class just to hold my strings.
Problem is, I can’t decide what the access levels.modifiers should be. Currently I have them as public const string, as I was previously using the strings in switch statements across my project, so they needed a constant value.
I’ve heard using static readonly string is a better practice than using public const BUT this gives me an error with my switch statements, is there any other way around that or is keeping them as public const a passable solution?
Don’t use magic strings to control your program! If you have a static class with a lot of named strings that will never change, that’s just a really bad enum.
Readonly isn’t better or worse than consts. They’re different - consts are for values that are known at compile time and will always be the same thing whenever you run the program. Readonly are for things that are supposed to be initialized when you create an object/load a class, and never changed after that.
The strings in the static class are just the names of my scenes, the switch i mentioned just gets the name of a certain scene and does stuff depending on what it is. It seemed good enough tbh.
Ah, okay, sorry about that. That’s a completely different thing than general hard-coded strings spread throughout the project. I was envisioning that you were storing the state of the game as strings, and checking the current state by checking those strings. It’s common among beginners, and it’s always a bad idea.
A place to store levels is another thing - you need those strings as that’s how Unity’s API works.
Could you give a bit more details about how you’re doing level loading? Do you have specific scripts that goes to specific levels, or do you have scripts for switching level that you send the names of the level to? What kind of game is this?
We’re solving this by storing all of the levels that exist in the build settings on edit time, and then use those to populate a drop down menu on our level changer objects, but that might not be relevant for your case.
Ahh I see why you had that thought now, I do make a lot of mistakes probably but thankfully not that one it seems. The only hard coded strings I have at the moment (aside from the case statements in my switch below), are my tags for collision purposes, the names of my enemies, and my scene names. Things that are very easily changed but used more than enough.
Atm here is my level loading code
void Awake()
{
// Get the scene name to decide what UI layout should be displayed
sceneName = SceneManager.GetActiveScene ().name;
switch (sceneName)
{
case ReferenceStrings.mainMenu:
AddButtons ();
break;
case ReferenceStrings.levelSelect:
AddButtons ();
break;
case ReferenceStrings.options:
AddButtons ();
break;
case ReferenceStrings.gameOver:
gameOverScore.text = "Final Score: " + app.model.gameModel.Score.ToString();
gameOverKills.text = "Total Kills: " + app.model.gameModel.TotalKills.ToString();
break;
default:
gameButtons.AddRange(Button.FindObjectsOfType<Button>());
pauseMenu.alpha = 0;
updateUI = true;
break;
}
}
// function to add buttons to the list depending on what level we're on.
void AddButtons()
{
gameButtons.AddRange(Button.FindObjectsOfType<Button>());
foreach(Button obj in gameButtons)
{
obj.onClick.AddListener(ChooseLevel);
}
}
The AddButtons function just gets the buttons from that specific scene and links them to the ChooseLevel function I have, which I will put below as it’s part of the level select in fairness. I’m sure there’s better ways to do this, but it’s only a 2D space shooter, and this way is extendable enough for something like that, if you have a better way then feel free to suggest it.
// Function to tell the gameController which level to load
void ChooseLevel()
{
// gets the name of the currently clicked button via the event system
string clickedName = EventSystem.current.currentSelectedGameObject.name;
// Pass the level to the gameController
switch (clickedName)
{
case "PlayButton":
app.controller.gameController.ChangeLevel (ReferenceStrings.levelSelect);
break;
case "OptionsButton":
app.controller.gameController.ChangeLevel (ReferenceStrings.options);
break;
case "ExitButton":
app.controller.gameController.ChangeLevel (ReferenceStrings.mainMenu);
break;
case "LevelOne":
app.controller.gameController.ChangeLevel (ReferenceStrings.levelOne);
break;
case "LevelTwo":
app.controller.gameController.ChangeLevel (ReferenceStrings.levelTwo);
break;
case "LevelThree":
app.controller.gameController.ChangeLevel (ReferenceStrings.levelThree);
break;
case "LevelFour":
app.controller.gameController.ChangeLevel (ReferenceStrings.levelFour);
break;
case "LevelFive":
app.controller.gameController.ChangeLevel (ReferenceStrings.levelFive);
break;
default:
print ("could not get the name of clicked button");
break;
}
}
So yeah, currently the GameView loads up each scene from the code above, then depending on what button is clicked, it’s passed to the GameController to handle loading the level.
It’s probably overly complicated, the project requires using MVC and I’ve noticed I keep over-complicating things due to this, since I’m new to that pattern.
The project… requires MVC? Are you in some software engineering class?
The code is fine enough if it works well. I wouldn’t change it only to do the same thing in a prettier way before you need to expand it. It has some issues you should think about in the future, though. Or you can think about if it’s getting graded.
The main problem is using the names of the buttons. I’m always really iffy about code that relies on checking the name of game objects. A team member might change that name, or you might change it when you’ve forgotten that your code relies on the name, and the error won’t be discovered until much later. Depending on the names of things in your scene (or the transform order) also means that as the project gets larger, the team will get terrified of doing necessary clean up in the hierarchy, as that might break code.
You could set this all up in the inspector. Have the button’s OnClick point to the script with the level loading code, and have ChooseLevel take a string-argument that’s the level you want to go to. Then you can assign that argument in the button’s inspector. That would function similarly, but guards against well-meaning gameobject name changes.
I’m putting a lot of time and effort into this in all honesty, so expanding it and releasing it as a free game is something I’d be considering, so the cleaner I can keep the code, the better.
Ahh that’s the thing, the point of the project is demonstrating scripting, the vast majority has to be written in code. In all honesty, if I was to release this, I would be setting these types of things up in the inspector as it’s far more versatile and I know that, since I’ve used it previously. I’m just trying to avoid anything other than scripting, for the sole purpose of this project.
I’m definitely agreed with you that I don’t like relying on the button names but it was the best solution from a bad bunch, ya know?
EDIT: Oh sorry, yeah I’m in a software degree so design patterns…wooo
for constant strings that are always the same, use const, as already stated
If you’re going to do that, I suggest naming your static class something that is more descriptive. Something like ‘SceneNames’:
public static class SceneNames
{
public const string GameOver = "GameOver";
}
Instead of long switch statements, you could create a ‘ISceneBehaviour’, then classes for each scene for how they behave… what gets enabled/disabled/etc, happens in those switch statements.
Adding new scenes to the game is then as easy as defining a new class for that scene and it’s behaviour.
Your button issue also can be done without that large switch statement, while still being done in code.
I notice you reach into the EventSystem and get out the currently clicked button… but what calls ‘ChooseLevel’? Is that called when some event is received? Why not have unique event handlers for each button, or tokens directly only the buttons.
ChooseLevel is called by the button’s OnClick - check the bottom of the first part of the code.
I noticed something there too, this:
This setup means that any button in the scene is going to try to load a level. That’s probably not what you want.
I’d ask whomever’s grading this if you can follow Unity standards - like setting up these connections in the inspector. If you can’t do that, I’d suggest that you generate the buttons in code (based on a prefab), instead of adding them to the canvas.
To someone not familiar with Unity, FindObjectsOfType is going to look incredibly bad - you’re just grabbing all the objects with a certain type? To someone that’s worked a bunch with Unity, FindObjectsOfType is almost as bad - each call carries an assumption about the state of the current scene that it’s impossible to ensure holds.[/QUOTE]
@Baste actually that’s exactly what I want. Yeah, I’m aware it’s most likely not the best way to do it but every button in the scene has a purpose, and only the buttons in each specific scene can be grabbed, so unless I’m missing something, it isn’t something that can “break”.
Plus if I decide to add another button, or remove one. I can simply delete/or add it in the inspector and then update “ChooseLevel” to reflect that. Quick and easy. FindObjectsOfType is actually well suited for what I am doing there, or at least i would say it is.
The only buttons in those scenes, are the buttons that go to different levels, so I don’t really see why it’s a bad thing to do in this scenario.
I didn’t catch that method (had to scroll in the code to see it)… yeah, that’s like super bad.
If you can’t set these things up in the inspector. Then at least identify them on load, rather than on click. Resolve them by name then.
And heck, you’re having to ‘configure’ the buttons in the editor/inspector seeing as you have to name them, why not just go a little extra. What’s so wrong with attaching the listener there? Or at the least attaching a script to each button to uniquely attribute it.
Ah screw it, I’ll just set them up fully in the inspector probably. I still personally, don’t see why it’s “super bad”, based on what I said above your post but you guys, to be fair, clearly know far more about programming than I do.
I’m trying to have as few scripts as possible, personally dislike the idea of having scripts just to handle my buttons, so that’s why the overall GameView script handles setting up all UI not related to the player or enemy objects.