Possible critique of code

7fb33cd08426441a703be8e6012e58f0
0
Jason5755 101 Apr 03, 2013 at 21:21

I have taken a couple programming classes, but for the most part I am self taught. So after a lot of studying and working several example studies I started to come up projects of my own and seeing if I figure them out. I don’t really have anyone that I can bounce things off of and thought I would try using this forum.

The following code is a C# class for a random weapon generator that I’ve been working on. Its far from complete, but I wanted to get some feedback from some more experienced programmers to see if the parts that are look good or not. I realize that there are many ways to accomplish a task when it comes to programming, but some are obviously inefficient and I want to try avoiding these bad habits. Besides, Its good to see different perspectives that can help expand my knowledge.

public class RandomWeapon // This class is used to create a random weapon
{
public string weaponName;    // Weapon name
public string weaponType;    // Weapon type
public string weaponRarity;   // Weapon rarity
public int weaponLvl;        // Weapon lvl


public RandomWeapon(int npcLvl, string npcRarity,int playerLuck) // RandomWeapon constructor
{
weaponRarity = WeaponRarity(npcRarity,playerLuck);
weaponLvl = WeaponLvl(npcLvl);
}
/*public string WeaponType()
{
}*/
public string WeaponRarity(string npcRarity, int playerLuck)
{
  double rand1 = 0.0;
  double npcRarityMultiplier = 0.0;
  double playerLuckModifer = playerLuck * .001;
  string rarity = "";
  string[] weaponRarityArray = {"Common", "Enchanted", "Exceptional", "Fabled", "Cosmic"};
  string[] npcRarityArray = {"Normal","Elite","Special","Boss"};
  double[] rarityBaseProbabilty = {.01,.03,.10,.35,1.00};
  double[] npcRarityMultiplierArray = {1.0,1.5,1.75,2.0};
  Random random = new Random();
  rand1 = random.NextDouble();

  Console.WriteLine(rand1);


  if (npcRarity == npcRarityArray[0])
   npcRarityMultiplier = npcRarityMultiplierArray[0];
  else if (npcRarity == npcRarityArray[1])
   npcRarityMultiplier = npcRarityMultiplierArray[1];
  else if (npcRarity == npcRarityArray[2])
   npcRarityMultiplier = npcRarityMultiplierArray[2];
  else if (npcRarity == npcRarityArray[3])
   npcRarityMultiplier = npcRarityMultiplierArray[3];
  
  Console.WriteLine(npcRarityMultiplier);
  

  if (rand1 < (rarityBaseProbabilty[0] + playerLuckModifer) * npcRarityMultiplier)
   rarity = weaponRarityArray[4];
  else if (rand1 < (rarityBaseProbabilty[1] + playerLuckModifer) * npcRarityMultiplier)
   rarity = weaponRarityArray[3];
  else if (rand1 < (rarityBaseProbabilty[2] + playerLuckModifer) * npcRarityMultiplier)
   rarity = weaponRarityArray[2];
  else if (rand1 < (rarityBaseProbabilty[3] + playerLuckModifer) * npcRarityMultiplier)
   rarity = weaponRarityArray[1];
  else if (rand1 < (rarityBaseProbabilty[4] + playerLuckModifer) * npcRarityMultiplier)
   rarity = weaponRarityArray[0];

  return rarity;
}
public int WeaponLvl(int npcLvl)
{
  int lvl = 0;
  Random rand1 = new Random();
  lvl = rand1.Next(npcLvl - 5, npcLvl + 5);
  return lvl;
}
}

void Main()
{
RandomWeapon randomWeapon = new RandomWeapon(20,"Boss",40);
randomWeapon.weaponRarity.Dump();
randomWeapon.weaponLvl.Dump();
}

7 Replies

Please log in or register to post a reply.

A8433b04cb41dd57113740b779f61acb
0
Reedbeta 167 Apr 03, 2013 at 21:50

