|
Written by a former VB6 programmer? 
|
|
|
|
|
Why, yes, I have seen a project with everything that could be made static was made static, but they did have a reason. When I asked why all those methods were static, I was told "so that they can be called by an outside program". I didn't say they had a good reason. 
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
Not everything, but really a lot: WPF.
|
|
|
|
|
One place I interviewed at a few years ago showed me their code -- every class was a Singleton!  -- I didn't ask for the job.
|
|
|
|
|
Once upon a time
Manager: Show me your design diagrams.
Developer: Here they are but I am still working on it.
Manager: Where is the class for our front page.
Developer: (confused) That would be a static page so we dont need the classes.
Manager: OK now you work on task 2. i will get this one done by developer 2.
Later that day
Developer2: Is it complete
Manager: yes.
Developer2: where are the relationships between classes?
Manager: hmm, well all these classes and functions are static so we don't need them.
Developer2:WHAT?
Manager: yes now go and do it before I sack you.
|
|
|
|
|
My boss recently got the habit of making a 'helper class' for just about anything. Each helper class has one shared (static) function that is usually called in just one place in the code. Even the helper classes have helper classes and each class is a seperate file sometimes even in seperate folders... 
I am guessing he does this because classes now have a 'single responsibility'. Mixing up 'single method' and 'single responibility' is a common mistake I think. It especially doesn't hold true if all methods are shared (static). I'm also not about to argue about it with him. If he comes up with this kind of stuff you can imagine what kind of programmer he is: one that simply doesn't understand.
It's an OO world.
public class Naerling : Lazy<Person>{
public void DoWork(){ throw new NotImplementedException(); }
}
|
|
|
|
|
Shocking!
public class SysAdmin : Employee
{
public override void DoWork(IWorkItem workItem)
{
if (workItem.User.Type == UserType.NoLearn){
throw new NoIWillNotFixYourComputerException(new Luser(workItem.User));
}else{
base.DoWork(workItem);
}
}
}
|
|
|
|
|
Why use repeaters/datalists/grids when you can do it via a foreach loop and throw the whole string on a div.innerHTML? 
The item template never changes, and the guy keep doing it via code. 
Am i unlucky?
Look.
string html = "<table><tr>";
int cnt = 1;
foreach (DataRow reg in Data.Rows)
{
if (cnt > 3)
{
html+= "</tr><tr>";
cnt = 1;
}
html += "<td height='210px' valign='top' align='left'>" +
"<table align='left' height='100%' width='239px'>" +
" <tr>" +
" <td valign='top'>" +
@" <a class="Registroclick" href="javascript: void(0)" idseg="0" idcat=" + reg["some reg"].ToString() + " endereco='" + destpage+ "?idcat=" + reg["some reg"].ToString() + "'><img src='" + vid+ "destaque/" + reg["some reg"].ToString() + "' width='235' height='136' border='0'/></a><br/>" +
@" <a class="Registroclick" href="javascript: void(0)" idseg="0" idcat=" + reg["some reg"].ToString() + " endereco='" + destpage+ "?idcat=" + reg["some reg"].ToString() + "'>" + reg["some reg"] + "</a><br/><br/>" +
" </td>" +
" </tr>" +
"</table>" +
"</td>";
cnt++;
}
html += "</tr></table>";
divHtml.InnerHtml = html;
That's my bro, there's an classic asp tag to use in asp.net? Maybe he needs it.
It's a ascx with 150 lines. 100 lines on this method.
Also StringBuilder() and String.Format() seems to be useless.
Now imagine out how to update this layout;
return true;
|
|
|
|
|
Maybe he just wants tables, not the random crap (not working in some browsers) you can get from using an ASP control.
|
|
|
|
|
I'm reading a 790 line SQL sproc, a lot of poor coding peppered with extrainious begin/end statements. It starts early on with a "begin tran" lots of poor inserts and updates. Looks like SQL 2000 code, no try/catch logic, then the kicker:
End
IF @@ERROR <> 0
-- error statements using the first needed begin/end block
Else
Commit Tran
End
That was the first reference in the whole novel to @@ERROR! (Code created for Msft) 
For those who don't know SQL, that "End" before the "IF" FORCES it to always try to Commit the transaction, no matter how many errors the user has hit by now.
|
|
|
|
|
I have seen something like that. A big problem if it's in a high security thing.
return true;
|
|
|
|
|
Sounds like it was written by someone who doesn't really know SQL. They are only worth keeping around if they can be trained to do it the right way. If they argue that their way is perfectly fine, then chuck them out the door.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
One developer at work like to do things the hard way.
protected void ProcessDropDown(DropDownList myCombo, string myValue)
{
foreach (ListItem myItem in myCombo.Items)
{
if (myItem.Value == myValue)
{
myItem.Selected = true;
break;
}
}
}
All this just to select an item on aspx pages, should i throw him by the window?
And yes, this method is used only on the .cs file. So why use protected and do this thing? My eyes still hurt.
return true;
|
|
|
|
|
There are times when I am glad that 'the hard way' is still available. For one thing I don't like to be confined to using the one and only 'right way' to do things. Data binding may be the 'easy way', but also creates its own new problems. Just srearch for 'GridView' in Q&A. Besides that, data binding can be unacceptably slow in some cases. Filling controls directly can speed up things very much. And when I decide to do it 'the hard way' I also tend to keep all eggs in one basket and fill all controls that way, just to minimize the potential for misunderstandings.
Luckily, I have not been thrown by the window for doing that yet.
Edit: 'protected' is probably not the best choice in this case, but it does not do any harm.
At least artificial intelligence already is superior to natural stupidity
modified 10-May-12 3:22am.
|
|
|
|
|
|
This seems like he just didn't know about SelectedItem ... a harmless mistake, easily corrected.
|
|
|
|
|
That would have been too easy. But I think I came across some control without SelectedItem or SelectedIndex properties.
At least artificial intelligence already is superior to natural stupidity
|
|
|
|
|
Hopefully, that guy can be taught the correct way to do things. If not, then it is time to take him to that window. 
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
CIDev wrote: take him to that window
At least post a sign at the window: "This is the window out of which you will be thrown if you write bad code."
|
|
|
|
|
Found this little nugget just now (only the name of the stored procedure was changed):
Dim strSelect As String = "exec SomeStoredProcedure " & 1
|
|
|
|
|
Well, at least it's not vulnerable to SQL injection...
|
|
|
|
|
I suspect the original was, and the constant was put in there when a variable was no longer required, but they didn't want to put in the extra effort of placing the constant inside the string.
|
|
|
|
|
Could have been worse. It could have been C# and then you would have to do
String strSelect = "exec SomeStoredProcedure " + "1";
At least VB allows the 1 to be concatenated into the string.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
ryanb31 wrote: At least VB allows the 1 to be concatenated into the string.
Which is completely terrible that it allows you to do that. I also came across some code the other day that said something like this:
Dim str As String = "blah" & i + 1
In C#, you'd do this:
string str = "blah" + (i + 1).ToString();
Which I think is much more understandable.
|
|
|
|
|
Implicit conversions works equally well in VB and C#, AFAIK.
I would obviously use paratheses and ToString in VB as well, for clarity purposes.
It's basically as easy to code badly in C# as it is in VB.
In the end it's up to you.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
