42

So my professor was giving back some feedback on a project I've been working on. He docked a few marks for this code:

if (comboVendor.SelectedIndex == 0) {
  createVendor cv = new createVendor();
  cv.ShowDialog();
  loadVendors();
}

This is in a combobox "index changed" handler. It's used when the user wants to create a new vendor, my top option (index 0, that never changes) opens the "Create a new Vendor" dialog. So the content of my combo box ends up looking like this:

Create New Vendor...
Existing Vendor
Existing Vendor 2
Existing Vendor 3

His problem is with the first line code:

if (comboVendor.SelectedIndex == 0)

He claims that the 0 should be a constant, and actually docked me marks because of that. He claims I shouldn't use literals in my code at all.

The thing is, I don't understand why I would want to make that code in that situation a constant. That index will never change, nor is it something that you would need to tweak. It seems like a waste of memory to keep a single 0 in memory that's used for a very specific situation and never changes.

17
  • 32
    He's being dogmatic. Magic numbers are in general a good thing to avoid. I think -1, 0 and 1 can be considered exceptions to that rule. Also, a constant like this wouldn't take any more room than the literal 0. Dec 1, 2011 at 21:03
  • 23
    @DaveMooney: Are you sure you're not having a knee-jerk reaction here? It's true that things like the -1 in str.indexOf(substr) != -1 for "str contains substr" is prefectly justified. But here, the meaning of the 0 is neither obvious (what't the relation to creating a new vendor?) nor truly constant (what if the way to create a new vendor changes?).
    – user7043
    Dec 1, 2011 at 21:08
  • 62
    you have to learn the rules before you get to break the rules
    – Ryathal
    Dec 1, 2011 at 21:23
  • 12
    I had this method fail on me unexpectedly. The list was sorted alphabetically. I used - - Create new - - so the dashes would sort first and hard coded index 0 to the CreateNew method. Then someone added an item starting with a single quote 'My Item' which sorts before dashes. My hard coding caused the program to crash the next time the list was loaded. I had to manually modify the list's data file to recover the customer's data. Dec 1, 2011 at 23:47
  • 14
    You can use int.Zero instead to make him happy :) Dec 2, 2011 at 0:04

14 Answers 14

90

The actual correct way of doing this in C# is to not rely on the ordering of the ComboItems at all.

public partial class MyForm : Form
{
    private readonly object VENDOR_NEW = new object();

    public MyForm()
    {
        InitializeComponents();
        comboVendor.Items.Insert(0, VENDOR_NEW);
    }

    private void comboVendor_Format(object sender, ListControlConvertEventArgs e)
    {
        e.Value = (e.ListItem == VENDOR_NEW ? "Create New Vendor" : e.ListItem);
    }

    private void comboVendor_SelectedIndexChanged(object sender, EventArgs e)
    {
        if(comboVendor.SelectedItem == VENDOR_NEW)
        {
            //Special logic for selecting "create new vendor"
        }
        else
        {
            //Usual logic
        }
    }
}
8
  • 22
    Well, if this is C#, you shouldn't be using ALL_CAPS for constants. Constants should be PascalCased - stackoverflow.com/questions/242534/…
    – Groky
    Dec 2, 2011 at 11:05
  • 4
    @Groky : Why does this matter? Who cares about how he names his constants? This is 100% correct if he uses ALL_CAPS in a consistent manner for constants. Dec 2, 2011 at 14:45
  • 4
    @marcof: Well, if the constants are part of a public interface then he should be following MS naming guidelines. If not, then he should at least be learning best practise early.
    – Groky
    Dec 2, 2011 at 15:28
  • 2
    This is still incorrect. It shifts the problem from having an integer literal (zero) to a string literal (Create New Vendor). At best the problem is now 'what if the label changes' instead of 'what if the index changes'.
    – Freiheit
    Dec 2, 2011 at 18:49
  • 7
    @Freiheit: Incorrect. The string constant here is only for display; it doesn't affect the logic of the program whatsoever. Unlike using magic-values/strings to store program-state, changing this string (or adding/removing things from the list) could never break anything. Dec 2, 2011 at 20:20
83

The order in the combo box could change. What if you add another option like "Create Special Vendor..." before your "Create New Vender..."

The advantage of using a constant is if there are many methods that depend on the order of the combo box, you only have to change the constant and not all the methods if this does change.

Using a constant is also more readable than a literal.

if (comboVendor.SelectedIndex == NewVendorIndex)

Most compiled languages will substitute the constant at compile time, so there is no performance penalty.

