CODEWITHSUNDEEP
c#coding-style
Bryan Rowe
Bryan Rowe

Reputation: 9323

Constant abuse?

I have run across a bunch of code in a few C# projects that have the following constants: 

    const int ZERO_RECORDS = 0;
    const int FIRST_ROW = 0;
    const int DEFAULT_INDEX = 0;
    const int STRINGS_ARE_EQUAL = 0;

Has anyone ever seen anything like this? Is there any way to rationalize using constants to represent language constructs? IE: C#'s first index in an array is at position 0. I would think that if a developer needs to depend on a constant to tell them that the language is 0 based, there is a bigger issue at hand.

The most common usage of these constants is in handling Data Tables or within 'for' loops.

Am I out of place thinking these are a code smell? I feel that these aren't a whole lot better than:

const int ZERO = 0;
const string A = "A";

Upvotes: 19

Views: 1876

Answers (17)

David Braverman
David Braverman

Reputation: 81

It's all right to use constants to represent abstract values, but quite another to represent constructs in your own language.

const int FIRST_ROW = 0 doesn't make sense.

const int MINIMUM_WIDGET_COUNT = 0 makes more sense.

The presumption that you should follow a coding standard makes sense. (That is, coding standards are presumptively correct within an organization.) Slavishly following it when the presumption isn't met doesn't make sense.

So I agree with the earlier posters that some of the smelly constants probably resulted from following a coding standard ("no magic numbers") to the letter without exception. That's the problem here.

Upvotes: 0

stefanB
stefanB

Reputation: 79840

I think sometimes people blindly follow 'Coding standards' which say "Don't use hardcoded values, define them as constants so that it's easier to manage the code when it needs to be updated' - which is fair enough for stuff like:

const in MAX_NUMBER_OF_ELEMENTS_I_WILL_ALLOW = 100

But does not make sense for:

if(str1.CompareTo(str2) == STRINGS_ARE_EQUAL)

Because everytime I see this code I need to search for what STRINGS_ARE_EQUAL is defined as and then check with docs if that is correct.

Instead if I see:

if(str1.CompareTo(str2) == 0)

I skip step 1 (search what STRINGS_ARE... is defined as) and can check specs for what value 0 means.

You would correctly feel like replacing this with Equals() and use CompareTo() in cases where you are interested in more that just one case, e.g.:

switch (bla.CompareTo(bla1))
{
     case IS_EQUAL:
     case IS_SMALLER:
     case IS_BIGGER:
     default:
}

using if/else statements if appropriate (no idea what CompareTo() returns ...)

I would still check if you defined the values correctly according to specs.