I’d use enums rather than strings for things like the weapon rarities or NPC rarities where you have a finite, named list of things. Internally it’s an integer, so it’s cheaper to compare and manipulate enums than strings. It also means the compiler will check for misspellings, etc. And you could avoid the if-else chain checking the npcRarity, since it would be an integer; you could just do npcRarityMultiplierArray[npcRarity].

The random-choice algorithm seems fine, although you should use a loop rather than writing an if-else chain here. Also, it was confusing to me that the rarityBaseProbability array is written in the opposite order to all the others. You should be able to reverse it and take 1.0 minus each value, so you’d get { 0.65, 0.9, 0.97, 0.99, 1.0 }, and that should let you use the same logic but keep all the arrays in the same order.

A638aa42130293f319eda7fa4ba121f4
0
fireside 141 Apr 03, 2013 at 21:54

Generally, on something like this, the variables would be private and you would use a “get” function for them in order to keep from accidentally changing them in your code. For a small game, it would be unnecessary. Putting the arrays in a function gets rid of them immediately, which is good for a small array. Otherwise, you should use constants and statics so you don’t get multiples, but have them on hand. The other nice thing about keeping them in a more permanent form is that you may need them for an unforeseen reason later and you will only need to add a simpler function.

B5262118b588a5a420230bfbef4a2cdf
0
Stainless 151 Apr 04, 2013 at 09:05

Using strings is an awful idea, just think about what the code is actually doing. A string compare is slow. Also a string needs a lot more memory than an int

Reedbeta has already pointed out that, but I don’t think you really have it in your head what your code does. Lets take one line of code and give you a breakdown of what it does

