|
select *
from dbo.Departments
where (accountnumber = @accountnumber and LEN(@accountnumber) >= 4)
or catid = @catid
order by deptname
Better?
I'm wondering what the problem is exactly... Except for the weird IF statement, use of * in production code and horrible formatting.
My blog[ ^]
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}
|
|
|
|
|
Sander Rossel wrote: I'm wondering what the problem is exactly...
The problem is that he's looking up the department by either account number or category ID. This is just bad practice and made worse by the fact (which I forgot to mention) that the PROC's name is "GetClientDepartments" which implies the list is being filtered by clients, not my accounts or categories.
There should be at least two PROC's, one called "GetDepartmentsByAccountNumber" and the other "GetDepartmentsByCategory". It's just plain laziness that he put the functionality for two very different things into one proc.
Now get this -- on the client side, he's always populating the department and catid values!
Marc
|
|
|
|
|
Well in that case it's A BIT weird.
I've worked with such procedures a lot. They usually were part of some bigger procedure. This, for example, could be part of getting a product, but depending on the department and available data and some customer setting and lots of other stuff you would ultimately get 1 out of maybe 10 possible outcomes.
Anyway, if you worked where I worked (notice past tense!) this wouldn't bother you as much as it does
My blog[ ^]
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}
|
|
|
|
|
|
manchanx wrote: Hopefully not your new client/employer?
No, thankfully. Haven't heard from them yet, this is code I'm cleaning up before (hopefully) I come on board with the other client.
Marc
|
|
|
|
|
Yeah, needs a CASE . IF has no place in SQL. He must not have been trained in set-based thinking.
|
|
|
|
|
I'd like to propose a simple extra switch to test Sanders code, while still be able to switch back to the old one.
if (select COUNT(*) from tblSettings where 'test' = 'true') = 0
begin
if LEN(@accountnumber) >= 4
begin
select * from dbo.Departments where accountnumber = @accountnumber order by deptname;
end
else
begin
select * from dbo.departments where catid = @catid order by deptname;
end
end
else
begin
select *
from dbo.Departments
where (accountnumber = @accountnumber and LEN(@accountnumber) >= 4)
or catid = @catid
order by deptname
end; You should replace the "4" with a value that you get from a settings-table ofcourse. You don't want to recompile your sproc simply because the length of catids' change. Better yet, replace it with the MIN(LEN(accountnumber)).
Bastard Programmer from Hell
If you can't read my code, try converting it here[^]
|
|
|
|
|
Well, having one stored procedure for each possible search parameter would be a REAL horror, IMHO. You can get to the hundreds of procedures really fast.
I would likely have more than one parameter in the sproc, and default values, so I could write:
EXEC GetClientDepartments @accountNumber = 'xxxxxxxx';
-- or...
EXEC GetClientDepartments @catId = 'xxxx';
t-SQL code can get really, really messy if you don't pay attention.
But, other than that, the code is not really an horror (t-SQL code quality is a lot lower than what you're expecting).
My 2c.
|
|
|
|
|
I was having a code review session and found out that the password is just being hashed and stored in the db. The db had a field for salt when i designed it and I specifically asked to use the salted hashes. So i suggested the developer to use the salted hash and he agreed. Later i asked him and he said he implemented it.
Now 3 months later I am working on some query optimization and during this i run a query on the user table and to my surprise the salt field contains "mmmmmm Salty passwords..." for all the records. And when i checked the code, the code contains this hard coded string in register action method (asp.net MVC).
Good thing is that the developer left the organization otherwise I would have been serving a life sentence for killing him.
|
|
|
|
|
Nothing's worse than a programmer with a sense of humour!
My blog[ ^]
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}
|
|
|
|
|
..in the case of humor the salt would be randomly pulled from a pre-seeded table, also containing Bacon and CListCtrl.
Bastard Programmer from Hell
If you can't read my code, try converting it here[^]
|
|
|
|
|
No comment needed...
If (InputBox("Access is highly restricted. Please enter the password: ") = "nooneneedstoseethisscreensokeepoutyoumonkey.") Then
"If you don't fail at least 90 percent of the time, you're not aiming high enough."
Alan Kay.
|
|
|
|
|
On a positive note, they've got enough characters and included punctuation
How do you know so much about swallows? Well, you have to know these things when you're a king, you know.
modified 31-Aug-21 21:01pm.
|
|
|
|
|
|
erm. the password to 'something in our network' (I wn't say what just in case!!!) is actually "correcthorsebatterystaple"
PooperPig - Coming Soon
|
|
|
|
|
Like.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
Ha! A few of our screens are protected with a password of WMTSU and some numbers. Easy to remember: we're making this stuff up!
"Go forth into the source" - Neal Morse
|
|
|
|
|
At my former employer we had a small config app with such a password baked into the code.
It ran with all our clients on many computers.
The regular user couldn't change the configs and we, and admins at the customers, always knew the password.
A bit more secure than a regular config file or a non-password protected config app.
It did the job!
I wouldn't use it on a website or anything though
My blog[ ^]
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}
|
|
|
|
|
Even in those circumstances, the password should at the very least be encrypted.
Further, should it become necessary to change the password, your app will have to be rebuilt. There are many, many better alternatives.
"If you don't fail at least 90 percent of the time, you're not aiming high enough."
Alan Kay.
|
|
|
|
|
Rob Grainger wrote: the password should at the very least be encrypted. Why? It wasn't a big secret. Just some local configuration...
Rob Grainger wrote: should it become necessary to change the password That was the beauty of it, the password was always the same!
Rob Grainger wrote: There are many, many better alternatives I know, I'd actually never use such a solution. I'm just saying it did the job of keeping users out and letting admins in
My blog[ ^]
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
}
|
|
|
|
|
In the following chunk of code:
service.SetClosingResultOfAtePolicy(this, terminationType);
The suggestion was "Add argument name 'terminationType' (Alt+Enter)", yielding:
service.SetClosingResultOfAtePolicy(this, terminationType: terminationType);
Because that really helps improve clarity, right?
"If you don't fail at least 90 percent of the time, you're not aiming high enough."
Alan Kay.
|
|
|
|
|
But of course! Not it is twice as clear as was before!
Skipper: We'll fix it.
Alex: Fix it? How you gonna fix this?
Skipper: Grit, spit and a whole lotta duct tape.
|
|
|
|
|
What's the full signature of SetClosingResultOfAtePolicy(...) though?
How do you know so much about swallows? Well, you have to know these things when you're a king, you know.
modified 31-Aug-21 21:01pm.
|
|
|
|
|
Er, not sure how that is relevant, but...
public void SetClosingResultOfAtePolicy(AtePolicy policy, TerminationType terminationType)
"If you don't fail at least 90 percent of the time, you're not aiming high enough."
Alan Kay.
|
|
|
|
|
It's relevant if you have optional parameters
In this case though, it looks like overkill.
How do you know so much about swallows? Well, you have to know these things when you're a king, you know.
modified 31-Aug-21 21:01pm.
|
|
|
|