HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

C# program that uses events

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
usesprogramthatevents

Problem

I am trying to learn C#. Please tell me if the following code respects coding standards and naming conventions because I want to write beautiful code. The purpose of this program was to help me understand different concepts that I've learned in the last few days but I was having difficulties thoroughly understanding them (especially events and delegates). Please show me what you would change by exemplifying, not just by recommending, because I have a harder time understanding phrases than code.

For example, I've been told to use "async/await and the TPL". I've made a couple simple programs with them, yet I still don’t know how I would apply the concepts here. I'll keep trying while waiting for an answer from you, I'll be reading the chapters from here in the next few hours.

Also, I've been told I should have used Action. I want to use Actions because I like Action and Func, yet I don’t know where should I use Action.

TProgram.cs:

```
#define DEBUG
#define TRACE
using System;
using System.Diagnostics;
using System.Text;
using System.Threading;

namespace LearnEventsAndOthers
{
class Program
{
private static void OnTicked(object sender, TickingEventArgs e)
{
Console.WriteLine("Event triggered in object {0} at tick {1}", ((Metronom)sender).Id, e.TickCount);
}

static int Main(string[] args)
{
#region Standard Code - Init

Console.InputEncoding = Console.OutputEncoding = Encoding.UTF8;
Console.SetWindowSize(160, 56);
Console.BufferHeight = 9000;
Console.BufferWidth = 300;
#if DEBUG
Console.WriteLine("The program is running in #DEBUG mode.");
#endif
#if TRACE
Console.WriteLine("The program is running in #TRACE mode.");
#endif

#endregion

// Creating objects and testing overridden functions Equals and GetHashCode...
Metronom m = new Metronom();
Metronom n = new Metrono

Solution


  • c



  • l



  • m



  • n



  • _t



These are poorly named variables that don't follow what most developers have accepted as the norm.

You need to be more descriptive with your names, even when writing Generic code.

  • c -> clock or timer or stopwatch1



That brings me to l.... You don't need it and shouldn't use it, it's a Magical Variable that is just smelly.

You should instead write it like this

private void Clock()
{
    Stopwatch clock = new Stopwatch();
    clock.Start();
    Debug.WriteLineIf(_t.IsAlive, string.Format("Thread from object {0} has gone LIVE!", this.Id));
    while (!_shouldStop)
    {
        if ((clock.ElapsedTicks % this.RandomStop) == 0)
        {
            OnTickedEvent(clock.ElapsedTicks);
        }
    }
}


Edit:

I see your point that you need that magic smelly variable to hold the exact value, but what if the timing is just right so that

l % this.RandomStop != 0


Just a thought.

Edit:

Now there is also an issue with your boolean _shouldStop and that you don't initialize it to a value, it may not cause a direct issue because if a boolean is not initialized it should return a false value when referenced (IIRC), but this is not a good habit to get into, if it were another object type it would return an error the first time that Clock is called.

Easy solution, since you don't want it stopped from the get go just set the variable on declaration

private volatile bool _shouldStop = false;


m and n are Metronomes, so their names should reflect that, each one is a Metronom.

Since this is a simple method for testing the class that you made I would just name them metronome1 and metronome2.

If they served a specific purpose you could name them something more descriptive.

And that brings me to the last one

_t


Okay, it's a thread.

  • Is it my thread?



  • Is it your thread?



  • Is it the main thread?



  • Is it scope specific?



  • Is this thread safe?



  • What is the purpose of this thread?



You should be much more descriptive with your naming.

As a Developer you write code, it should read like a book, so write it elegantly and don't be skimpy.

When I read code and see a single letter variable I have to stop looking at the code and figure out what that letter represents because the name doesn't tell me anything about the object.

Additional:

I noticed that you declare your Main method as an integer method... not sure why you do that just to return 0;

You should just declare it void there isn't any reason that I can see that you need an integer method.

Code Snippets

private void Clock()
{
    Stopwatch clock = new Stopwatch();
    clock.Start();
    Debug.WriteLineIf(_t.IsAlive, string.Format("Thread from object {0} has gone LIVE!", this.Id));
    while (!_shouldStop)
    {
        if ((clock.ElapsedTicks % this.RandomStop) == 0)
        {
            OnTickedEvent(clock.ElapsedTicks);
        }
    }
}
l % this.RandomStop != 0
private volatile bool _shouldStop = false;

Context

StackExchange Code Review Q#70208, answer score: 6

Revisions (0)

No revisions yet.