2

I have a switch case statements in c#, here all the cases i made as private constants ,is there any bad programming practice going on here, or do i need to use enumeration here and enumerator in case block.Only three constants i showed here, i have ten constants and ten case block

private const String FEASIBLESIZE = "Total FEASIBLESIZE";
private const String AVAILABLESIZE = "Total AVAILABLESIZE";
private const String EXCESSSIZE = "Total EXCESSSIZE";
                          .
                          . 
switch (value.ToString())
{
    case FEASIBLESIZE:
        Level.Add(TEAMSIZE, test.ToString());
        break;

    case AVAILABLESIZE:
        Level.Add(BROADSIZE, test.ToString());                                
        break;

    case EXCESSSIZE:
        Level.Add(NARROWSIZE, test.ToString());
        break;
         .
         .
         .
4
  • 5
    ALLCAPS makes your C# code look like something written in COBOL in the early 70s. Commented Jul 13, 2012 at 9:26
  • 1
    Something smells here. Why do you convert Value to string? Commented Jul 13, 2012 at 9:29
  • 1
    Uppercase is often used for constants in c#, which is this case. Commented Jul 13, 2012 at 9:29
  • @TomasGrosup, yes, as we can see in this question this is unfortunately is still the case. But this doesn't mean that the standard C# naming conventions recommend it. Let me quote: Do not use SCREAMING_CAPS. So yeah, the fact that some people are still using this completely horrible convention doesn't mean that this is the C# standard convention. Commented Jul 13, 2012 at 9:39

5 Answers 5

5

Aside from the horrible formatting it looks roughly okay. Of course that's a bit hard to tell without actually knowing your code. Darin is correct though, in that you're not adhering to the default naming conventions (all caps is a no-go anywhere in C#).

But I have seen much worse, if that's any consolation.

Sign up to request clarification or add additional context in comments.

1 Comment

+1 for I have seen much worse, if that's any consolation :-)
4

What you are doing looks like something that can be replaced using a Dictionary<string,string> mapping from one size type to another.

var sizeMap = new Dictionary<string,string>();

sizeMap.Add(FEASIBLESIZE, TEAMSIZE);
sizeMap.Add(AVAILABLESIZE, BROADSIZE);
sizeMap.Add(EXCESSSIZE, NARROWSIZE);

And instead of the switch:

Level.Add(sizeMap[value.ToString()], test.ToString());

Comments

3

Please try to scope the case with curly braces this is just individual style but helps when lines of code grows up and also always use the default: too

case FEASIBLESIZE:
{
  Level.Add(TEAMSIZE, test.ToString());
  break;
}
default:
///...
break;

Comments

2

Your constants appear to be a candidate for Enum, I would go for Enum rather than const here....

3 Comments

good does it has any perfomance impacts? i think so can you explain the reason?
Based on your const values looks like they all describe Size so they are kind of a group so better to group them in an Enum. And I suppose Enum comparison in switch case should be faster.
@vettori doesn't Oded's solution impress you? If my logic allowed I would go for what he suggested...You could still go for Enum the Dictionary<string,string> becomes Dictionary<yourEnum,string>
1

Bad Programming Practice:

private const String FEASIBLESIZE = "Total FEASIBLESIZE";

Good Programming Practice:

private const String FEASIBLE_SIZE = "Total FEASIBLESIZE";

Better Programming Practice:

private const String FeasibleSize = "Total FEASIBLESIZE";

2 Comments

FEASIBLE_SIZE is a definite improvement over FEASIBLESIZE but AFAIK the recommended style for c# is FeasibleSize. stackoverflow.com/questions/242534/….
@AlanT: AFAIK FeasibleSize is to use with variables, and FEASIBLE_SIZE with constant.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.