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 Metronom();
Trace.Listeners.Add(new TextWriterTraceListener(System.IO.File.CreateText("TraceInfo.txt")));
Trace.AutoFlush = true;
Debug.Indent();
Debug.WriteLineIf(!m.Equals(n), string.Format("Objects n and m have been created and they have different IDs: {0} vs {1}", m.GetHashCode(), n.GetHashCode()));
Debug.Assert(!m.Equals(n), "Objects n and m have been created but they are equal! This shouldn't happen.");
// Testing event subscription...
m.TickedEvent += OnTicked;
n.TickedEvent += OnTicked;
// Testing destructors...
Console.WriteLine(Environment.NewLine + "Press any key to destroy the objects!" + Environment.NewLine);
Console.ReadKey();
m.RequestStop();
Console.WriteLine(Environment.NewLine + "The first object was destroyed. Waiting 5 seconds before destroying the second...");
Thread.Sleep(5000);
n.RequestStop();
#region Standard Code - Exit
Console.WriteLine("nPress <enter> to exit!");
Console.ReadLine();
return 0;
#endregion
}
}
}
Metronom.cs:
#define DEBUG
#define TRACE
using System;
using System.Diagnostics;
using System.Threading;
namespace LearnEventsAndOthers
{
public class TickingEventArgs : EventArgs
{
public long TickCount { get; set; }
}
public class Metronom
{
public event EventHandler<TickingEventArgs> TickedEvent;
public Guid Id { get; private set; }
public int RandomStop { get; private set; } // Used to trigger the TickedEvent at random time spans.
protected internal const int SpeedModifier = 17777777; // Used to control the speed at which events are triggered. Higher means slower.
// Why did I make it protected internal? I really wanted to use the access modifier "internal".
private readonly Thread _t;
private volatile bool _shouldStop;
public Metronom()
{
this.Id = Guid.NewGuid();
checked
{
int hashCode = Math.Abs(this.GetHashCode());
while (hashCode > SpeedModifier)
hashCode -= SpeedModifier;
RandomStop = hashCode;
}
_t = new Thread(Clock) { IsBackground = true };
_t.Start();
Debug.WriteLine("Object initialized with GUID HashCode " + this.GetHashCode());
}
~Metronom()
{
_t.Join();
Debug.WriteLineIf(_t.IsAlive == false, string.Format("Thread from object {0} has been stopped.", this.Id));
Debug.Assert(_t.IsAlive == false, string.Format("Thread from object {0} is still running.", this.Id));
}
protected virtual void OnTickedEvent(long l)
{
EventHandler<TickingEventArgs> handler = TickedEvent;
if (handler != null) handler(this, new TickingEventArgs() { TickCount = l });
}
public void RequestStop()
{
_shouldStop = true;
}
private void Clock()
{
Stopwatch c = new Stopwatch();
c.Start();
Debug.WriteLineIf(_t.IsAlive, string.Format("Thread from object {0} has gone LIVE!", this.Id));
while (!_shouldStop)
{
long l = c.ElapsedTicks;
if ((l % this.RandomStop) == 0)
{
OnTickedEvent(l);
}
}
}
public override bool Equals(object obj)
{
return obj is Metronom && (((Metronom)obj).Id.Equals(Id));
}
public override sealed int GetHashCode()
{
return Id.GetHashCode();
}
}
}
Solution
clmn_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->clockortimerorstopwatch1
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.
There’s a subtle bug hidden in your code.
int hashCode = Math.Abs(this.GetHashCode());
When Math.Abs is passed int.MinValue, it throws an OverflowException:
Negating the minimum value of a twos complement number is invalid.
GetHashCode defers to Guid‘s GetHashCode, which makes no guarantees that it won’t return int.MinValue.
How unlikely is it? Well I’ve had a program running for fifteen minutes now to find a Guid with a hash code of int.MinValue and I’ve only found one, but it can happen and it’s good to be aware of (and fix!).
One neat way to fix the bug would be
int hashCode = this.GetHashCode() & int.MaxValue;
that is, assuming you don’t really care about the value returned by GetHashCode, you just want some non-negative integer.
This loop
while (!_shouldStop)
{
long l = c.ElapsedTicks;
if ((l % this.RandomStop) == 0)
{
OnTickedEvent(l);
}
}
pegs the CPU. Two metronomes increases my CPU usage from ~10% to 50%; four metronomes takes it up to 100%. Take a look at System.Timers.Timer for a way of getting the same sort of functionality without using up all the CPU.
I would strongly recommend against using #define DEBUG and #define TRACE in your files like this. To compile without them would require going through every file in your project and removing those lines.
These symbols can be set in Visual Studio at a project level in the project build settings:
Alternatively you can set them using the /define option if you’re compiling by hand.