Jump to content
  • Advertisement
kostile

C# What does good code look like?

Recommended Posts

I really don't know what is considered better or even good for the most part. My code varies and has evolved over the years as a developer and sometimes I think it looks cleaner, but it is never easy to edit or add something. I fight with myself when writing things like particle engines because I know the purpose of a particle engine is to be quick and efficient, but how much overhead is there really just by giving a reference of an emitter to a particle that is contained within it? Why should I write behaviors for particles as functions that return a value vs just writing a function that returns nothing but handles all of the behavior? I feel like with the latter I end up with code that is easier to modify, but it looks terrible. With the previous I end up with very clean code but is god awful to edit as I have one iteration loop that basically controls everything based on the function it is given.

 

Here is some more recent code I have written that I think would be considered "good" but I am really not sure. The gist of this code is that it will spawn an enemy in a pattern that right now is determined by the writer. I will eventually write functions that determine which shape should be spawned when and where, but right now it just will spawn a shape from the SpawnPolyGroup function with 5 enemies per line and 6 lines in the shape. So it makes a hexagon with 5 enemies per line. My question is, is it better to have the SpawnGroup function control the for loop and return values from each SpawnPatterns (being Vector2 as locations), or would it be better to have each function in the SpawnPatterns class have it's own iterative loop? Also, any other tips on my code for cleanliness and professionalism would be great. I am self taught and never had a tutor, but have read a lot about people being self taught writing code that is unprofessional, but I have never reached out for someone to look at my code aside from a few people so I am hoping that I can learn something to be more professional in the future in hopes to find a paying job as a programmer.

   public static class Spawner
    {
        public static bool isSpawning = false;
        public static float spawnTimer = 0;
        public static bool groupSpawning = false;

        public static Dictionary<Enemy, float> delayedSpawnDict = new Dictionary<Enemy, float>();
        public static float timeDelta = 0;

        //Arguments for currentPattern should be (int maxEnemies, int currentIteration, int radius, Vector2 PatternLocation)
        public static Func<int, int, Vector2, Vector2, Vector2> currentPattern = SpawnPatterns.Line;

        public static void Update(GameTime gameTime)
        {
            if (!isSpawning)
            { return; }

            if (timeDelta > 0)
            {
                timeDelta -= gameTime.ElapsedGameTime.Milliseconds;
            }

            if (timeDelta <= 0 && delayedSpawnDict.Count() > 0)
            {
                KeyValuePair<Enemy, float> pair = delayedSpawnDict.ElementAt(0);
                Enemies.enemies.Add(pair.Key);
                pair.Key.Spawn();
                timeDelta = pair.Value;
                delayedSpawnDict.Remove(pair.Key);
            }


            //If enemies alive is greater than max enemies then do not spawn
            if (Enemies.enemies.Count + delayedSpawnDict.Count() >= 1)
            { return; }

            //Spawn Individual
            if (spawnTimer >= 500)
            {
                SpawnPolyGroup(gameTime, new Vector2(800, 0), 5, 6);
                spawnTimer = 0;
            }
            spawnTimer += gameTime.ElapsedGameTime.Milliseconds;
        }

        public static void SpawnGroup(GameTime gameTime, Vector2 location, Vector2 mod1, int amount = 5, float delay = 100)
        {
            for (int i = 0; i < amount; i++)
            {
                Vector2 enemyLocation = currentPattern(amount, i, mod1, location);

                if (enemyLocation == default(Vector2))
                {
                    return;
                }

                Enemy enemy = new Enemy(15, Assets.hollowCircle, enemyLocation, EnemyUtilities.Follow, 500);

                if (delay > 0)
                {
                    delayedSpawnDict.Add(enemy, delay);
                }
                else
                {
                    Enemies.enemies.Add(enemy);
                }

            }
        }

        public static void SpawnPolyGroup(GameTime gameTime, Vector2 location, int amount = 5, int sides = 5)
        {
            float radians = AOneMath.AngleToRadian(360 / sides);

            float rotation = AOneMath.AngleToRadian(90 * (sides % 2));

            for (int i = 0; i < sides; i ++)
            {
                int distanceFromCenter = 200;

                float x1 = location.X + (float)Math.Cos(radians * i + rotation) * distanceFromCenter;
                float y1 = location.Y + (float)Math.Sin(radians * i + rotation) * distanceFromCenter;
                Vector2 p1 = new Vector2(x1, y1);

                float x2 = location.X + (float)Math.Cos(radians * (i + 1) + rotation) * distanceFromCenter;
                float y2 = location.Y + (float)Math.Sin(radians * (i + 1) + rotation) * distanceFromCenter;
                Vector2 p2 = new Vector2(x2, y2);

                SpawnGroup(gameTime, p2, p1, amount);
            }
        }
    }

    public static class SpawnPatterns
    {

        /// <param name="radius">Radius, Best to have X and Y the same</param>
        /// <param name="patternLocation">Center location of circle</param>
        /// <returns>Enemy location based on current iteration of max from radius to the patternLocation as the center</returns>
        public static Vector2 Circle(int max, int current, Vector2 radius, Vector2 patternLocation)
        {
            float radians = AOneMath.AngleToRadian(360/max);

            float x = (float)Math.Cos(radians * current);
            float y = (float)Math.Sin(radians * current);

            return new Vector2(patternLocation.X + (x * radius.X), patternLocation.Y + (y * radius.Y));
        }

        /// <param name="pointB">The end point where the last enemy should be in the line</param>
        /// <param name="pointA">The start point where the first enemy should be in the line</param>
        /// <returns>Enemy location based on current iteration of max from pointA to pointB</returns>
        public static Vector2 Line(int max, int current, Vector2 pointA, Vector2 pointB)
        {
            float x = MathHelper.Lerp(pointA.X, pointB.X, (float)current / (float)max);
            float y = MathHelper.Lerp(pointA.Y, pointB.Y, (float)current / (float)max);

            return new Vector2(x, y);
        }


        /// <param name="p1">Top left point of area to spawn randomly</param>
        /// <param name="p2">Bottom right point of area to spawn randomly</param>
        public static Vector2 Random(int max, int current, Vector2 p1, Vector2 p2)
        {
            float x = AOneMath.random.Next((int)p1.X, (int)p2.X);
            float y = AOneMath.random.Next((int)p1.Y, (int)p2.Y);

            return new Vector2(x, y);
        }
    }

 

