Search Unity

Keeping methods readable and concise

Discussion in 'General Discussion' started by godfreyidk, Feb 18, 2019.

  1. godfreyidk

    godfreyidk

    Joined:
    Feb 4, 2018
    Posts:
    14
    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?
     
  2. Joe-Censored

    Joe-Censored

    Joined:
    Mar 26, 2013
    Posts:
    6,906
    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
     
  3. TonyLi

    TonyLi

    Joined:
    Apr 10, 2012
    Posts:
    9,879
    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.
     
  4. godfreyidk

    godfreyidk

    Joined:
    Feb 4, 2018
    Posts:
    14
    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)?
     
  5. TonyLi

    TonyLi

    Joined:
    Apr 10, 2012
    Posts:
    9,879
    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? :)
     
    wccrawford, xVergilx, Kiwasi and 3 others like this.
  6. godfreyidk

    godfreyidk

    Joined:
    Feb 4, 2018
    Posts:
    14
    Yep, spot on.

    Cheers.
     
  7. MrArcher

    MrArcher

    Joined:
    Feb 27, 2014
    Posts:
    78
    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.
     
  8. godfreyidk

    godfreyidk

    Joined:
    Feb 4, 2018
    Posts:
    14
    Hey MrArcher,

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

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    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.
     
    Last edited: Feb 20, 2019
    godfreyidk likes this.
  10. ensiferum888

    ensiferum888

    Joined:
    May 11, 2013
    Posts:
    265
    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.
     
  11. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    I have never met a fellow senior dev that does not agree
     
    Socrates likes this.
  12. Ryiah

    Ryiah

    Joined:
    Oct 11, 2012
    Posts:
    14,907
    Fixed that for you. We've got plenty of people here who would qualify as senior devs and many have disagreed. :p
     
  13. Murgilod

    Murgilod

    Joined:
    Nov 12, 2013
    Posts:
    6,348
    There are senior devs on this forum that disagree.
     
    ShilohGames, Kiwasi and angrypenguin like this.
  14. Ryiah

    Ryiah

    Joined:
    Oct 11, 2012
    Posts:
    14,907
    Yeah I meant the forum.
     
    angrypenguin likes this.
  15. Murgilod

    Murgilod

    Joined:
    Nov 12, 2013
    Posts:
    6,348
    Oh, gotcha, I thought you meant like... senior developers in other dev fields.
     
    angrypenguin likes this.
  16. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    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
     
  17. Murgilod

    Murgilod

    Joined:
    Nov 12, 2013
    Posts:
    6,348
    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.
     
    angrypenguin likes this.
  18. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    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.
     
  19. Vryken

    Vryken

    Joined:
    Jan 23, 2018
    Posts:
    793
    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.
     
  20. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    Thinking too much ahead is not good either. It's better to keep a agile code base that can evolve into something beautiful
     
  21. vertexmachina

    vertexmachina

    Joined:
    Feb 12, 2019
    Posts:
    9
    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.
     
    Socrates, Kiwasi, Vryken and 2 others like this.
  22. Lu4e

    Lu4e

    Joined:
    Mar 23, 2018
    Posts:
    277
    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.
     
  23. Joe-Censored

    Joe-Censored

    Joined:
    Mar 26, 2013
    Posts:
    6,906
    I also do this sometimes, in addition to what I said earlier.
     
  24. Ryiah

    Ryiah

    Joined:
    Oct 11, 2012
    Posts:
    14,907
    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
     
    Kiwasi likes this.
  25. Murgilod

    Murgilod

    Joined:
    Nov 12, 2013
    Posts:
    6,348
    Yes, it's just a title. That's why it was the lead developer on the project every time.
     
  26. Ryiah

    Ryiah

    Joined:
    Oct 11, 2012
    Posts:
    14,907
    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.
     
  27. Murgilod

    Murgilod

    Joined:
    Nov 12, 2013
    Posts:
    6,348
    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.
     
  28. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    They are not different fields. Game devs would benefit alot from writing clean code.
     
  29. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    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.
     
  30. Murgilod

    Murgilod

    Joined:
    Nov 12, 2013
    Posts:
    6,348
    Type declarations aren't unclean.
     
    ShilohGames and angrypenguin like this.
  31. TonyLi

    TonyLi

    Joined:
    Apr 10, 2012
    Posts:
    9,879
    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.
     
    Lurking-Ninja, Socrates, Lu4e and 2 others like this.
  32. Ryiah

    Ryiah

    Joined:
    Oct 11, 2012
    Posts:
    14,907
    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
     
  33. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    They most differently are. More so with generics, but any type declaration is redundant. And if its not then your domain needs refactor
     
  34. vertexmachina

    vertexmachina

    Joined:
    Feb 12, 2019
    Posts:
    9
    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.
     
  35. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
  36. BIGTIMEMASTER

    BIGTIMEMASTER

    Joined:
    Jun 1, 2017
    Posts:
    2,830
    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.
     
  37. Ryiah

    Ryiah

    Joined:
    Oct 11, 2012
    Posts:
    14,907
    In my defense I wasn't paying attention to his code. I was paying attention to the statement about performance. :p
     
  38. TonyLi

    TonyLi

    Joined:
    Apr 10, 2012
    Posts:
    9,879
    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. :)
     
  39. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    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
     
  40. Vryken

    Vryken

    Joined:
    Jan 23, 2018
    Posts:
    793
    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.
     
    BIGTIMEMASTER likes this.
  41. Murgilod

    Murgilod

    Joined:
    Nov 12, 2013
    Posts:
    6,348
     
    angrypenguin and Ryiah like this.
  42. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    Stupid smart phone keyboard, I ment to write most definitely
     
  43. Antony-Blackett

    Antony-Blackett

    Joined:
    Feb 15, 2011
    Posts:
    1,285
    Read the 'Code Complete' textbook. it's an oldie but a goodie.
     
  44. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    12,225
    Game developers do benefit from wiring clean code.
     
    Kiwasi likes this.
  45. Kiwasi

    Kiwasi

    Joined:
    Dec 5, 2013
    Posts:
    16,572
    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.
     
  46. Lu4e

    Lu4e

    Joined:
    Mar 23, 2018
    Posts:
    277
    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.
     
    Last edited: Feb 21, 2019
  47. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    So it's a mystery so many don't
     
  48. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    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
     
  49. Lu4e

    Lu4e

    Joined:
    Mar 23, 2018
    Posts:
    277
    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.
     
  50. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,406
    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