8
  • 5
    I'd use a descriptive code in the value attribute, rather than rely on an index that can change (moving the create option to the bottom of the list, will absolutely break the constant method). Then just look for that code.
    – CaffGeek
    Dec 1, 2011 at 22:07
  • 1
    @Chad I agree identifying controls by the order in a gui is very fragile. If the language supports adding a value to the gui element that can be looked up, I'd use that. Dec 1, 2011 at 22:13
  • 3
    @solution because that is the convention.
    – Ikke
    Dec 2, 2011 at 6:31
  • 3
    @Ikke That is definitely not the convention. stackoverflow.com/questions/242534/… Dec 2, 2011 at 6:43
  • 1
    If we are talking about C# then NewVendorIndex IS the convention. It's consistent with the rest of .NET style.
    – MaR
    Dec 2, 2011 at 12:14
36

This situation you describe is a judgement call, personally I wouldn't use one if its only used once and is already readable.

The real answer though is that he picked this off to teach you a lesson.

Don't forget that he is a professor, his job is to teach you coding and best practices.

I'd say he is doing a pretty good job actually.

Sure he might come off a bit absolute but I'm certain you'll think again before using magic numbers.

Also he got under your skin enough for you to join an online community about programmers just to find out what is considered a best practice in this situation.

Hats off to your professor.

2
  • +1 for indicating that, in this case, the means justify the end result
    – m-smith
    Dec 2, 2011 at 16:50
  • @Thanos- "Don't forget that he is a professor, his job is to teach you coding and best practices." this was never the case for my professor. I would say his job is to teach you what the the department feels is important once the course is created.
    – Ramhound
    Dec 2, 2011 at 18:09
13

[...] my top option (index 0, that never changes) opens the "Create a new Vendor" dialog".

That fact that you had to explain that proves why you should use a constant. If you introduced a constant like NEW_VENDOR_DIALOG, your code would be more self-explanatory. Besides, compilers optimize out constants, so there won't be a change in performance.

Write programs for programmers, not compilers. Unless you're specifically trying to micro-optimize, which it doesn't seem like you are.

2
  • 2
    -1 for even mentioning performance. Even if not optimized a computer can do billions of operations like this before the user notices. Dec 2, 2011 at 18:39
  • 3
    @Boris OP seemed concerned about the waste of resources, which is why I mentioned it. I don't see how my answer would be less correct because of that.
    – kba
    Dec 3, 2011 at 14:23
12

He claims that the 0 should be a constant, and actually docked me marks because of that.

I'd agree. The use of zero here is "magic". Imagine that you are reading this code for the first time. You do not know why zero is special, and the literal tells you nothing about why zero is special. If instead you said if(comboVendor.SelectedIndex == CreateNewVendorIndex) then it becomes extremely clear to the first-time reader what the code means.

He claims I shouldn't use literals in my code at all.

That's an extreme position; a realistic position would be to say that use of literals is a red flag that indicates that the code might not be as clear as it could be. Sometimes it is appropriate.

The thing is, I don't understand why I would want to make that code in that situation a constant. That index will never change

That it will never change is an excellent reason to make it a constant. That's why constants are called constants; because they never change.

nor is it something that you would need to tweak.

Really? You can't see any situation in which someone might want to change the order of things in a combo box?

The fact that you can see a reason why this might change in the future is a good reason to not make it a constant. Rather it should be a non-constant readonly static integer field. A constant should be a quantity that is guaranteed to stay the same for all time. Pi and the atomic number of gold are good constants. Version numbers are not; they change every version. The price of gold is obviously a terrible constant; it changes every second. Only make constant things that never, ever change.

It seems like a waste of memory to keep a single 0 in memory that's used for a very specific situation and never changes.

Now we come to the crux of the matter.

This is perhaps the most important line in your question because it indicates that you have some deeply flawed understanding of (1) memory, and (2) optimization. You're in school to learn, and now would be a great time to get a correct understanding of the fundamentals. Can you explain in detail why you believe that "it is a waste of memory to keep a single zero in memory"? First off, why do you believe that optimizing the usage of four bytes of memory in a process with at least two billion bytes of user-addressible storage is relevant? Second, precisely what resource do you imagine is being consumed here? What do you mean by "memory" being consumed?

I am interested in the answers to these questions first because they're an opportunity for you to learn how your understanding of optimization and memory management is incorrect, and second because I always want to know why beginners believe bizarre things, so that I can design better tools to lead them to have correct beliefs.

6

He's right. You're right. You're wrong.

