Fog Creek Software
Discussion Board




Design Question...

I've got a Windows application that writes information to performance counters so that we can keep an eye on it's performance.

When the program starts, it creates the counters and as it runs, it updates them. Nothing fancy.

My quandry is that I've got the performance counters names in the code twice: Once when I create them, and again when I need to update them.

I'm a big believer in the DRY principle (Don't Repeat Yourself) and I don't like having the counter names in the code twice. If we ever change the counter names, we've got to search through the code and find all the occurrences. (Not likely, but still...)

What do you think is the best way to only have the counter names in the code once? I suppose I could have a series of functions that return the names of the counters, but we've got dozens of counters and that would require a function for each counter.

FWIW, the code is written in C#.

Mark Hoffman
Monday, May 19, 2003

The thing I see with it is that the function that returns the counter would be named appropriately. I think it'd either be named, or take an integer paramater and returrn the nth counter.

The problem is, you need a name somewhere, unless your code will have calls to getFirstCounter() all over the place.

I wouldn't bother, if it's only twice. When it's 3  times, maybe start worrying, but for now, I can't think of a way that's better, and the alternatives seem worse.

Mike Swieton
Monday, May 19, 2003

"I wouldn't bother, if it's only twice. When it's 3  times, maybe start worrying, but for now, I can't think of a way that's better, and the alternatives seem worse. "

Trouble is, when it becomes 3 times, I won't have the time to address it then.

After posting, I was like "Duh..Why not just use constants?" So, I'm leaning towards have a listing of about 15-20 constant strings that define the performance counter name.

Mark Hoffman
Monday, May 19, 2003

You could have a class Counters that would simulate being a 'global' module for your constants:

public class Counters
{
    public const FirstCounter = "Real name of First Counter";
    public const SecondCounter = "name of second counter";

  // etc..
}

And then you would be able to access them as if they were part of an enumeration, but with string type:

// ...
InitCounter( Counters.FirstCounter );
// ...

Sergio
Monday, May 19, 2003

It would be better to make your class 'internal' instad of 'public', though.

Sergio
Monday, May 19, 2003

Can you use static instances of the Objects you want?  I would replace the code above with: (forgive my Java code, I haven't learned C#)

public class Counters
{
    public static final Counter first = new Counter("Real name of First Counter");
    public static final Counter second = new Counter("Real name of First Counter");
  // etc..
}

And then you would be able to access them as objects.
Counters.first.log(...);

Adam Young
Monday, May 19, 2003

Why not use some kind of factory pattern and declare the counter name once on instantiation?

Eg:

myErgsCounter = counterFactory.Create("ErgCounter");
myHumusCounter = counterFactory.Create("HumusCounter");
myAppleCounter = counterFactory.Create("AppleCounter");

Geoff Bennett
Monday, May 19, 2003

question:

what is the difference between these two:

1)

x = getDoodadCounter();
x++;

2)

g_doodadcounter++;

?

Mike Swieton
Monday, May 19, 2003

clarification of the above:

what is the difference in terms of overall results and design? I mean, you're hardcoding the name either way...

Mike Swieton
Monday, May 19, 2003

The getDoodadCounter() function hides the fact that it uses a global or static variable to store the counter. You can later use this to change the implementation without affecting the rest of the application.

Frederik Slijkerman
Tuesday, May 20, 2003

Defined constants sounds like good KISS. OTOH, why do you think there would be a need to change them later? Maybe there lies the solution as to what you need to code for, and why simple constants may just not be good enough.

Just me (Sir to you)
Tuesday, May 20, 2003

If you have time, why not go ahead and create a generic performance counter class?  You'll end up using it again in another application anyway.

Then instantiate them using your constants.

Then again, you may have already done that by now.

Steve Barbour
Tuesday, May 20, 2003

Thanks for the comments, everyone! Lots of of good ideas to digest.

Mark Hoffman
Tuesday, May 20, 2003

The idea that you should never repeat anything is a misconception. You always have to repeat something.

If you specify counters by literal string, you have to repeat the strings.

If you specify counters by constants that evaluate to literal strings, you have to repeat the constants.

If you access counters through a counter class, you have to repeat the class or instance name.

The question you should ask yourself is this: would you EVER conceivably want to change the counter name? And if you would, wouldn't the same reason cause you to change the constant or class/object name if you used the other approaches?

You hide literal constant behind symbolic constants or other constructs only if those literal constants are likely to change. Otherwise, it's completely pointless.

