5
\$\begingroup\$

I build my first "bigger" program with classes. Does this look good? Or do you see something, which I could do better? Do you think I could use a delegate for changing the stones to money here? And how to do this?

using System;

// collect stones
// if 20 stones collected --> event --> change stones for money

namespace FunInTheQuarry
{
    class MainClass
    {
        static void Main(string[] args)
        {
            Quarry quarry = new Quarry();
            Person person1 = new Person();

            for (int i = 0; i < 1000; i++)
            {
                System.Threading.Thread.Sleep(300);
                person1.Work();
                Console.WriteLine("{0} stones in the quarry.", Quarry.stone);
            }
            Console.Read();
        }
    }

    public class Quarry
    {
        public static int stone { get; set; }

        public Quarry()
        {
            stone = 10000;
        }
    }

    public class Person
    {
        public int stone;
        public int money;

        public Person()
        {
            stone = 0;
            money = 0;
        }

        public void Work()
        {
            stone += 1;
            Quarry.stone -= 1;
            if (this.stone >= 20)
            {
                ChangeStonesToMoney(ref this.stone, ref this.money);
            }
        }

        public void ChangeStonesToMoney(ref int stone, ref int money)
        {
            stone -= 20;
            money += 1;
            Console.WriteLine("One Person has now {0} dollar.", this.money);
        }

    }
}

EDIT: this is my revised work.

http://pastebin.com/99gRrUqv

\$\endgroup\$
6
\$\begingroup\$

Before worrying about delegates, there are some very basic notions we should review! :)

Never, ever getting back together expose your fields. They should be private. Use properties like you did in your Quarry class instead. This pattern is called encapsulation and is an important practice in the software development process.

In general, an instance (like new Quarry()) shouldn't deal with static fields. Why is that? Imagine the following scenario where you have 2 quarries.

Quarry quarry = new Quarry();
quarry.stone -= 100; //Somebody's been working hard
Quarry quarry2 = new Quarry();
Console.WriteLine(quarry.stone);

What is printed? Is it 9900? Nope, because your instance constructor resets the static field. The stone field shouldn't be static. Each Quarry should have its own number of stones. Maybe the number of stones could be customized with a parameter in the constructor?

public class Quarry
{
    public int Stones { get; set; }

    public Quarry(int stones)
    {
        Stones = stones;
    }
}

This way each of your Quarry is independant.

Next problem, there is no relation between your Person and your Quarry. This is bad. A Person works in a Quarry (in your example, we'll keep it simple), but in your code, we don't see this relation. To keep it simple, we'll say that a Person Works with a Quarry. What does it mean? It means that the Work method should receive a Quarry parameter.

public class Person
{
    public int stone;
    public int money;

    public Person()
    {
        stone = 0;
        money = 0;
    }

    public void Work(Quarry quarry)
    {
        stone += 1;
        quarry.Stones -= 1;
        if (this.stone >= 20)
        {
            ChangeStonesToMoney(ref this.stone, ref this.money);
        }
    }

    public void ChangeStonesToMoney(ref int stone, ref int money)
    {
        stone -= 20;
        money += 1;
        Console.WriteLine("One Person has now {0} dollar.", this.money);
    }
}

Notice that stones isn't the best variable name you can use. Perhaps workedStones would be better. Or something like this.

Also, you use Thread.Sleep(300); in your main loop, but I assume that it's the person's work that takes 300 milliseconds (damn those persons are fast!). So maybe the sleep should be in the Work method. Using Thread.Sleep() is unorthodox and is usually not recommended, but for your example I get why you're using it! :)

I noticed you were using ref. That's bad. I mean, you used it in a "good" scenario, as you want to modify an int by reference, but you don't need to do this and you shouldn't include ref everywhere just for the kick of it.

You already have access to stone and money, so why pass them as parameter?

public void Work()
{
    stone += 1;
    Quarry.stone -= 1;
    if (this.stone >= 20)
    {
        ChangeStonesToMoney();
    }
}

public void ChangeStonesToMoney()
{
    stone -= 20;
    money += 1;
    Console.WriteLine("One Person has now {0} dollar.", this.money);
}

Now, I don't have time to go for a longer review, but I suggest you think about the following points :

  • Why is the Person class responsible for knowing how many stones are worth a dollar? Shouldn't another class be responsible for this?
  • Shouldn't a Quarry have a list of Person that works for it? Instead of having different people work on a quarry once in awhile like it is actually done at the moment.
  • Are these people under the minimum wage?

Otherwise, there's some good stuff in there, you've done a good job for a first little application :)

\$\endgroup\$
  • \$\begingroup\$ Thank you very much! One question. Why does ChangeStonesToMoney() know who the money gets if I have more than one Person. \$\endgroup\$ – kame Oct 29 '15 at 15:49
  • \$\begingroup\$ @kame I'm not sure I understand your question. \$\endgroup\$ – IEatBagels Oct 29 '15 at 16:28
  • \$\begingroup\$ I think I have to make a better plan about the relation of all my classes and objects. \$\endgroup\$ – kame Oct 30 '15 at 12:52
  • \$\begingroup\$ @kame I propose you post a follow-up question, this way you'll get other reviews :) \$\endgroup\$ – IEatBagels Oct 30 '15 at 13:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.