He's right, conceptually, that magic numbers should be avoided. Constants make code more readable by adding context to what the number means. In the future when someone reads your code, they know why a particular number was used. And if you need to change a value somewhere down the line, its far better to change it in one place rather than try to hunt down everywhere where a particular number is used.

That being said, you are right. In this specific case, I really dont think a constant is warranted. You are looking for the first item in the list, which is always zero. It will never be 23. Or -pi. You are specifically looking for zero. I really dont think you need to clutter the code up by making it a constant.

You are wrong, though, in assuming a constant gets carried around as a variable, 'using memory'. A constant is there for the human and the compiler. It tells the compiler to put that value in that spot during compilation, where you'd otherwise have put a literal number. And even if it did carry the constant in memory, for all but the most demanding applications, the loss of efficiency wouldnt even be measurable. Worrying about the memory use of a single integer definately falls into 'premature optimization'.

3
  • 1
    I would have almost voted this answer up, if not for the third paragraph, claiming that using literal 0 is clear enough and no constant is warranted. Much better solution would have been not to rely on indexing of the combo box item at all, but rather on the value of the selected item instead. Magic constants are magic constants even if they are 0 or 3.14 or whatnot - name them appropriately as this makes the code more readable. Dec 8, 2011 at 11:53
  • It may be better to use the value in the list, but that wasnt what his question was about. His question was about whether that was an appropriate use for a constant. And if one is comparing to 0 within the context of interfacing with the GUI (for whatever reason - perhaps someone is looking for the fist item regardless of value), I think it'd be unnecessary to use a constant in that spot. Dec 8, 2011 at 21:38
  • I would not agree... From just looking at the code, it is never immediately obvious what is the significance of 0 - it might really be that he is always looking for the first item in the list and that is it, but then it would read much cleaner, if the comparison would be 'comboVendor.SelectedIndex == FirstIndex' instead? Dec 9, 2011 at 8:52
2

I would have replaced the 0 with a constant to make the meaning clear, such as NewVendorIndex. You never know if your order will change.

1

That is the total preference of your professor. Typically, you only use a constant if the literal will be used multiple times, you want to make it obvious to the reader what the purpose of the line is, or your literal will possibly be changed in the future, and you only want to change it in one place. However, for this semester, the professor is boss, so I would do it from now on in that class.

Good training for the corporate world? Quite Possibly.

1
  • 2
    I don't agree to the "only use a constant if the literal will be used multiple times" part. With 0 (and maybe -1, 1) it is often clear, in most cases it is good to give a name to a thing which is way more clearer wile reading the code.
    – johannes
    Dec 1, 2011 at 21:43
1

To be honest, while I don't think your code is best practice, his suggestion is frankly a little grotesque.

A more common practice for a .NET combobox is to give the "Select.." item an empty value, while the actual items have meaningful values, and then do:

if (string.IsNullOrEmpty(comboVendor.SelectedValue))

rather than

if (comboVendor.SelectedIndex == 0)
5
  • 3
    In your example, null is simply another literal. The heart of this lesson is that literals are to be avoided. Dec 2, 2011 at 7:52
  • @overslacked - There is no null literal in my example. Dec 2, 2011 at 10:37
  • Even if there was a null literal, null is an acceptable literal to use, I don't even believe there is a way to check if a reference to an object is null without using checking if its equal to null.
    – Ramhound
    Dec 2, 2011 at 18:18
  • Carson63000 - I was referring to your use of IsNullOrEmpty. You're swapping one "magic value" for another. @Ramhound - Null is an acceptable literal in some cases, no question, but I do not believe this is a good example (using null or blank as a magic value). Dec 3, 2011 at 6:29
  • -1 for "a little grotesque". While you're right that using the value would be more conventional and easier to understand, if using the selectedindex, using a named constant is definitely better than just 0.
    – jmoreno
    Dec 3, 2011 at 17:56
1

He is not wrong for stressing the value of using constants and you are not wrong in using literals. Unless he had stressed that this is the expected coding style, you should not lose marks for using literals since they are not harmful. I have seen literals used all over the place so many times in commercial code.

His point is good though. This may be his way to make you aware of the benefits of constants:

1-They protect your code to some extent from accidental tampering

2-As @DeadMG says in his answer, if the same literal value is used in many places it may appear with a different value by mistake - So constants preserve consistency.

3-Constants preserve type, so you don't have to use something like 0F to mean zero.

