|
I'm working on a maintenance project for a client. It's ASP .NET MVC. When exporting some charts, they use an HTML To Image converter. I found this today:
if (Session["tWidth"] != null)
{
htmlToImageConverter.BrowserWidth = int.Parse("1025");
}
I've seen similar posts here before. Why call int.Parse() and pass what's obviously an integer, instead of simply assigning: 1025? Why, oh why?
djj55: Nice but may have a permission problem
Pete O'Hanlon: He has my permission to run it.
|
|
|
|
|
And what is the tWidth for? Is it an integer? Or just some random value?
Getting information off the Internet is like taking a drink from a fire hydrant.
- Mitchell Kapor
|
|
|
|
|
I had a difficult time understanding that as well. 'tWidth' is, in fact, an integer. In other places throughout the project it's used to store the width of certain widgets in a report. But this particular statement, I have no idea. That's literally all the code in it. It never references 'tWidth' in this particular section. *smh*
There are a ton of bad practices in this project. A lot of empty 'catch' statements after a large 'try' block and so forth. Almost *ZERO* comments. Blah, blah, blah.
djj55: Nice but may have a permission problem
Pete O'Hanlon: He has my permission to run it.
|
|
|
|
|
Getting information off the Internet is like taking a drink from a fire hydrant.
- Mitchell Kapor
|
|
|
|
|
It may be for those special people with a 1025 x 768½ screen resolution?
|
|
|
|
|
The reason behind that WTF is likely simple: somewhen during development, there was a problem, or an extreme value case had to be tested (width greater than screen width), and a developer decided to replace temporarily
htmlToImageConverter.BrowserWidth = int.Parse(tWidth);
by
htmlToImageConverter.BrowserWidth = int.Parse("1025");
But then forgot to change it back. And nobody complained till you happened to find that WTF...
|
|
|
|
|
Quote: But then forgot to change it back. You're probably right; however, that takes all the fun out of harassing the previous developer for this.
There are only 10 types of people in the world, those who understand binary and those who don't.
|
|
|
|
|
That's a very reasonable observation, and you're probably quite right. Thank you for that. Now I'm going to put "tWidth" back in there and see if it fixes some of these problems. Haha.
djj55: Nice but may have a permission problem
Pete O'Hanlon: He has my permission to run it.
|
|
|
|
|
Be careful! It might introduce a new defect now - do you know how many later changes assume that that WTF is the correct solution?
|
|
|
|
|
Haha. I changed it and it didn't make any difference whatsoever.
djj55: Nice but may have a permission problem
Pete O'Hanlon: He has my permission to run it.
|
|
|
|
|
You lack experience with such WTF code! In some two or three years, a customer will complain about a page being too wide or too narrow...
And then the new guy working on the project will submit your little change as a WTF: a useless change introducing a new bug.
That's how such WTF code works...
|
|
|
|
|
I do lack experience. This is the first project I've ever worked on that was written by someone else.
djj55: Nice but may have a permission problem
Pete O'Hanlon: He has my permission to run it.
|
|
|
|
|
Bernhard Hiller wrote: and a developer decided to replace temporarily
You seem to know about it very well. I can almost pinpoint who that was.
Signature construction in progress. Sorry for the inconvenience.
Damn you have the perfect signature - CBadger
|
|
|
|
|
Oh dear, I'd better use a different login name for such posts...
|
|
|
|
|
that's easy to fix, just change it to:
int.Parse(int.Parse("1025").ToString());
|
|
|
|
|
djj55: Nice but may have a permission problem
Pete O'Hanlon: He has my permission to run it.
|
|
|
|
|
This is from another forum I visit once in a while, somebody posting a code snippet:
void loop(){
byte rxbyte; byte temp; rxbyte = serial_getch();
if (rxbyte == 254) {
switch (serial_getch()) {
And it continues on like that. At least we know what the 'compiler' is doing when you declare a variable
|
|
|
|
|
You'd think they'd spell check it.
|
|
|
|
|
I see not speelign errors!
Mind you, I like the quotes round "compiler" - as if they aren't quite sure it is a compiler, it might be a fridge or something like that...
I do rely on spell checkers a lot - I even have a VS extension for string and comment checking - but I like the Chrome one, especially when it picks up grammar problems!
The only instant messaging I do involves my middle finger.
English doesn't borrow from other languages.
English follows other languages down dark alleys, knocks them over and goes through their pockets for loose grammar.
|
|
|
|
|
|
A pure spelling checker wouldn't spot that - you need a grammar checker as each word is correctly spelled.
And yes, I missed it completely!
The only instant messaging I do involves my middle finger.
English doesn't borrow from other languages.
English follows other languages down dark alleys, knocks them over and goes through their pockets for loose grammar.
|
|
|
|
|
|
|
The real question is where should commenting stop?
I have some coworkers who comment .NET functions and classes that we don't use a lot. Especially when it's more complex and intellisense alone won't help you a lot.
Just assume we don't use strings a lot. It would be something like:
string s = "Hello!"; Someone at our company might not know what a string is (after all, we rarely use it (in this example)), so this will save them a quick Google lookup.
At the other hand, is it really our job to document .NET classes?
Your example is clearly a little too much though!
It's an OO world.
public class Sander : Lazy<Person>{
public void DoWork(){ throw new NotImplementedException(); }
}
|
|
|
|
|
I would say if the people working on your code can't put the caret in the middle of "string" (or whatever you are using) and hit F1 to read the MSDN documentation, they probably shouldn't be messing around in the code base.
There are a lot of theories about how to document code, and here's mine:
1. Classes, methods, and properties (public especially, but all really), should be documented using the built-in /// method, with at least the summary and parameter explanations, remarks should be added if there is more, and document the exceptions that the method throws. This makes the other developers life easier (in Intellisense and later when you generate API documentation) to understand what a class/method/property is for.
2. Internal documentation (inside methods), generally skip it. There are three (main) reasons why somebody would document inside a function, (a) its complicated or (b) its long. If either of these are the case, it should be refactored. If the purpose of the function is sufficiently documented and the developer is qualified to work on it, the interior doesn't need to be documented. (There are of course rare exceptions, such as documenting bug work-arounds which if the developer didn't have knowledge of the bug, the code wouldn't make sense). The other reason (c) why people put comments in functions is generally white-noise. They are documenting the thought process which is unnecessary. A proper description of the input/output of the function and it being correctly "factored" shouldn't require knowing the developers thought process as they wrote it. When you see that they are probably doing "design by code", or "code-first", I won't get into that...
I used to be in the school of "every line should be documented", but I learned that really the comments where only there for the sake of writing the code, when I was debugging it I never looked at the comments again. It was more of "thinking out loud" than anything. Since adopting my method, I write cleaner, less cluttered code...
|
|
|
|