Keeping methods readable and concise

Hello,

This is more of a general problem with architecture post, so feel free to skip it if such a topic doesn't tickle your fancy at the current moment
Anyway, my problem is that, despite keeping the responsibility/intention logic of my methods very small and focused. The moment i need to make that logic cover various (branching) conditions, the consequent conditional (if/else) blocks quickly become numerous. And the method becomes huge -in terms of just the branching (not necessarily from handling more responsibilities).

So for for anyone that's long since resolved this pitfall, could you be so kind as to tell me how to do the same please?

 

Whenever I have a method which grows significantly in size with various branching if/thens I consider it a candidate to get broken up into multiple methods. Often I later end up with various methods that do the actual work, and the original method just having the if/then logic. YMMV

 

To quote Knuth, "computer programming is an art, because it applies accumulated knowledge to the world, because it requires skill and ingenuity, and especially because it produces objects of beauty."

It sounds flowery, but one way to read it is that when code looks unwieldy, it's a message to step back and get creative about how you're framing the problem. You can usually rephrase long if/thens using a different approach such as virtual methods. (There are a gazillion ways to refactor if/thens; I just mention virtual methods because they're relatively easy to understand.)

The classic example is drawing shapes. Say you have this ugly method:

Code (csharp):

1. void DrawShape(Shape shape)
2. {
3.     if (shape.type == circle)
4.     {
5.         DrawCircle(shape);
6.     }
7.     else if (shape.type == square)
8.     {
9.         DrawSquare(shape);
10.     }
11.     else if (shape.type == triangle)
12.     {
13.         DrawTriangle(shape);
14.     }
15. }

Not only is it ugly, but you'd also need to modify the method every time you add a new shape.

It would be better if the shape knows how to draw itself. Then your DrawShape() method is just one line:

Code (csharp):

1. void DrawShape(Shape shape)
2. {
3.    shape.Draw();
4. }
5.  
6. class Circle : Shape { /* code to draw circle */ }
7. class Square : Shape { /* code to draw triangle */ }
8. class Triangle : Shape { /* code to draw triangle */ }

Again, polymorphism certainly isn't the only solution here. But it demonstrates that stepping back and taking a different approach is often better than simply chopping up a long, unwieldy method into several smaller unwieldly methods. The more approaches you know about, the easier it is to reframe the problem. That's part of the art that Knuth was talking about, and it's a continual learning process.

 

Hello Toni,
I simply had to reply and thank you. This was a great example, and something i'll definitely be using going forward.
Did you by chance learn this through trial-and-error? Or from a book(s)?

 

My academic background is in software engineering. But even if we read something in a book or hear it in a lecture, we never truly learn it without lots of trial-and-error and continual learning, right?

 

Yep, spot on.

Cheers.

 

This. A good rule of thumb I've found is to look at the method in your IDE. If you can't see the entire method on one screen (i.e. you need to scroll to read the entire method), then it's a good candidate for refactorization. Making sure your methods, parameters and variables all have descriptive names (while not being too verbose either) is one of the trickier aspects of coding.

This is related to another rule of thumb, which is to scan through your method and ask 'Would I be able to understand this at a glance, without having to think through the logic?'. If not, then it's another candidate for refactorization.

In the above example with your if/else problem, there's the option of switch statements or polymorphism as mentioned. Another one for clarity's sake could simply be to break each branch into a method.

Code (CSharp):

1. CharacterAction MakeDecision()
2. {
3.     if (CanSeeEnemy() ) {
4.         return MakeAttackDecision();
5.     }
6.     else {
7.         return MakeIdleDecision();
8.     }
9. }
10.  
11. CharacterAction MakeAttackDecision()
12. {
13.     if (character.hasBow){
14.         return RangedAttack();
15.     }
16.     else {
17.         return MeleeAttack();
18.     }
19. }

etc.

 

Hey MrArcher,

I'm making a note of this. Thank you.

 

Great points, when it comes to polymorphism also check out composition. Difference is that instead of creating you behaviour from the bottom to top in a stack manner, you put it parallell. The good thing with this pattern is that you can basically stick any component (sometimes called strategies) to add in more behaviour something you often cant do in classic inheritance without creating side effects.

Also only comment why not how, comments are noise and make your methods less readable.

A example of a bad comment is

Code (CSharp):

1. var start = current; //Setting the start vector to current position

If anything here you should consider other names for your variables.

A example of a good comment, actual one from our game

Code (CSharp):

1.         public override IEnumerator Execute()
2.         {
3.             if (topItem != null)
4.             {
5.                 ShowPopup(topItem.ColliderParent.transform, GetGrabText());
6.                 while (topItem && !topItem.IsAttached)
7.                 {
8.                     yield return null;
9.                 }
10.                 yield return null; //Give SteamVR a frame to active action set on grabbed item, its a bug in SteamVR 2.2 that does not let you call action.GetLocalizedOriginPart in the same frame after you have activated the action set
11.             }
12.         }

And lastly use var. It removes noise and makes the methods easier to read.
example of bad code,

Code (CSharp):

1. Enemy enemy = GetClosestEnemy();

The method clearly tells us what the variable is. No need to state that again. Also when using generics with which can even have nesterd type arguments its really good to just ommit the type declaration and use var.

I have become so used to vars that I even do var velocity = 1000f; Some would say its error prone, remove the f and you have changed the type into a int. But I do not agree, modern compilers help you there. And thats its a velocity at thousand meters per second is much more important for the domain than that its a float, you should focus on bringing forth your domain not noise.

 

I will never get on board with the var thing, if anything it makes the code much harder to read. At least it does for me.

Plus I don't know what the hell var is, I don't even know if it's a base type, a class, an array so I need to dig in even further to see what GetClosestEnemy() returns in order to know what I can do with the var I just returned.

For me this is the stupidest thing C# has ever introduced and I will never ever use it.

 

I have never met a fellow senior dev that does not agree

 

Fixed that for you. We've got plenty of people here who would qualify as senior devs and many have disagreed.

 

There are senior devs on this forum that disagree.

 

Yeah I meant the forum.

 

Oh, gotcha, I thought you meant like... senior developers in other dev fields.

 

Game Dev is behind in so many areas
Nah, the type declaration is reduntant noise. The domain should be about the domain not declaring variables.

Edit: most here are hobby Devs btw

 

I have had experience with "senior devs" in a number of fields including access to their code. There's a reason you get so much pushback when you say this.

 

Well the problem is its not a protected title. Anyone can call themself senior, most of the time it just mean they are old or have worked a long time. I mean those that are real senior developers.

 

This thread is great. It covers so many concepts and paradigms. I spend far too much time rewriting code because I forget to think ahead.

 

Thinking too much ahead is not good either. It's better to keep a agile code base that can evolve into something beautiful

 

I agree with this but I've also seen it taken to the extreme and it reduces readability and clarity. They follow the "one screen rule" like dogma and end up with dozens of static functions that are called only once and from only within a single function, all in the name of making a function fit on one screen.

Code (CSharp):

1. void foo()
2. {
3.     doThis();
4.  
5.     doThat();
6.  
7.     doThisToo();
8.  
9.     doThatToo();
10.  
11.     alsoDoThis();
12. }
13.  

You could instead have scoped code blocks with comment lines in place of function names.

Code (CSharp):

1. void foo()
2. {
3.     // Do this
4.     {
5.          ...
6.     }
7.  
8.     // Do that
9.     {
10.          ...
11.     }
12.  
13.     // Do this too
14.     {
15.          ...
16.     }
17.  
18.     // Do that too
19.     {
20.          ...
21.     }
22.  
23.     // Also do this
24.     {
25.          ...
26.     }
27. }
28.  

The scoping keeps things visually separated so you can collapse them in your IDE, and also keeps any of their temporary variables inside of them so they don't affect the outer scope of the function.

Then if you later need the same functionality of one of those blocks again, you can break it out into a function.

 

Nice strategy pattern example from @TonyLi , its also my favourite pattern in design, not only it allows minimal code change, but also easy to assign implementation for team, and not to mention its has very high coverage from unit test.

Pattern has been a top-down language in software engineering, which saves us lot of time from code reading once we recognise its form from teammate or our old projects.

 

I also do this sometimes, in addition to what I said earlier.

 

On the topic of keeping code visually separated by hiding it away, there is a preprocessor for this purpose. I recommend checking it out because it's an excellent way to hide away entire sections of your code. To my knowledge it's supported by every IDE that works with Unity including Script Inspector 3 (an IDE that runs within Unity).

https://docs.microsoft.com/en-us/do...e/preprocessor-directives/preprocessor-region
https://assetstore.unity.com/packages/tools/visual-scripting/script-inspector-3-3535

 

Yes, it's just a title. That's why it was the lead developer on the project every time.

 

You mean other than the fact that he's very deceptive in the way he words it? If he was a game developer and was referring to other game developers I would have far less of a problem with it, but he's conveniently neglecting to mention that the "senior developers" in question are in a business-focused field.

Business-focused software development has wildly different practices from game development. It's not that we're behind (and the fact that he can't understand this shows how little value there is in the title of "senior developer"), but rather that our problems are different. We need performance moreso than being agile.

 

Maybe that's it. Most of the senior devs I've worked with outside of games have dealt stuff like enterprise level server solutions and the like.

 

They are not different fields. Game devs would benefit alot from writing clean code.

 

Just dont take polymorphism as a silver bullet. I take my usual example.

Lets say you have a Handgrenade base class, than you have fraggrenade and smokegrenade subclasses of that base class. Fine.

Now you want to throw a C4 into the mix. It has all the explosive attributes of a fraggrenade, but its not a grenade.

 

Type declarations aren't unclean.

 

I do want to reiterate that my point in relation to the original question was not about a specific pattern (polymorphism or whatever) but rather being able to step back when code looks ugly and determine if a different approach can produce a more elegant solution.

 

Yes, but there are ways to write clean code that don't impact other important aspects of game development and there are ways to write clean code that have massive implications on the way your program behaves. While "var" is generally a low impact alteration (reference link below) it's heavily debated as to whether it truly improves readability in most cases.

https://stackoverflow.com/questions/356846/will-using-var-affect-performance/356855#356855

 

They most differently are. More so with generics, but any type declaration is redundant. And if its not then your domain needs refactor

 

The first solution is rarely the best solution anyway.

Step 1: Make it work.
Step 2: Make it clean.

Unfortunately there's rarely time for step 2. But if you attempt to combine the two steps together you often lose the forest for the trees and nothing gets done.

 

List<int> bar1 = Foo(); will not compile

 

not a programmer, but what is all the fuss over "clean" code? This just means that, should you have to return to the code later, or somebody else, it is legible enough that hunting for a needle in the haystack isn't more work than necessary, right? Or does it have something to do with performance?

Like, sometimes if I'm working on some complex textures, the layer stack can get pretty hairy so I organize it with folders and logical naming. Ok, easy enough. Anybody else could probably come through and nitpick my particular method of organization, but, like... so what? Don't they have something better to do? Anybody who will be working with me on the regular would just talk with me to work out a system that works for both of us.

 

In my defense I wasn't paying attention to his code. I was paying attention to the statement about performance.

 

It's partly about maintaining it weeks or years later.

But, more immediately, clean code is easier to understand and verify for correctness. You can look at an image and see if it's right or not. With code, it's difficult to see all the edge cases, especially if the code is a pile of spaghetti full of interdependencies.

But internet discussions about what's clean code often goes off the rails.

 

I wouldn't trust someone who cant even write code that compile
The IL will be exactly the same. Anyone saying there is a performance difference is just not knowing what they are talking about

 

You can think of it like writing an essay.

A good essay has well-named headers that relate to its associated body paragraphs, and each paragraph consists of one main point described in detail.
Simple and easy to read.

A bad essay has no headers and no paragraphs - everything is just a long string of points that vaguely relate to something.
While this essay does describe a topic, nobody really wants to read a giant wall of text.

 

Stupid smart phone keyboard, I ment to write most definitely

 

Read the 'Code Complete' textbook. it's an oldie but a goodie.

 

Game developers do benefit from wiring clean code.

 

Mostly this.

Clean code is all about reducing the cognitive load on the developer. In general terms, the less the developer has to think about a particular line of code, data structure or method, the better. This frees up more space in the programmers brain to think about useful stuff. This is true both the first time you write code, and when you come back days, weeks or years later to the same code.

This specific debate of var vs non var is one of the more annoying recurrent debates on the forum that occurs whenever we have a discussion about code architecture. Its a relatively insignificant issue, the modern equivalent of debating how many angels can fit on the head of a pin. Or in art terms whether purple or violet is objectively the best color. Like the backwards American windscreen wiper controls, it only takes a few minutes to switch between the different styles.

The forums would be a better place if we simply banned discussion of the topic. Noone has brought up any new arguments or changed anyone's mind in at least three years.

 

Relax, don't get me wrong, pattern is not only about polymorphism.

From your example, we can have factory + strategy patterns together for explosive attributes. When C4 explosive, it calls the factory of explosive effects to display different explosive attributes from strategy. If attributes are additive, we can use decorator pattern for adding each attributes with unlimited combination, very handy.

When designing a system, one thing worth to mention is how objects lifetime in system, they could also have varies lifetime too, kind of questions could ask ourself on how they behave, and apply suitable patterns to our design.

Edit:
Hmm..here is a list on benefits and drawbacks from patterns, good for exam.

 

So it's a mystery so many don't

 

Yeah, and with unity it translates for example into slapping a explosive component on a frag/c4 and a smoke emitter on a smoke grenade and call GetComponent<IExplosive>() doesn't need to be harder than that.

Only big question here is what to name the interface since a smoke emitter isn't explosive we don't have smoke grenades yet, need to find a better name for it when we do

 

Yup, with Unity, just let patterns work for our game logic, and let prefab instances be the product from our factory.

For smoke emitter, since we already have an abstract effect factory, just subclass smoke to effect. With decorator pattern, you could also mix different effects together. It also make unit test quite effortless, since complex conditional statements are no longer exist inside method.

By the way, I am not against anyone using var in their implementation, but definitely not my style.

I come in peace to contribute something I know for hobby devs who interested in design, I am also learning new things from others everyday, cheers.

 

There are more important cases for clean code than var or not var. But all little things add up, and var surly helps make the code more clean