if (npcRarity == npcRarityArray[[/color][color=#006666]0[/color][color=#666600]])

What this has to do is …

  get sizeof array element
  multiply 0 by sizeof array element
  add base of array
  get pointer to array element
  get size of input string
  foreach char in input string
  compare char with equivalent char in array element
  if match, continue loop
  else return false

Since you are doing a set of if else statements, it will do this for every array element until it finds a match

If you had something like this

public enum NpcRarity
{
   Normal,
   Elite,
   Rare,
   Boss
};

then you can use a switch statement

    switch (npcrarity)
   {
      case NpcRarity.Normal:
          multiplier = 0.3;
          break;

Then what that ends up as after you have compiled it is

   index = rarity - base;
   index << 2;
   jump table[pc+index];

If you can start thinking about what your code will end up doing at low level now, then as you progress your code will get better and better.

Too many times I see people get into bad habits when they are starting out and, because current machines are so fast, the code works so they just leave it alone.

Only later when they turn up in a real work place do people start to notice that the code is rubbish, then it’s a real problem.

Start thinking the right way now and you’ll end up a real coder.

7fb33cd08426441a703be8e6012e58f0
0
Jason5755 101 Apr 04, 2013 at 13:25

Thanks for all the information. I will do some more reading on some of the topics discussed here, makes some edits based off of your suggestions and post them for some more feedback.

B5262118b588a5a420230bfbef4a2cdf
0
Stainless 151 Apr 05, 2013 at 08:57

good for you.

we’ll always be here to help

7fb33cd08426441a703be8e6012e58f0
0
Jason5755 101 Apr 22, 2013 at 21:40

Ok….. I know it’s been awhile, but I’ve been doing some reading on the suggestions provided when normal life wasn’t getting in the way and this is what I have so far.

public class RandomWeapon // This class is used to create a random weapon
    {
        private string _weaponName;  // Weapon name
        private string _weaponType;  // Weapon type
        private string _weaponRarity;   // Weapon rarity
        private int _weaponLvl;      // Weapon lvl
        public enum weaponRarityEnum : int
        {
            Common,
            Enchanted,
            Exceptional,
            Fabled,
            Cosmic
        };
        public enum npcRarityEnum : int
        {
            Normal,
            Elite,
            Special,
            Boss
        };
        public string weaponName
        {
            get
            {
                return this._weaponName;
            }
        }
        public string weaponType
        {
            get
            {
                return this._weaponType;
            }
        }
        public string weaponRarity
        {
            get
            {
                return this._weaponRarity;
            }
        }
       
        public int weaponLvl
        {
            get
            {
                return this._weaponLvl;
            }
        }
        // RandomWeapon constructor
        public RandomWeapon(int npcLvl, npcRarityEnum npcRarity, int playerLuck)
        {
            _weaponRarity = WeaponRarity(npcRarity, playerLuck);
            _weaponLvl = WeaponLvl(npcLvl);
        }
        /*public string WeaponType()
        {
        }*/
        public string WeaponRarity(npcRarityEnum npcRarity, int playerLuck)
        {
            double rand1 = 0.0;
            double npcRarityMultiplier = 0.0;
            double playerLuckModifer = playerLuck * .001;
            string rarity = "";
            double[] rarityBaseProbabilty = { .01, .03, .10, .35, 1.00 };
            //double[] npcRarityMultiplierArray = {1.0,1.5,1.75,2.0};
            Random random = new Random();
            rand1 = random.NextDouble();
           
            Console.WriteLine(rand1);
            switch (npcRarity)
            {
                case npcRarityEnum.Normal:
                    npcRarityMultiplier = 1.0;
                    break;
                case npcRarityEnum.Elite:
                    npcRarityMultiplier = 1.5;
                    break;
                case npcRarityEnum.Special:
                    npcRarityMultiplier = 1.75;
                    break;
                case npcRarityEnum.Boss:
                    npcRarityMultiplier = 2.0;
                    break;
            }
            /*if (npcRarity == npcRarityArray[0])
                npcRarityMultiplier = npcRarityMultiplierArray[0];
            else if (npcRarity == npcRarityArray[1])
                npcRarityMultiplier = npcRarityMultiplierArray[1];
            else if (npcRarity == npcRarityArray[2])
                npcRarityMultiplier = npcRarityMultiplierArray[2];
            else if (npcRarity == npcRarityArray[3])
                npcRarityMultiplier = npcRarityMultiplierArray[3];*/
            Console.WriteLine(npcRarityMultiplier);

            if (rand1 < (rarityBaseProbabilty[0] + playerLuckModifer) * npcRarityMultiplier)
                rarity = weaponRarityEnum.Cosmic.ToString();
            else if (rand1 < (rarityBaseProbabilty[1] + playerLuckModifer) * npcRarityMultiplier)
                rarity = weaponRarityEnum.Fabled.ToString();
            else if (rand1 < (rarityBaseProbabilty[2] + playerLuckModifer) * npcRarityMultiplier)
                rarity = weaponRarityEnum.Exceptional.ToString();
            else if (rand1 < (rarityBaseProbabilty[3] + playerLuckModifer) * npcRarityMultiplier)
                rarity = weaponRarityEnum.Enchanted.ToString();
            else if (rand1 < (rarityBaseProbabilty[4] + playerLuckModifer) * npcRarityMultiplier)
                rarity = weaponRarityEnum.Common.ToString();
            return rarity;
        }
        public int WeaponLvl(int npcLvl)
        {
            int lvl = 0;
            Random rand1 = new Random();
            lvl = rand1.Next(npcLvl - 5, npcLvl + 5);
            return lvl;
        }
    }

   
}
public static void Main()
    {
        RandomWeapon randomWeapon = new RandomWeapon(20, RandomWeapon.npcRarityEnum.Boss, 40);
        Console.WriteLine(randomWeapon.weaponRarity);
        Console.WriteLine(randomWeapon.weaponLvl);
    }
82965f1f14a5b4f49db388cc64f26f70
0
tuananhpl1 101 Apr 25, 2013 at 04:36

I’m having such trouble, Good info.