Share this post


Link to post
Share on other sites
Advertisement

The Spawner class probably shouldn't be static - it would make it difficult to deal with if you ever needed two separate spawners, so you might as well change it and get the pain out of the way now.

SpawnPatterns seems OK as a static class for now.

The MSDN recommendations for public variables in C# are to use PascalCase (capitalized first letter), and private variables camelCase (lowercase first letter).  Though these are coding conventions and you're free to choose whatever you want.

That Func is complicated enough that you might want to define your own delegate type for it - this lets you give each parameter a name which will be helpful for remembering what each parameter does.

For cumbersome types like the KeyValuePair<Enemy, float> you could use 'var' instead to shorten it.

The {return;} all on one line looks weird.  Just take the braces off and indent the 'return;', or put the { } on separate lines.  This is another coding convention choice to make.

Overall your code looks pretty good to me.

Edited by Nypyren

Share this post


Link to post
Share on other sites

Thanks for the critique, I wasn't aware of the PascalCase for public variables convention but I am trying to follow the "standard" convention so that is good to know. I like the recommendations and will do this and continue to do this in the future. I'm am really glad to hear that this isn't terrible because like I said, it is more recent code that I think looks much better than most of my previous. 

Share this post


Link to post
Share on other sites

Personally, I don't like your setup with functions having parameters they either don't use or need ("max" + "current" in the Random function), or which go against what the function is called and contains unnecessary gotcha's (In "Circle", "radius" is a Vector2 which can contain different values for x & y, which would produce something other than a circle).

Additionally, your "Line" function seems to be incorrect. If you have max = 2 (which would give current = 0 and current = 1), you will never return the end-point (which your comment states that it should).

A similar issue can be found in "Circle".

