|
Thomas Weller wrote: NEVER ever throw an exception to code a foreseeable condition - even if it might be rare.
I guess it depends what you mean by 'foreseeable'. Certainly if one could readily determine before performing an operation that it would throw an exception that one was planning on catching, and if there was any significant likelihood of such exception occurring, doing an early test before attempting the operation would be a good idea. Testing the length test on the array, for example, would seem appropriate, though in some applications that may be just about the only exception that could be guaranteed not to occur.
On the other hand, it may not be possible to determine in advance all exceptions that might occur. In the code sample given, the '.Value' property could throw an exception (e.g. ObjectDisposed or whatever) even if the length of the array was valid. While a general 'Catch' would be icky, there are times when such a thing is appropriate. For example, in expression-evaluation window, nearly all exceptions thrown during expression evaluation should be caught. An application shouldn't die just because someone typed in an expression that caused an exception the application programmer hadn't foreseen.
I don't particularly like -1 as a return value, but the overall style would seem appropriate for some applications.
|
|
|
|
|
supercat9 wrote: On the other hand, it may not be possible to determine in advance all exceptions that might occur.
Of course not. In that case we wouldn't need exceptions at all...
What I mean is: 'Foreseeable' in terms of business/application logic. If something totally unexpected happens (like e.g. the 'ObjectDisposed' you mentioned), there should be a general top level exception handler at the application level, but for sure such an exception must never be swallowed silently by the executing code. Not in a million years, in no circumstance whatsoever!
supercat9 wrote: I don't particularly like -1 as a return value
Same with me, but in the case of my example -1 has a clear, discrete meaning that can de documented (less than 4 items -> computation not possible), while in the original snippet -1 means sth. went wrong (and I will never tell you what the problem was...). That's a huge difference - especially on large business-scale software projects, where you might have hundreds of such properties...
Regards
Thomas
www.thomas-weller.de
Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the Universe trying to produce bigger and better idiots. So far, the Universe is winning. Programmer - an organism that turns coffee into software.
|
|
|
|
|
If something totally unexpected happens (like e.g. the 'ObjectDisposed' you mentioned), there should be a general top level exception handler at the application level, but for sure such an exception must never be swallowed silently by the executing code. Not in a million years, in no circumstance whatsoever!
That depends what one is trying to do. Would people like Visual Studio very much if it died any time an exception occurred while evaluating expressions in the Watch window? Or is the proper action to show the problematic expression with an indication that it cannot be evaluated?
If a program is supposed to graph a function, it may at times be unclear the best way to handle an exception. If one is trying to graph an expression like f(x)=1/x, it's best that the problematic case at x=0 be swallowed, while graphing is allowed to continue on the other side. On the other hand, if the function is one which will always throw an exception, calling the function 1,000 times while drawing the graph will cause 1,000 exceptions to be thrown.
Swallowing exceptions in a fashion that hides any indication of what type of exception occurred seems like a bad idea, but it would seem there are many situations where a calling application is merely interested in whether or not an operation worked, and not what went wrong if it didn't.
|
|
|
|
|
supercat9 wrote: Would people like Visual Studio very much if it died any time an exception occurred while evaluating expressions in the Watch window? Or is the proper action to show the problematic expression with an indication that it cannot be evaluated?
I'm not sure what your argument is here.
VS dying under those circumstances isn't "swallowing the exception", it is ignoring it completely.
VS reporting the problem isn't swallowing the exception either, it is processing it and reporting the existance of a problem - exactly what exceptions are there for.
Swallowing an exception is (IMHO) "pretending it didn't happen" and carrying on regardless.
If a problem occurs saving my work every ten minutes and the exception gets swallowed, I am going to be a very unhappy bunny when I find out later it was all lost!
No trees were harmed in the sending of this message; however, a significant number of electrons were slightly inconvenienced.
This message is made of fully recyclable Zeros and Ones
|
|
|
|
|
VS reporting the problem isn't swallowing the exception either, it is processing it and reporting the existance of a problem - exactly what exceptions are there for.
The "-1" return seems overly vague as to what problem occurred when computing the data, but it does indicate that a problem occurred. I would not expect the caller of a function like the one given to ignore the return value. It may not process it particularly as an error, but it would probably store it someplace.
Suppose the objective is to produce a table showing various data associated with a bunch of people; for whatever reason, certain data will be unavailable for certain people. Exceptions would be appropriate if one's objective was to leave unprocessed any person for whom some information could not be retrieved correctly. Return codes are more appropriate if the objective is to produce a table in which computable values are filled in and uncomputable values show "N/A".
|
|
|
|
|
If I see such a thing i can say only one thing:
"Everybody please remain calm! You Sir!! Get your hands up in the air and step away from the keyboard!"
Learn from the mistakes of others, you may not live long enough to make them all yourself.
|
|
|
|
|
Lol...good one
Unfortunately, I've come across so many such instances of bad programmers in my career (and I'm sure most of you have as well). This fortunately for me has been the only instance where the developer has understood what they've done wrong and positively taken on board the recommendations I've made.
I actually came across this while doing a code review for a team that I had not worked with before.
|
|
|
|
|
I don't like returning -1 for an error (particularly in this case) because -1 could be a valid result. eg. the values are all -1. There are a couple of nice solutions I can think of. One would be to use a nullable type ie. double? . The other way would be to let an exception be thrown or throw your own exceptions.
Only looking at the first four elements is probably the worst thing IMHO though. What do you think of this?
public double CrappyTotal
{
get
{
double total = 0;
foreach (var d in this.SomeItem.Details)
{
total += d.Value;
}
return total / (double)this.SomeItem.Details.Count;
}
}
No exception handling . I would be happy without error handling. I could add a check for nulls at the start, but I don't think it's worth the time. It would probably depend how and where it was being used though.
So, are there people out there who insist that every exception must be handled?
|
|
|
|
|
Yes,
That is one of the ways I suggested, but also added that if you are calculating an average, then the name should probably reflect that as well
|
|
|
|
|
|
this is one of the many gems I'm finding (and fixing) in some third party produced code:
protected void btnLogin_Click(Object s, EventArgs e)
{
bool loginOK = false;
try
{
loginOK = Account.LoginUser(Page, txtUserName.Text, txtPassword.Text);
}
catch (Exception ex)
{
string error = string.Empty;
if (ex.Message == "Invalid attempt to read when no data is present.")
{
error = "Username not found.";
}
else
{
error = ex.Message;
}
lblMessage.Text = error;
return;
}
if (loginOK == true)
{
Response.Redirect("~/Default.aspx");
}
else
{
lblMessage.Text = "Password does not match.";
}
}
public static bool LoginUser(Page page, string uname, string pass)
{
bool passwordVerified = false;
try
{
passwordVerified = AccountDB.CheckPassword(uname, pass);
}
catch (Exception ex)
{
throw;
}
if (passwordVerified == true)
{
string roles = "JobSeeker";
FormsAuthenticationTicket authTicket = new
FormsAuthenticationTicket(1,
uname,
DateTime.Now,
DateTime.Now.AddMinutes(60),
false,
roles
);
string encryptedTicket = FormsAuthentication.Encrypt(authTicket);
HttpCookie authCookie = new HttpCookie(FormsAuthentication.FormsCookieName, encryptedTicket);
page.Response.Cookies.Add(authCookie);
int userID = AccountDB.GetUserIDByUsername(uname);
AccountDB.UpdateLoginDate(userID, DateTime.Now);
return true;
}
else
{
return false;
}
}
public static bool CheckPassword(string username, string password)
{
bool passwordMatch = false;
SqlConnection conn = new SqlConnection(ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString);
SqlCommand cmd = new SqlCommand("CheckPassword", conn);
cmd.CommandType = CommandType.StoredProcedure;
SqlParameter sqlParam = cmd.Parameters.Add("@userName", SqlDbType.VarChar, 255);
sqlParam.Value = username;
try
{
conn.Open();
SqlDataReader reader = cmd.ExecuteReader();
reader.Read();
string dbPasswordHash = reader.GetString(0);
string salt = reader.GetString(1);
reader.Close();
string hashedPasswordAndSalt = Account.CreatePasswordHash(password, salt);
passwordMatch = hashedPasswordAndSalt.Equals(dbPasswordHash);
}
catch (Exception ex)
{
throw;
}
finally
{
conn.Close();
}
return passwordMatch;
}
I'm not sure what's worse, that a professional development company has people who think this is how you use exceptions, or that my company actually paid money for this code
I love the way they put database errors in the message to the user, and identify which they got wrong, the username or the password.
modified on Wednesday, July 8, 2009 12:55 PM
|
|
|
|
|
What did you do wrong to deserve this? Did you destroy Sealand?
On another note, the comments in your code block stretch the screen. Could you please fix them?
Between the idea
And the reality
Between the motion
And the act
Falls the Shadow
|
|
|
|
|
fixed the comments, sorry
I think it must be bad karma, the whole site is like this (they were the lowest bidder).
It does function, and there are pieces that are seem good, but then I come across stuff like this and I really want to bang my head against the wall.
|
|
|
|
|
I am not familiar with db connection issues. Could you explain why is it bad, please?
icewolf_snowfire wrote:
SqlDataReader reader = cmd.ExecuteReader();
reader.Read();
Thanks.
Greetings - Jacek
|
|
|
|
|
when you use a SqlDataReader, you always have to check if it actually contains any data with reader.HasRows if it doesn't have data, like in this case if the username is not in the database, it throws an InvalidOperationException "Invalid attempt to read when no data is present"
what's happening is the person who wrote this, didn't understand what was causing the exception, so he just handled in with a try catch, rather than fixing the actual problem.
|
|
|
|
|
I get it. It seems to be another case of an "exceptional coding". Moreover, it rethrows the exception so the overlaying method will get a plain InvalidOperationException with no clue what is going on, as far as I understand the throw; syntax. Terrifying.
Greetings - Jacek
|
|
|
|
|
icewolf_snowfire wrote: reader.HasRows
Wouldn't if (reader.Read()) { ... } be ok too?
xacc.ideIronScheme - 1.0 beta 3 - out now! ((lambda (x) `((lambda (x) ,x) ',x)) '`((lambda (x) ,x) ',x))
|
|
|
|
|
you're right it would, I didn't realize that was there. (and neither did they)
it would be less lines of code, so slightly more efficient?
|
|
|
|
|
icewolf_snowfire wrote: it would be less lines of code, so slightly more efficient?
lol.
Greetings - Jacek
|
|
|
|
|
I've always used while (reader.Read()) { ... } which does the trick.
Mark Brock
"We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
Click here to view my blog
|
|
|
|
|
I'm not sure what's worse, that a professional development company has people who think this is how you use exceptions, or that my company actually paid money for this code Unsure
I love the way they put database errors in the message to the user, and identify which they got wrong, the username or the password.Mad
In many situations, it's entirely reasonable to distinguish a bad username from a bad password. User names are generally not secure, and legitimate users may not always remember which variation of their username they used at a particular site.
Having a login routine throw an exception for user-not-found is not the best, but if a custom exception were used for that purpose, it wouldn't be totally horrible. The only really horrible thing I see is the munging of the exception message.
BTW, one feature I'd like to see on a web site would be an option for users to specify a string that should be displayed on an unsuccessful login attempt, with the instruction that the string should contain something recognizable, but should not contain any security-related information. That would allow someone who mistakenly tries to log in with someone else's username to immediately realize their mistake.
|
|
|
|
|
It would also be nice if sites told you what the rules for passwords were so that you knew which passwords you were likely to have used on a given site. Often I've had to try to create a new account to find out what the rules are for a site so I can login again. Life was so much easier before websites started getting themselves removed from BugMeNot!
|
|
|
|
|
Russell Jones wrote: It would also be nice if sites told you what the rules for passwords were so that you knew which passwords you were likely to have used on a given site. Often I've had to try to create a new account to find out what the rules are for a site so I can login again. Life was so much easier before websites started getting themselves removed from BugMeNot!
No kidding. If a site requires passwords to be precisely eight characters, how is it any less secure to remind people of that at the login screen than after they create a new account? (Of course, requiring that passwords be exactly eight characters seems a dumb design anyway--even if the system only had space to store eight bytes, and policy factors dictated an eight-character minimum, the system should easily be able to hash a password of arbitrary length into an eight-byte digest or--failing that--just take the first eight bytes of the password and ignore the rest).
|
|
|
|
|
I've seen the exact same crap from a third party 'development company' my employer has recently stopped using.
I ended up submitting report after report on how bad their code was ... finally got listened to and we promptly dumped them.
|
|
|
|
|
I don't think these are the bad Idea or junk or crap etc. whatever you called it.
It just the way of programming. And it the way the programmer want it to be.
One Algorithm can be done in many way.
So If you think you can write a better one, You should not shout into their face an say something Like
"Your code is bad. I found this junk in your code. I am the best." Impressive Huh!?
What you should do is give them a suggestion, Though it free
|
|
|
|
|