This is of course different if the specs defines something like ComparisonClass::StringsAreEqual value or something like that (I've just made that one up) then you would not use 0 but the appropriate variable.

So it depends, when you specifically need to access first element in array arr[0] is better than arr[FIRST_ELEMENT] because I will still go and check what you have defined as FIRST_ELEMENT because I will not trust you and it might be something different than 0 - for example your 0 element is dud and the real first element is stored at 1 - who knows.

Upvotes: 2

devuxer
devuxer

Reputation: 42374

There are generally four reasons I can think of for using a constant:

  1. As a substitute for a value that could reasonably change in the future (e.g., IdColumnNumber = 1).
  2. As a label for a value that may not be easy to understand or meaningful on its own (e.g. FirstAsciiLetter = 65),
  3. As a shorter and less error-prone way of typing a lengthy or hard to type value (e.g., LongSongTitle = "Supercalifragilisticexpialidocious")
  4. As a memory aid for a value that is hard to remember (e.g., PI = 3.14159265)

For your particular examples, here's how I'd judge each example:

const int ZERO_RECORDS = 0;
// almost definitely a code smell

const int FIRST_ROW = 0;
// first row could be 1 or 0, so this potentially fits reason #2,
// however, doesn't make much sense for standard .NET collections
// because they are always zero-based

const int DEFAULT_INDEX = 0;
// this fits reason #2, possibly #1

const int STRINGS_ARE_EQUAL = 0;
// this very nicely fits reason #2, possibly #4
// (at least for anyone not intimately familiar with string.CompareTo())

So, I would say that, no, these are not worse than Zero = 0 or A = "A".

Upvotes: 1

Ken Lange
Ken Lange

Reputation: 963

You might see something like this in a cross-platform situation where you would use the file with the set of constants appropriate to the platform. But Probably not with these actual examples. This looks like a COBOL coder was trying to make his C# look more like english language (No offence intended to COBOL coders).

Upvotes: 0

grenade
grenade

Reputation: 32169

Right you are to question this smell young code warrior. However, these named constants derive from coding practices much older than the dawn of Visual Studio. They probably are redundant but you could do worse than to understand the origin of the convention. Think NASA computers, way back when...

Upvotes: 0

Daniel
Daniel

Reputation: 75

Is this code something in your office or something you downloaded?

If it's in the office, I think it's a problem with management if people are randomly placing constants around. Globally, there shouldn't be any constants unless everyone has a clear idea or agreement of what those constants are used for.

In C# ideally you'd want to create a class that holds constants that are used globally by every other class. For example, 

class MathConstants
{
 public const int ZERO=0;
}

Then in later classes something like:

....
if(something==MathConstants.ZERO)
...

At least that's how I see it. This way everyone can understand what those constants are without even reading anything else. It would reduce confusion.

Upvotes: 1

Kena
Kena

Reputation: 6921

Smells a bit, but I could see cases where this would make sense, especially if you have programmers switching from language to language all the time.

For instance, MATLAB is one-indexed, so I could imagine someone getting fed up with making off-by-one mistakes whenever they switch languages, and defining DEFAULT_INDEX in both C++ and MATLAB programs to abstract the difference. Not necessarily elegant, but if that's what it takes...

Upvotes: 0

Earlz
Earlz

Reputation: 63885

That is definite bad coding.

I say constants should be used only where needed where things could possible change sometime later. For instance, I have a lot of "configuration" options like SESSION_TIMEOUT defined where it should stay the same, but maybe it could be tweaked later on down the road. I do not think ZERO can ever be tweaked down the road.

Also, for magic numbers zero should not be included.

I'm a bit strange I think on that belief though because I would say something like this is going to far

//input is FIELD_xxx where xxx is a number
input.SubString(LENGTH_OF_FIELD_NAME); //cut out the FIELD_ to give us the number

Upvotes: 3

Brian
Brian

Reputation: 2261

If the zero indicates something other than zero (in this case STRINGS_ARE_EQUAL) then that IS Magical. Creating a constant for it is both acceptable and makes the code more readable.

Creating a constant called ZERO is pointless and a waste of finger energy!

Upvotes: 0

Anon.
Anon.

Reputation: 60013

Am I out of place thinking these are a code smell? I feel that these aren't a whole lot better than:

Compare the following:

if(str1.CompareTo(str2) == STRINGS_ARE_EQUAL) ...

with

if(str1.CompareTo(str2) == ZERO) ...
if(str1.CompareTo(str2) == 0) ...

Which one makes more immediate sense?

Upvotes: 12

Adriaan Stander
Adriaan Stander

Reputation: 166486

You should have a look at some of the things at thedailywtf

One2Pt20462262185th

and

Enterprise SQL

Upvotes: 2

micahtan
micahtan

Reputation: 19180

Am I out of place thinking these are a code smell? I feel that these aren't a whole lot better than:

const int ZERO = 0;

const int A = 'A';

Probably a bit of smell, but definitely better than ZERO=0 and A='A'. In the first case they're defining logical constants, i.e. some abstract idea (string equality) with a concrete value implementation.

In your example, you're defining literal constants -- the variables represent the values themselves. If this is the case, I would think that an enumeration is preferred since they rarely are singular values.

Upvotes: 3

gbn
gbn

Reputation: 432431

Abuse, IMHO. "Zero" is just is one of the basics.

Although the STRINGS_ARE_EQUAL could be easy, why not ".Equals"?

Accepted limited use of magic numbers?

Upvotes: 10

tloach
tloach

Reputation: 8040

Some people consider any raw number within a program to be a 'magic number'. I have seen coding standards that basically said that you couldn't just write an integer into a program, it had to be a const int.

Upvotes: 5

Paul Sasik
Paul Sasik

Reputation: 81509

i don't know if i would call them smells, but they do seem redundant. Though DEFAULT_INDEX could actually be useful.

The point is to avoid magic numbers and zeros aren't really magical.

Upvotes: 1

Tim Robinson
Tim Robinson

Reputation: 54764

I'd go for code smell. If these kinds of constants are necessary, put them in an enum:

enum StringEquality
{
    Equal,
    NotEqual
}

(However I suspect STRINGS_ARE_EQUAL is what gets returned by string.Compare, so hacking it to return an enum might be even more verbose.)

Edit: Also SHOUTING_CASE isn't a particularly .NET-style naming convention.

Upvotes: 1

Tom Neyland
Tom Neyland

Reputation: 6968

That definitely a code smell.

The intent may have been to 'add readability' to the code, however things like that actually decrease the readability of code in my opinion.

Upvotes: 6

Related Questions