Share this post


Link to post
Share on other sites

Good code:

  • does what it's meant to (i.e. meets the product specification)
  • does what it claims to (i.e. has accurately descriptive names and comments)
  • is easy to understand
  • is easy to change and extend

And really these are 4 overlapping aspects rather than 4 separate ones.

With those thoughts in mind, here's how I would approach your code:

  • ditch all the 'static' stuff. That implies there is, and can only ever be, one instance of those things. That is not "easy to change and extend". There's a lot of documentation about why singletons and globals are bad, and static variables are part of the same family.
  • remove unused arguments like 'GameTime'. The presence of the argument implies that the function does something with the current time, but it does not. This makes it harder to understand.
  • Make sure your comments match your code. A comment says "//Spawn Individual" but the code says "SpawnPolyGroup". Your code should do what it claims to, and here it doesn't.
  • Add comments to your functions. You don't want to have to read the whole function to know what it does, and the function names are rarely descriptive enough. You want this code to be easy to understand months after you've written it.
  • Rename SpawnPatterns to something more mathematical - there's nothing intrinsic to spawning in those functions, just mathematics. This means your code isn't really doing what it claims (you think you're getting spawn patterns, but actually you just get basic geometry) and it's harder to extend (because if you find yourself needing basic geometry in future, you have to remember that you have some hidden away inside a SpawnPatterns class).
  • Be consistent. You have a timeDelta which ticks downwards, and a spawnTimer which ticks upwards. It's not immediately clear why they differ and it's certainly not necessary to have them counting in different directions. As such, it's error-prone (as you might change the code and forget which one counts up and which one counts down) and hard to understand (what do these 2 different timers mean?) and not so easy to extend (could you perhaps have a standard class for timers like this?)

Share this post


Link to post
Share on other sites

I see what you are saying about having params that aren't used but what would be a clean solution to that? The idea was to have the generic Func that i just call through the SpawnGroup function and i can easily change which function is called, either random, line, or circle, but this does end up having functions that use the paremeters differently as well as not using some at all. 

Share this post


Link to post
Share on other sites

If you truly want a set of generic parameters then you might consider wrapping them in a struct, passing that to each of these functions instead, and providing comments/naming that makes it clear what you are doing, and why.

Share this post


Link to post
Share on other sites

 

6 hours ago, Kylotan said:

Good code:

  • does what it's meant to (i.e. meets the product specification)
  • does what it claims to (i.e. has accurately descriptive names and comments)
  • is easy to understand
  • is easy to change and extend

And really these are 4 overlapping aspects rather than 4 separate ones.

With those thoughts in mind, here's how I would approach your code:

  • ditch all the 'static' stuff. That implies there is, and can only ever be, one instance of those things. That is not "easy to change and extend". There's a lot of documentation about why singletons and globals are bad, and static variables are part of the same family.
  • remove unused arguments like 'GameTime'. The presence of the argument implies that the function does something with the current time, but it does not. This makes it harder to understand.
  • Make sure your comments match your code. A comment says "//Spawn Individual" but the code says "SpawnPolyGroup". Your code should do what it claims to, and here it doesn't.
  • Add comments to your functions. You don't want to have to read the whole function to know what it does, and the function names are rarely descriptive enough. You want this code to be easy to understand months after you've written it.
  • Rename SpawnPatterns to something more mathematical - there's nothing intrinsic to spawning in those functions, just mathematics. This means your code isn't really doing what it claims (you think you're getting spawn patterns, but actually you just get basic geometry) and it's harder to extend (because if you find yourself needing basic geometry in future, you have to remember that you have some hidden away inside a SpawnPatterns class).
  • Be consistent. You have a timeDelta which ticks downwards, and a spawnTimer which ticks upwards. It's not immediately clear why they differ and it's certainly not necessary to have them counting in different directions. As such, it's error-prone (as you might change the code and forget which one counts up and which one counts down) and hard to understand (what do these 2 different timers mean?) and not so easy to extend (could you perhaps have a standard class for timers like this?)

Your explanation of good code outlines it really well for me. I will have to revisit a lot of code with these ideas in mind. I'm well on my way of practicing good habits and i appreciate the feedback

Share this post


Link to post
Share on other sites

Do all those members really need to be public?  I'm a big believer in only exposing to the consumer of your code the bare minimum, and having them modify things through methods instead.  For example, what if in a later implementation, you find that you want to reset the spawnTimer when someone disables the spawner.  In your current implementation, that would be a painful retrofit, as you'd have to track down all the places where people are mucking with the internals of your class.  

Other things just seem unnecessary to expose, like timeDelta.  Why is that public?

And finally, whats up with all the hard coded numbers?   500?  800?  Without even a comment as to what they are.  Those should be named constants, or exposed so that users of the Spawner can set them to what they want.

Share this post


Link to post
Share on other sites
36 minutes ago, Kylotan said:

If you truly want a set of generic parameters then you might consider wrapping them in a struct, passing that to each of these functions instead, and providing comments/naming that makes it clear what you are doing, and why.

I will have to look into that. I have never tried to use structs for filling parameters. You wouldnt have an example or be able to elaborate a little? This is a totally new idea to me, and i have never thought about using structs for anything really.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

  • Advertisement
  • Advertisement
  • Popular Tags

  • Similar Content

    • By JoshuaFraser
      Hi and thanks for reading, I have an issue with this reactive crosshair script, everything works fine until I start changing the offset. Give the script a go and you will see what I mean, when I do SetOffset(0f); it doesnt always set back to the origional state, if anyone can spot a fix I'd be super appreciative!
      using System.Collections; using System.Collections.Generic; using UnityEngine; public class ReactiveCrosshair : MonoBehaviour { [SerializeField] GameObject c_limb_prefab; private float center_offset = 0f; private float current_offset = 0f; private float max_offset = .5f; private int number_of_limbs = 4; private float limb_length = .05f; private float limb_width = .005f; private List<GameObject> c_limbs = new List<GameObject>(); public void SetupCrosshair(){ for (int i = 0; i < number_of_limbs; i++) { GameObject line_go = (GameObject)Instantiate (c_limb_prefab); line_go.transform.SetParent (this.transform); Vector3 limb_pos = new Vector3 (0f,0f,0f); //line_go.transform.position = limb_pos; line_go.transform.localPosition = limb_pos; LineRenderer line = line_go.GetComponent<LineRenderer>(); line.startWidth = limb_width; line.positionCount = 2; line.SetPosition (0, line_go.transform.localPosition + new Vector3(center_offset, 0f, 0f)); line.SetPosition (1, line_go.transform.localPosition + new Vector3(center_offset + limb_length, 0f, 0f)); line.useWorldSpace = false; c_limbs.Add(line_go.gameObject); } if (c_limbs != null) { OrientLimbs (); SetOffset (0f); } } public void OrientLimbs(){ for (int i = 0; i < c_limbs.Count; i++) { float rotation_step = 360f / (float)c_limbs.Count; c_limbs [i].transform.RotateAround (c_limbs[i].transform.position, c_limbs[i].transform.forward, 90f + (rotation_step * (float)i)); } } public void SetOffset(float _current_spread){ float offset = Mathf.Lerp (0f, max_offset, _current_spread); for (int i = 0; i < number_of_limbs; i++) { if (offset > current_offset) { Vector3 pos = c_limbs [i].transform.position + (c_limbs [i].transform.TransformDirection (Vector3.right) * offset); c_limbs [i].transform.position = pos; } if (offset < current_offset) { Vector3 pos = c_limbs [i].transform.position - (c_limbs [i].transform.TransformDirection (Vector3.right) * offset); c_limbs [i].transform.position = pos; } } Debug.Log ("SetOffset() offset: " + offset.ToString () + " _current_spread: " + _current_spread.ToString() + " localPos: " + c_limbs[1].transform.localPosition); current_offset = offset; } }  
    • By Woody Stevens
      Hi,
       
      I am looking for a TCP or HTTP networking library similar to Lidgren (UDP).
       
      This is primarily for sending game map data and potentially other large messages from Server to Client.
       
      I do want to keep Lidgren for my chat messages, player position, small fast updates etc. I especially love the flow of data and the library usage in general, so any libraries of a similar style would be excellent. Preferably something open source, free and reliable.
      I also must be able to swap between localhost and an ip address with ease, like Lidgren, as I run a server for singleplayer/mp/lan.
       
      My game maps are similar to minecraft, but it is 2d and only one Z-level, so i'm sending a jagged array of Tile object data (currently only enum TileID.Grass) down the pipe to the Client. Problem is if i'm sending a large map 1024 x 1024 tiles down the to client that's quite a lot of data, and Lidgren is relatively slow to build the writes (before the message is even sent!). It is fine when i'm using smaller maps < 512 x 512 ( xTiles * yTiles ).

      I know about chunking and will look into implementing this later, whilst taking into account the user's position in the world to only send nearby chunks.
       
      An example of my code that can be slow:
      private void WriteWorld(NetOutgoingMessage outgoing) { try { var world = WorldManager.Instance.CurrentWorld; outgoing.Write(world.XTiles); outgoing.Write(world.YTiles); for (int x = 0; x < world.XTiles; x++) { for (int y = 0; y < world.YTiles; y++) { // Write Tile obj data outgoing.Write((int)world.Tiles[x][y]); // <-------- Slow here when xTiles and yTiles are each > 512 ! } } } catch (Exception ex) { // log send error } }  
      I'd love to hear from you guys, especially if any of you have come across a similar challenge.
    • By ethancodes
      I'm working on a system for my game that will allow the player to stack pick ups in a queue. As one pick up expires, the next automatically activates. I'm having an issue though where if I pick up the first one, it activates fine, but if i pick up a second directly after it, it overrides the first one, activates the second one, and then once it has run it's course, everything goes back to normal gameplay, no first pick up. I'm not sure why this is happening. Hopefully someone can spot what I'm doing wrong in my code.
      Here is the code for the pick up manager:
      // Update is called once per frame void Update () { if (pickUpQueue.Count != 0 && !pickUpActive) { pickUpActive = true; pickUpQueue[0].ActivatePickUp(); } DeactivatePickUp(); } void DeactivatePickUp () { if (pickUpQueue.Count != 0 && pickUpActive) { Destroy (pickUpQueue [0]); pickUpQueue.RemoveAt (0); pickUpActive = false; } } And here is the PickUp:
      public override void ActivatePickUp () { ball.GetComponent<Ball>().Speed = 2.0f; //increase ball speed... ball.GetComponent<Ball>().StartCoroutine(timer); //...set time that power up is active }  
      There is also a Base Pick Up:
      public void OnCollisionEnter2D (Collision2D collision) { Vector2 tweak = new Vector2 (Random.Range(0f, 0.2f),Random.Range(0f, 0.2f)); this.gameObject.GetComponent<Rigidbody2D>().velocity += tweak; //if the pickup makes contact with the paddle or ball.... if (collision.gameObject.tag == "Paddle" || collision.gameObject.tag == "Ball") { GameObject.FindObjectOfType<GameManager>().GetComponent<PickUpManager>().pickUpQueue.Add(this); Destroy(gameObject); //...and finally destroy power up object } } As a side note, I am trying to find a solution to this that will work for all of my pickups. Some pickups are ammo based, some are timed. 
    • By Hellados
      Hello guys, my name is Giorgi and i'm newbie game developer i'm learning Pixel art and after pixel art  i want learn C# and don't know how and where start i'm bad with programming language and know only HTML/CSS
    • By D34DPOOL
      Edit Your Profile D34DPOOL 0 Threads 0 Updates 0 Messages Network Mod DB GameFront Sign Out Add jobEdit jobDeleteC# Programmer for a Unity FPS at Anywhere   Programmers located Anywhere.
      Posted by D34DPOOL on May 20th, 2018
      Hello, my name is Mason, and I've been working on a Quake style arena shooter about destroying boxes on and off for about a year now. I have a proof of concept with all of the basic features, but as an artist with little programming skill I've reached the end of my abilities as a programmer haha. I need someone to help fix bugs, optomize code, and to implent new features into the game. As a programmer you will have creative freedom to suggest new features and modes to add into the game if you choose to, I'm usually very open to suggestions :).
      What is required:
      Skill using C#
      Experience with Unity
      Experience using UNET (since it is a multiplayer game), or the effort and ability to learn it
      Compensation:
      Since the game currently has no funding, we can split whatever revenue the game makes in the future. However if you would perfer I can create 2D and/or 3D assets for whatever you need in return for your time and work.
      It's a very open and chill enviornment, where you'll have relative creative freedom. I hope you are interested in joining the team, and have a good day!
       
      To apply email me at mangemason@yahoo.com
    • By Andrew Parkes
      I am a talented 2D/3D artist with 3 years animation working experience and a Degree in Illustration and Animation. I have won a world-wide art competition hosted by SFX magazine and am looking to develop a survival game. I have some knowledge of C sharp and have notes for a survival based game with flexible storyline and PVP. Looking for developers to team up with. I can create models, animations and artwork and I have beginner knowledge of C sharp with Unity. The idea is Inventory menu based gameplay and is inspired by games like DAYZ.
      Here is some early sci-fi concept art to give you an idea of the work level. Hope to work with like minded people and create something special. email me andrewparkesanim@gmail.com.
      Developers who share the same passion please contact me, or if you have a similar project and want me to join your team email me. 
      Many thanks, Andrew.

    • By mike44
      The reference assemblies for framework ".NETFramework,Version=v3.5" were not found. To resolve this, install the SDK or Targeting Pack for this framework version or retarget your application to a version of the framework for which you have the SDK or Targeting Pack installed. Note that assemblies will be resolved from the Global Assembly Cache (GAC) and will be used in place of reference assemblies. Therefore your assembly may not be correctly targeted for the framework you intend.

      Hi
      what to do with the above error in ms code/Unity3d Project on Ubuntu 18.04? I've installed it like:
      https://www.microsoft.com/net/learn/...ux/ubuntu18-04
      Many thanks
    • By Dave Haylett
      Hi.
      I have pulled in five NuGet packages for my Visual Studio 2017 project, however when I build the project, VS spits out 10 .DLL and .XML files in the root of the binary folder, to do with the packages. Can't I shove them into a \packages folder so the user doesn't see these ugly resources next to the .exe file?
      I've Googled moving the packages but the only responses seem to be around moving the installation folder of the NuGet packages on the local machine, as opposed to where VS builds them to.
    • By MoreLion
      hey all! We are looking for members for our Unity horror game! 
      Here’s the story:
      After a deadly virus plunges the world into chaos killing 85% of the human population there are now what they call “zones” these zones are watched very closely by the surviving government, people are checked every day for the virus, even if you touch the spit or any human waste or fluids of the victim who is infected, you will die. But one day, people in the west zone start to go missing, 1 woman goes outside the walls to uncover the mystery, is there more to the virus than meets the eye?, That is where your story starts.
      This game is not a long development game, I have loads other game ideas,
      I will also allow you to have a bit of creative freedom if you wish to add or share a idea!
      And no, it’s not a zombie game lol I feel like zombie games are too generic, in this game you will encounter terrifying beasts!
      There is some concept art one of our concept artists have made
      If interested email liondude12@gmail.com
    • By Victor Rodriguez
      Hi there! Is the first time that I'm posting here so I'm sorry if I'm doing it wrong ha. 
      So here it comes, my doubt is, I'm doing a game with different levels, each of these levels in one different scene. Each scene contains to cameras that you can change pressing a button. Everything works fine. 
      The only problem is that I would like it to look a bit more professional, and I would like that if you finish the level with camera2, the next level start the same way. I've been thinking about using dontdestroyonloadon both cameras, but obviously this cameras need to be attached to the player to make the movement work, what do you recommend? Sorry If I've explained it in a messy way, and feel free to dm me for anything. Thanks in advance! 
  • Advertisement
  • Popular Now

  • Forum Statistics

    • Total Topics
      631394
    • Total Posts
      2999755
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!