The Weird and The Wonderful

12-Aug-11 3:47

Sadly, the sucker never heard of the Convert class...

Re: Try Catch - at least it won't break

GibbleCH15-Aug-11 5:28

GibbleCH

15-Aug-11 5:28

or TryParse ...

Re: Try Catch - at least it won't break

BillW3322-Aug-11 6:54

BillW33

22-Aug-11 6:54

Seeing far too much code like that is what made me choose my signature line.

Just because the code works, it doesn't mean that it is good code.

if .... else case

Colin Mullikin11-Aug-11 2:31

11-Aug-11 2:31

Delphi

if (FileType = 0) then
    FileSaveAs(Sender)
  else begin
    case FileType of
      1:SaveCDL(FileName,List);
      2:SaveDXF(FileName,List);
      3:SaveFile(FileName,List);
    end; {case FileType}
  end; {if (FileType = 0) else ...}

I came across this in some code (written by someone else) that I am looking through to find the cause of a save/load error. Sigh | :sigh:

The United States invariably does the right thing, after having exhausted every other alternative. -Winston Churchill

America is the only country that went from barbarism to decadence without civilization in between. -Oscar Wilde

Wow, even the French showed a little more spine than that before they got their sh*t pushed in.[^] -Colin Mullikin

BobJanova11-Aug-11 4:05

BobJanova

11-Aug-11 4:05

Nice one!

Lutosław11-Aug-11 23:25

Lutosław

11-Aug-11 23:25

Is it about

FileType=0

being an assignment instead of comparison?

Greetings - Jacek

TorstenFrings12-Aug-11 1:13

TorstenFrings

12-Aug-11 1:13

no, this is the VB way of comparison

Lutosław12-Aug-11 1:44

Lutosław

12-Aug-11 1:44

It seems to be Pascal, not VB. But, in Pascal = is also a comparison, AFAIR. Frankly, I don't see where the horror is.

Greetings - Jacek

TorstenFrings12-Aug-11 1:56

TorstenFrings

12-Aug-11 1:56

omg yes it's PASCAL ... guess the horror is the fact, that there is no need for the outer IF, because it could be handled inside the CASE as well

Colin Mullikin14-Aug-11 18:27

14-Aug-11 18:27

TorstenFrings wrote:
no need for the outer IF, because it could be handled inside the CASE

Yeah, that's it. Having the if instead of just case 0 seems a bit like someone wasn't actually thinking about what they were coding.

Colin Mullikin14-Aug-11 18:28

14-Aug-11 18:28

Jacek Gajek wrote:
It seems to be Pascal, not VB.

It's Delphi. So, you were almost right. Thumbs Up | :thumbsup:

Vladimir Svyatski18-Aug-11 9:42

Vladimir Svyatski

18-Aug-11 9:42

Colin Mullikin wrote:
It's Delphi. So, you were almost right.

He was absolutely right. Delphi is not a programming language, it's IDE just like Visual Studio. Object Pascal is used as a programming language inside. I know what I'm talking about. I had the pleasure of working with Delphi/C++ Builder. It was more than 5 years ago though. I still remember C++ Builder 6. But Borland is dead now Frown | :(

Vladimir Svyatski18-Aug-11 9:30

Vladimir Svyatski

18-Aug-11 9:30

Oh, sh*t, Pascal Smile | :)

Haven't seen it for several years. Is Delphi still alive? Anyway wouldn't it be easier to write something like

Delphi

case FileType of
      0:FileSaveAs(Sender);
      1:SaveCDL(FileName,List);
      2:SaveDXF(FileName,List);
      3:SaveFile(FileName,List);
end;

Or religion doesn't allow? Sorry for possible syntax mistakes. But like I said: I haven't seen it for several years.

modified 6-Aug-12 16:22pm.

Colin Mullikin18-Aug-11 14:05

Every line makes me die a little more...

18-Aug-11 14:05

Yep, you hit the nail on the head with that one. I have no clue why the original programmer didn't do that in the first place. I changed it to that, since it looks much cleaner.

leppie11-Aug-11 0:37

11-Aug-11 0:37

WTF~!!!!!!!!!!! WTF | :WTF:

var key = "13870";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13886";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13905";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13910";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13918";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13922";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13927";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13930";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13948";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13965";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "13986";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

var key = "14002";
var checkboxName = key + "_Selected";
var commentName = key + "_Comments";
if(document.getElementById(checkboxName).checked == true)
{
   document.getElementById(commentName).value = commentBox.value;
   appliedToCount ++;
}

Update:

This code was in fact generated... Refactoring, thy name is frailty!

Shameel11-Aug-11 6:03

Shameel

11-Aug-11 6:03

leppie wrote:
.checked == true

This is an instance of shame within a shame.

GibbleCH11-Aug-11 8:48

GibbleCH

11-Aug-11 8:48

I presume each of these duplicated sections was in it's own method, otherwise it won't even compile. Since each var statement keeps declaring variables that are already declared.

leppie11-Aug-11 9:10

11-Aug-11 9:10

It is acceptable javascript.

Brady Kelly11-Aug-11 10:29

Brady Kelly

11-Aug-11 10:29

leppie wrote:
This code was in fact generated...

by a generator written by a tool.

Allan Thomas12-Aug-11 2:18

Allan Thomas

12-Aug-11 2:18

How about a 13 month KPI that has code that has logic to work out the summary values to be displayed for each month, storing values used elsewhere in seperate variables (not an array) and then having that code duplicated 13 times (with a slight change in variables/controls). It would have been at least 10 pages and all embedded in the click event of the button Frown | :(

.

After checking the code to see if anything else was buried in there I refactored this (with the help of a few functions) into two loops with about a dozen lines total. It helped because a bit later I found two other places where the same thing happened and while the code was different I was able to copy and paste the code and update the loop for the different KPI's.

leppie12-Aug-11 3:45

12-Aug-11 3:45

Exactly what should have been done.

I see the same pattern as you described.

They use jQuery, so I dont understand why they can't even make it a one liner...