Programming question - case fall through

[shouldbworking]

Writing away a magical application and I come across a situation where I want to use case fall through, but c# will not allow it if I have any content in the thing. I really dont see the evil in how im using it, but its made me use a goto statement for the first time in about 10 years of programming, so i thought id post up the code here to see if im missing an obvious way of doing this

{{{
switch (control.GetType().ToString())
{
case "System.Web.UI.WebControls.TextBox" :
//if blank we dont want to add it to the criteria so drop out early
if (((TextBox)control).Text == "&quot
{
break;
}
goto default;
//deliberate drop through
case "System.Web.UI.WebControls.DropDownList":
//if blank or please select we dont want to add it to the criteria so drop out early
DropDownList tempDDL = (DropDownList)control;
if ( tempDDL.SelectedValue == "" || tempDDL.SelectedValue == "Please select" )
{
break;
}
goto default;
//deliberate drop through
case "System.Web.UI.WebControls.CheckBox":
goto default;
//deliberate drop through
default :
//if its a search field then add it to the criteria.
if (((WebControl)control).Attributes["SearchFieldID"] != null)
{
addToQuery((WebControl)control);
}
break;
}
}}}

The wider context of that bit of code is that its part of a search form for a web application and as the database structure frequently changes I made a way of adding new search fields without having to add in any code, just add the html control onto the search form and a special id to indicate what field it refers to in the database.

Opinions? requests for further information?

Edited by shouldbworking on Friday 17th August 10:54

[fredf]

I don't know c#, so this is based on c

The goto's aren't providing a drop-through, a drop-through would work through the next case statement not jump to the default. Which do you want?

If you just want to jump to the default, move the default out of the switch statement, and put all the break's back in

Edited by fredf on Friday 17th August 11:20

[shouldbworking]

fredf said:

I don't know c#, so this is based on c

The goto's aren't providing a drop-through, a drop-through would work through the next case statement not jump to the default. Which do you want?

If you just want to jump to the default, move the default out of the switch statement, and put all the break's back in

Edited by fredf on Friday 17th August 11:20

The gotos are present because the language will not allow you to do a fallthrough if you have code in the case

[dern]

shouldbworking said:

Opinions?

Rewrite it using if statements or indeed any other method than the switch statement because that's an unreadable and unmaintainable mess imo. Use methods like ProcessTextBox() and ProcessDropdown() etc and then call AddFieldIfSearchField(list, thing) or something like that. It'll make it much more readable and people will be able to maintain it.

Also consider removing string literals and using the internationalisation features as having if(something == "Please select&quot is a bug waiting to happen. Also consider writing a method called IsFieldValueSupplied(field) because if(value == "&quot doesn't pick up cases like " ". And so on.

[shouldbworking]

dern said:

shouldbworking said:

Opinions?

Rewrite it using if statements or indeed any other method than the switch statement because that's an unreadable and unmaintainable mess imo. Use methods like ProcessTextBox() and ProcessDropdown() etc and then call AddFieldIfSearchField(list, thing) or something like that. It'll make it much more readable and people will be able to maintain it.

Also consider removing string literals and using the internationalisation features as having if(something == "Please select&quot is a bug waiting to happen. Also consider writing a method called IsFieldValueSupplied(field) because if(value == "&quot doesn't pick up cases like " ". And so on.

Thanks for the suggestions.

Im not going to change it from the switch statement format because id then be repeating the control.GetType, but I will break the default out into its own method and have the different cases call that.

I get what youre saying about the checking to see if fields are supplied. The strings describing the different types I wont be changing, as these will only change if some seriously drastic rewriting happened to the language class structure and even if it did happen, the software would just be run on a website using an older version of dotnet.

Thanks again

new code :
{{{

if( isFieldSupplied( control ) ) {
addToQuery( (WebControl)control );
}

}}}

The isFieldSupplied method still does the checking for all controls determining type via a case statement at the moment. Depending on how complicated that gets I may break it out into various overloads of it.

ps. PH code formatting hates me

Edited by shouldbworking on Friday 17th August 12:45

[ginettag27]

Or possibly enclose it in a single IF statement, to cover the fallthrough?? Admitteldy a bit of a faff... but should get the desired results (although I've only come from C/C++ and 4GLs, which would give you what you want anyway!!!)

[dern]

shouldbworking said:

Im not going to change it from the switch statement format because id then be repeating the control.GetType

If you don't want to do this because of perceived performance issues then I suspect the compiler would resolve that for you but I'd assign it to a variable called controlType then you can perform the if tests without any problem and you'd make the code substantially more readable. If you didn't convert your types to strings but compared the types themselves then you'd actually make the code quicker *and* more readable.

[sp60]

Your trying to make a case statement do something it wasn't designed to do!
It does match your criteria as you request but then your trying to make it fall through based on a sub criteria, which isn't the way they work. It either matches or defaults. It doesn't make sense for it to match and then default, which is why it forces you to break out.
I have to agree that this logic seems odd to me, but then everyone has a different approach when it comes to coding, which is no bad thing.

Personally I'd do something like:

foreach (WebControl con in this.Controls) {
if (((WebControl)control).Attributes["SearchFieldID"] != null) {
if (con is TextBox && ((TextBox)con).Text != "&quot {
addToQuery( (WebControl)con );
}
if (con is ComboBox) {
etc....

}
if (con is CheckBox ){
etc...
}
}
}

This (to me) is cleaner and easier to read, omits all the ones without the searchfield attributes first then starts the identification of types.
There are lots of ways of doing this though!

[annodomini2]

dern said:

shouldbworking said:

Im not going to change it from the switch statement format because id then be repeating the control.GetType

If you don't want to do this because of perceived performance issues then I suspect the compiler would resolve that for you but I'd assign it to a variable called controlType then you can perform the if tests without any problem and you'd make the code substantially more readable. If you didn't convert your types to strings but compared the types themselves then you'd actually make the code quicker *and* more readable.

To keep it in Switch-case you would have to do the following:

switch (control.GetType())
{
case System.Web.UI.WebControls.TextBox:
//if blank we dont want to add it to the criteria so drop out early
if (!(TextBox)control).Text == "&quot&quot
{
//if its a search field then add it to the criteria.
if (((WebControl)control).Attributes["SearchFieldID"] != null)
{
addToQuery((WebControl)control);
}
}
break;

case System.Web.UI.WebControls.DropDownList:
//if blank or please select we want to add it to the criteria
DropDownList tempDDL = (DropDownList)control;
if (!(tempDDL.SelectedValue == "" || tempDDL.SelectedValue == "Please select&quot)
{
//if its a search field then add it to the criteria.
if (((WebControl)control).Attributes["SearchFieldID"] != null)
{
addToQuery((WebControl)control);
}
}
break;

default:
//if its a search field then add it to the criteria.
if (((WebControl)control).Attributes["SearchFieldID"] != null)
{
addToQuery((WebControl)control);
}
break;
}

The check box thing is irrelevent if you are going straight to default as it will always be picked up.

Yes the code is slightly larger, but guarranteed to work.

[shouldbworking]

Yep its all changed around now I got halfway through changing it and then just kind of kept going as felt right and ended up with err. what dern suggested in the first place

Must just be having a dizzy coding day!

[dern]

annodomini2 said:

To keep it in Switch-case you would have to do the following:

switch (control.GetType())

What I meant was...

controlType = control.GetType();
switch(controlType)

...and then you wouldn't.

While this is pointless for this example the thing we were talking about was something like...

controlType = control.GetType();
if(controlType == something) {
...
} else if(controlType == somethingElse) {
...
}

...which removed the need to call GetType more than once (although imo the compiler would optimise this for you if it's worth it's salt but it's good practice not to make the assumption).

Edited by dern on Friday 17th August 17:11