4-For ease of reading, COBOL uses ZERO as a reserved word for the value zero (but also allows you to use the literal zero) - So, giving a value a name is sometimes helpful, for example: (Source: ms-Constants

class CalendarCalc
{
    const int months = 12;
    const int weeks = 52; //This is not the best way to initialize weeks see comment
    const int days = 365;

    const double daysPerWeek = (double) days / (double) weeks;
    const double daysPerMonth = (double) days / (double) months;
}

or as in your case (as shown in @Michael Krussel answer)

3
  • 2
    Isn't that going to be an strange definition for days per week? There are 7 days a week not 7.4287143 Dec 1, 2011 at 21:31
  • @WinstonEwert, thanks for your comment. 365/52 = 7.01923076923077=7+(1/52). Now, if you remove the fractional part and calculate 7*52 you get 364 days which is not the correct number of days in a year. Having a fraction is more accurate than loosing it (since you can format the result to show 7 if you only want to display the number). Anyway, it was just an example from MS about constants, but your point is interesting.
    – NoChance
    Dec 2, 2011 at 0:37
  • 1
    Of course its just an example, but I object to your statement that its more accurate to include the fraction. A week is defined as 7 days. Given your definition 26 weeks is 182.5 days which simply isn't accurate. Really, the problem is your int weeks = 52, there are not 52 weeks per year. There are 52.142857142857146 weeks in a year, and thats the number that you should keeping the fractions on. Of course, the only thing that's actually constant in that entire set of constants the number of months. Dec 2, 2011 at 1:00
0

You would only need to put it in a constant if it has a complex derivation, or if it's repeated often. Else, a literal is fine. Putting everything in a constant is total overkill.

1
  • Only if you consider twice often and you never need to modify the code. It is really easy to create bugs by changing one of two cases.
    – BillThor
    Dec 2, 2011 at 4:43
0

Actually, as mentioned, what if it's position changes? What you could/should do is use a code, rather than rely on index.

So, when you create your select list it ends up with html like

<select>
    <option value='CREATE'>Create New Vendor...</option>
    <option value='1'>Existing Vendor</option>
    <option value='2'>Existing Vendor 2</option>
    <option value='3'>Existing Vendor 3</option>
</select>

Then instead of checking for selectedIndex === 0, check that the the value is CREATECODE which would be in a constant and would have been used for both this test, and when creating the select list.

1
  • 3
    Probably a good approach, but seeing as he's using C#, html isn't the most hopeful example code. Dec 1, 2011 at 22:13
0

I would get rid of it altogether. Just put a create new button next to the combo box list. Double click an item in the list to edit, or click the button. Don't have the new functionality buried in the combobox. Then the magic number is removed altogether.

In general any literal number in code should be defined as a constant so as to put context around the number. What's zero mean? In this case 0 = NEW_VENDOR. In other cases, it might mean something different, so it is always a good idea for readability and maintainability to put some context around it.

0

As others have said, you should use some method other than index number to identify the combo box item that corresponds to a given action; or, you can find the index with some programmatic logic and store that in a variable.

The reason I'm writing is to address your comment on the "use of memory". In C#, as in most languages, constants are "folded" by the compiler. For example, compile the following program, and examine the IL. You'll find that all those numbers don't even make it into the IL, let alone the computer's memory:

public class Program
{
    public static int Main()
    {
        const int a = 1000;
        const int b = a + a;
        const int c = b + 42;
        const int d = 7928345;
        return (a + b + c + d) / (-a - b - c - d);
    }
}

resulting IL:

.method public hidebysig static 
    int32 Main () cil managed 
{
    .maxstack 1
    .locals init (
        [0] int32 CS$1$0000
    )

    IL_0000: nop
    IL_0001: ldc.i4.m1  // the constant value -1 to be returned.
    IL_0002: stloc.0
    IL_0003: br.s IL_0005

    IL_0005: ldloc.0
    IL_0006: ret
}

So whether you use a constant, a literal, or a kilobyte of code using constant arithmetic, the value is treated literally in the IL.

A related point: Constant folding applies to string literals. Many believe that a call like this causes too much needless, inefficient string concatenation:

public class Program
{
    public static int Main()
    {
        const string a = "a";
        const string b = a + a;
        const string c = "C";
        const string d = "Dee";
        return (a + b + c + d).Length;
    }
}

But check out the IL:

IL_0000: nop
IL_0001: ldstr "aaaCDee"
IL_0006: callvirt instance int32 [mscorlib]System.String::get_Length()
IL_000b: stloc.0
IL_000c: br.s IL_000e
IL_000e: ldloc.0
IL_000f: ret

Bottom line: Operators on constant expressions result in constant expressions, and the compiler does all the computation; it doesn't affect run-time performance.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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