You don't define a constant Integer.Zero so that you don't have to repeatedly use the literal constant 0. On the other hand, you would define a string constant to define the name of some input or output file -- because the value of that constant might actually change.

So I suggest you analyse the usage of this counter name and see if it's more like 0 or more like a file name, and make your decision based on that.

Chris Nahr
Tuesday, May 20, 2003

Chris,

By saying that I don't want to repeat something means I don't want to have the same logic embedded in more than once place.

For instance, if I refer to a counter as a string in 3 places then any change to that counter name will require it being changed in 3 places. If I have it mispelled in one place, the compiler will be quite happy and won't complain.

If I define the counter name as a constant, then I repeat the constant as many times as I want, but I only have to change the actual name of the counter one time. That was my purpose.

I've lost count of the number of times that I said "I'll never need to change this" and then had that decision come back and bite me. I think we all have had those situations.

So while I can't think of any reason for changing a performance counter's name down the road, I've learned enough to avoid hardcoding in 50 different places. One of those "ounce of prevention" thing...

Mark Hoffman
Tuesday, May 20, 2003

Get yourself a copy of "Code Complete" by Steve MCConell.

drazen
Tuesday, May 20, 2003

Okay, but wouldn't you at least get a runtime error if you tried to use a counter with an unknown (= misspelled) name? I'm not familiar with .NET performance counters but dictionary-type properties in .NET usually either throw an exception or return a null reference if you feed them an unknown identifier.

Anyway, what I'm trying to say is that the symbolic name for a performance counter is very likely just the literal name without the quotes; and in any situation where you would change the literal name you'd just as likely have to change the symbolic name, too.

In other words, it doesn't really make a difference if you get a name clash because of "CounterOne" or because of CounterOne. There isn't any additional information stored in the literal string that goes beyond the symbolic string, it's just an identifier in either case.

Chris Nahr
Tuesday, May 20, 2003

It probably doesn't make too much difference whether you go with something like:
CounterFactory.getFooCounter()
or
CounterFactory.getCounter(CounterFactory.COUNTER_TYPE_FOO);
But I prefer either of these to:
getCounter("Foo");
for the simple reason that if you decide to get rid of the Foo counter, the first two turn into compile-time errors, while the third is a run-time error.  Sure, it's a run-time error that will turn up the first time you go along that code path, but I always prefer compile/link errors to runtime errors.

Brian
Tuesday, May 20, 2003

The examples all looked like C code rather than C++, but they can be implemented in C++ so the same code doesn't have the same problems.


1)
x = getDoodadCounter();
x++;

This gets a copy of the variable, and increments it. The original isn't changed.  If x was a reference rather than a local variable, then you're required to implement the doodad counter in a way that it can be usefully manipulated that way.

If you use a class rather than a basic datatype such as an integer and getDoodadCounter() accesses the singleton instance, then the ++ operator may be perfectly acceptable, as the code to implement the concept of incrementing the counter by 1 will be isolated within the class, not the code that wanted the counter incremented.

If, as I first assumed, it just returns the current value then you've created a read-only value.

2)
g_doodadcounter++;

This actually changes the global variable. Obviously it requires you to have a global variable.

You could apply the singleton idea to allow useful code behind the ++ operator, but singletons tend to have an accessor method, rather than be global variables.

3) (new option)
setDoodadCounter( getDoodadCounter() + 1 );

Assuming the functions get and set the value of the counter:

This is a thread-unsafe way to increment the variable, without the code needing to know how or where the counter is stored. It could be a variable, a difference between two other related variables, a database entry, or something else.

andrew m
Tuesday, May 20, 2003

Brian,

You are right. An enumration would be a better way to implement the factory. I was just trying to quickly bang out a solution.

I think the factory pattern returning a generic performance counter class is a more orthagonal solution, and it allows greater code resuse. Keep the factory and the generic in their own module and they can be imported into any other application requiring performance counters.

Geoff Bennett
Tuesday, May 20, 2003

The reason to prefer constants over literal strings:

GetCounter( Counters.Foo )  // Compiles, works
GetCounter( "Foo" )              // Compiles, works
GetCounter( "Fop" )              // Compiles, unpredictable at runtime
GetCounter( Counters.Fop ) // Compile time error

If we're stuck using statically typed languages, we might as well take advantage of that static typing to catch stupid mistakes at compile time. That's what it's for.

Chris Tavares
Wednesday, May 21, 2003

*  Recent Topics

*  Fog Creek Home