|
It can be done 'better' (I think I have done up to 4 with that style before). I would likely land up using a switch for all the == cases, and have the < cases in the default case (using the ternary operator style you laid out).
Otherwise, I don't really like the ordered map. Another way to do it would be to:
int[] screenDeltas = new[] { 1, 1, 1, 1, 1, 1, 1, 1, 1, 4, 4, };
iScreen += iScreen < 0 ? 1 :
iScreen > 42 ? 212 :
screenDeltas[iScreen];
(Clearly code would be used to generate that array initializer)
Because array lookups are O(1) and I love my premature optimization.
He who asks a question is a fool for five minutes. He who does not ask a question remains a fool forever. [Chineese Proverb]
Jonathan C Dickinson (C# Software Engineer)
|
|
|
|
|
I don't see the problem, just look at the comments...
____________________________________________________________
Be brave little warrior, be VERY brave
|
|
|
|
|
The code quality is reminiscent of the legacy code I used to maintain that was written in the 70's and early 80's, before Ed Yourdon et. al. started talking about structured programming, which is itself a very old paradigm. I count a cyclomatic complexity of 51 (academia says 10 is "maintainable", really 18 or so if you stretch it) and it's full of "magic numbers" that have no obvious meaning other than they appear to be "screen numbers". It is kind of depressing to see that kind of code being written in a modern language, 30 years after the first efforts were made to educate people out of these kinds of practices.
|
|
|
|
|
I believe Yourden wrote about structured programming in the late 1960's, and it was well known to professionals then. Your legacy code writers had no excuse. (Like they ever do...)
There must be a selection-bias thing at work here wherein well-written legacy code doesn't need to be maintained, because it's well-written, and the legacy code that needs to be maintained, needs maintenance because it is bad code.
The other possibility is intolerable; that there are some kids in middle-school today who will be cursing our beautiful code in 20 years. That would mean that even great coders aren't great every day. Just because you're Mozart doesn't mean people will remember ALL your music. Lots of it was produced under deadline pressure, and was only ok.
|
|
|
|
|
Wouldn't it be easier to bind the values of each item in the checkbox to the value this thing would produce and then just take the SelectedValue?
|
|
|
|
|
Yes, or at least bind the items to an object, of which this magic number was a property.
|
|
|
|
|
BobJanova wrote: Yes, or at least bind the items to an object, of which this magic number was a property.
That is exactly what I did when I had the pleasure of encountering this code while creating the next generation of the application.
I employed a java enum style class in C#.
public class Display
{
public static readonly Display OpeningSplashdisplay =
new Display(1, "Opening Splash display");
public static readonly Display StartUpDateConfig =
new Display(2, "start up date config");
public static IEnumerable<Display> Values
{
get
{
object temp = null;
FieldInfo[] fields = typeof(Display).GetFields();
foreach (FieldInfo field in fields)
{
temp = field.GetValue(null);
if (temp is Display)
yield return (Display)temp;
}
}
}
private readonly byte value;
private readonly string name;
Display(byte value, string name)
{
this.value = value;
this.name = name;
}
public byte ToByte() { return value; }
public override string ToString() { return name; }
public static Display ParseByte(byte value)
{
foreach (Display d in Display.Values)
{
if (d.ToByte() == value)
return d;
}
return Display.OpeningSplashdisplay;
}
}
Here I load the combobox from the data structure.
cmbDisplay.Items.AddRange(new List<Display>(Display.Values).ToArray());
And this is how I handle the selection
private void cmbDisplay_SelectedIndexChanged(object sender, EventArgs e)
{
ComboBox c = (ComboBox)sender;
Display selected = (Display)c.SelectedItem;
_RawData[2] = selected.ToByte();
}
|
|
|
|
|
First, send my congratulations to your fellow, because he have a superior understanding of the use of the ternary operator, being smart (and brave) enough to nest that number of levels (the max i can nest is 4 whitout being lost in the logic). Second, slap him in the face, because this is a nightmare to maintain and a shame to see in any program, given that even a humble switch-case would have been better than this mess.
|
|
|
|
|
Bill Gates is a very rich man today... and do you want to know why? The answer is one word: versions.
Dave Barry
Read more at BrainyQuote[ ^]
|
|
|
|
|
The following code is copy/pasted verbatim from a live production environment (the entire project/solution is on the production file system, side-by-side with the executing code):
Private Function GrabDataSet(ByVal sSQL As String) As DataSet
Try
Dim oDAdapt As SqlClient.SqlDataAdapter
Dim oRs As New DataSet
Dim cmdSQL As String = sSQL
oRs = New DataSet
oDAdapt = New SqlClient.SqlDataAdapter(cmdSQL, oConn)
oDAdapt.Fill(oRs)
Return oRs
oRs.Dispose()
oDAdapt.Dispose()
Catch ex As Exception
End Try
End Function
Found this in a web service I'm upgrading. It's hard to find anything NOT wrong with this.
|
|
|
|
|
It's funny that it's called GrabDataSet instead of GetDataSet. What are you gonna do? Squeeze it until the data flows out.
Chris Meech
I am Canadian. [heard in a local bar]
In theory there is no difference between theory and practice. In practice there is. [Yogi Berra]
posting about Crystal Reports here is like discussing gay marriage on a catholic church’s website.[Nishant Sivakumar]
|
|
|
|
|
I feel that "Strangle" would be the more appropriate verb.
|
|
|
|
|
He he he
Chris, The best comeback I have for that is "But ... Get did not work "
|
|
|
|
|
AspDotNetDev wrote: It's hard to find anything NOT wrong with this
M, actually no. I think there are things done right in there. What's wrong with this?
Dim oDAdapt As SqlClient.SqlDataAdapter
or this?
Dim cmdSQL As String = sSQL
And the author catches ALL exceptions, isn't that incredible?
And the user doesn't even know something bad occurred, how cool is that?
[Ok, ending sarcasm now ]
Full-fledged Java/.NET lover, full-fledged PHP hater.
Full-fledged Google/Microsoft lover, full-fledged Apple hater.
Full-fledged Skype lover, full-fledged YM hater.
|
|
|
|
|
I'm always curious as to why people insist on declaring ALL variables inside a try/catch block. It's not as though declaring the first three variables there are likely to cause an exception (nor that instantiating the DataSet and SqlDataAdapter will cause problems).
|
|
|
|
|
I'm fairly certain the person that made this code did not consider such things. As an example:
Dim oRs As New DataSet
oRs = New DataSet
An instance is created, then shortly after another instance is created. And I really don't like when the parens aren't put after calling a constructor.
|
|
|
|
|
AspDotNetDev wrote: I really don't like when the parens aren't put after calling a constructor
I seem to remember reading somewhere that the VB editor removes the parens from the default constructor. That was an old version, but it wouldn't surprise me if this behaviour was still present.
|
|
|
|
|
Just tested it.
VB doesn't remove them but it doesn't add them neither so if you type them yourself they'll stay there.
|
|
|
|
|
VB only requires parens when it's necessary to add parameters. Otherwise, you can slip by without them.
XAlan Burkhart
|
|
|
|
|
Whaddyaknow
Thats a straight copy from MSDN
|
|
|
|
|
How can you trust a machine?
"Fear no factor", Prime Numbers.
|
|
|
|
|
Aside from redundant variable declarations and swallowed exceptions, I enjoy the way the author disposes of the DataSet and DataAdapter after the method has returned. It's the only way to be sure..
Also: VB.net and inherent naming convention make me grieve for the the 18 months I lost before I moved to C# and started calling things by names that infer even the slightest insight to their usage.
|
|
|
|
|
Trying to open a MessageBox in a webservice in case of an exception is also not the brightest idea. It was probably commented out because of 'unexpected' side effects, like somebody having to go to the server to click it away
At least artificial intelligence already is superior to natural stupidity
|
|
|
|
|
I've seen the MessageBox-without-a-GUI before, in a windows service but I didn't know it would "work" in a webservice too. Funnily enough, I've never tried this
|
|
|
|
|
I might use this as a test question for students to identify all mistakes. It would worth an easy 5 marks. I love the hint that the connection is opened at some stage and then left open. Bonus mark for picking that up. In fact maybe there whole ADO.NET component of the test could be that one question
"You get that on the big jobs."
|
|
|
|