Clean Code thoughts on a Method that Instantiates and returns a bool

Unity Courses Talk
#1

The lecture sets up a public Method that both Instantiates and returns a bool.

using UnityEngine;

public class Tower : MonoBehaviour
{
    Bank bank;

    private void Start() {
        bank = FindObjectOfType<Bank>();
    }

    public bool CreateTower(Tower tower, Vector3 position, GameObject parent) {
        Instantiate(tower.gameObject, position, Quaternion.identity, parent.transform);
        return true;
    }
}

I understand that it might seem handy to have both “functions” in one method, but from what I’ve been reading on clean coding - a method should never do more than one thing.

In this case, when reading the method name, it does not suggest to return anything, so it might be confusing that it has a bool type.

#2

Disclaimer: This is just my own personal point of view

At this point, it makes no sense. Later it will validate whether or not it can create that tower before it does and return whether or not it did in fact create it.

Returning a bool isn’t considered ‘doing’ anything, so in this case it is only doing one thing - creating a tower. I agree that it should rather be called TryCreateTower(), but it is only doing one thing.

Now, later it will also check if there is enough resources (money in the bank) to create this tower and either create it if there is, or do nothing if there isn’t. You could argue that it is now doing two things but you have to be careful not to ‘over do-one-thing’ because it could have the opposite effect of what you are trying to achieve.
Let’s take your example; If this was only instantiating a tower and not validating if we can build it, we wouldn’t need the function to begin with. It’s only instantiating an object. So, the calling method could just call Instatiate(...) instead of CreateTower(...). But what is this calling method? Is it also still doing ‘only one thing’? Maybe not. So, now what do you do? Break this up into smaller ‘only-do-one-thing’ methods? And what about the method that calls that one? You have to decide what the granularity of that one thing would be. Granted, that could be as broad as ‘the one thing this does is to control the character’ which is a little extreme, but the other extreme is that ‘this one thing will only create an instance’.

This is not a great example, but in databases there is a thing called data normalization. Very clever people went and defined what the granularity of a database could and should be. These are called normal forms and are referred to as the first, second, third, etc. normal form (1NF, 2NF, 3NF). These can go to the n’th degree but it is generally considered good enough to have data in 3NF. What I’m trying to say is you can ‘single responsibility’ to the n’th degree, but you can also get to a point where it just defeats the whole purpose of what the single responsibility rule is trying to achieve. You have to decide what makes sense as a single responsibility. TryCreateTower(...) (as it should’ve been named) can create the tower. Should it check if there is enough resources? That’s up to you, but I think it’s ok. If it was named correctly, it will be clear that there is something that can cause it to not be created.

#3

Disclaimer: I am not very experienced neither in C#, programming, or clean code.

We would still want a function since we’re calling it on a different class. And we don’t want the Tile to be the one instantiating Towers.

The TryCreateTower() is on option. On my (beginner) level of programming, I’d rather see all the info in the call instead of having to look inside the code to see what it checks for. This might look something like

if(Bank.HaveFundsFor(selectedBuilding.GetCost()){
Bank.DecreaseFunds(selectedBuilding.GetCost());
Factory.SpawnBuiding(selectedBuilding);
}

Yes, this is much more words than TryCreateTower(), but it also states exactly what is going on.

There’s more to comment on this approach in regard to clean code.

Firstly, the course introduces features (or ideas) mid-lecture. Thus, we don’t have a clear overview from the start of the project, which makes planning the code architecture not very effective. Rather, we go along with the need of the lectures, and then maybe sometimes refactor (on our own if we want, or by prompts of the course material).

Secondly, if we focus on things other than the naming (which I agree is very important), then there would be more work needed to make the code for this course cleaner.

If we just take the example of the Waypoint.OnMouseDown() that should lead to the Tower class checking with the Bank class and then possibly Instantiating here are a couple of thoughts:

  1. The Waypoint should not need to know of any Tower or Bank, it should just send an Event that it was clicked on;
  2. We would probably have a Game State Machine that would enable filtering clicks in different states
  3. We would have a class that holds data on all possible buildings that we can build (Scriptable Objects)
  4. We would have a UI that changes based on what we selected to build and based on our available credits/currency
  5. We would want most of these to have as little knowledge and dependency on each other to have them be as decoupled as possible since it makes changes to the system much easier

Again, I am not very knowledgeable about this. And although it sometimes takes a bit more classes and events to setup up initially, the further we go along with the project scope, it stays “easy and elegant” to build upon such a system.

#4

All of these are very valid.

Most of the courses take an iterative approach, and not all of them attempt to teach best practices. They are trying to get the learner to understand the concepts of game development rather than ‘the best way to write the code’. The RPG course is probably the only course (only one I can think of now) that takes this iterative approach and refactor towards best practices.

Absolutely, but like I mentioned earlier; They don’t fully focus on the best way to write the code.

Knowledgeable enough if you ask me. It took me many years of writing rubbish code before I started realising these things.

Edit

It’s not about how many words you write. It’s about how many times you have to write the same words. If, for some reason, you have more things that can spawn buildings - not very relevant in this case, but there could be cases - you would have to write this same code every time. We try to avoid that.

#5

Thank you for the comments @bixarrio.

It’s nice to be able to discuss these topics.

I’m looking forward to the Turn-Based Strategy and RPG courses to expand my knowledge.

Having a blast learning and creating.

Just lack the determination (yet) to start a full project on my own.

#6

I actually hate discussing these topics because I have this fear that most of my own knowledge isn’t correct and I’m exposing my flaws…

I haven’t mentioned the turn-base strategy course because I don’t remember us really taking an iterative approach on it. While we did do some iteration, most of it was already being geared towards the final product. It’s a great course.

#7

I love my flaws being exposed. Sure it’s unpleasant, but it’s the only way to learn.

I’ve mentioned the Turn-Based one since from what I’ve seen so far from the instructor (Hugo) has a lot of emphasis on clean coding.

1 Like
#8

This topic was automatically closed 20 days after the last reply. New replies are no longer allowed.

Privacy & Terms