Henning P. Schmiedehausen wrote:
There was a vote back in November where the inconclusive conclusion tended heavily towards rolling this code back into Turbine. I didn't want to go through the hassle of raising this again and I don't really want to wade into the enormity of commons for a very static 3 class package.[EMAIL PROTECTED] writes:
Hi Scott,
These classes are no longer deprecated - commons-http is clearly dead.
Well, as it is neither visible in commons proper nor in sandbox, one could say that. However, wouldn't it be better to resurrect these classes in commons-http (make it un-dead... ;-) )
Guilty as charged of mixing formatting changes with functional changes, though the only functional change was the removal of the deprecation tag.
- if (expiry == 0) + if (0 == expiry)
[...]
- data.getResponse().setHeader( - "Expires", formatHttpDate(new Date())); + data.getResponse().setHeader("Expires", + formatHttpDate(new Date()));
Well, caveat emptor. I learned to hate changes like this the hard way. If you use your tool, I might use another and a third developer yet another. The first change IMHO is completely unnecessary, folks like me might even be tempted to revert it back.
Doing Formatting changes just for the sake of formatting changes makes the whole source code management thing moot. You can't simply diff between older revisions to see what really changed because the functional changes are drowned in a sea of "...I prefer each method call on a separate line...", "... throws belongs right after the method signature, no it should be on a new line...","... opening brackets on a new line or not..." changes. If there are multiple developers, even things like "I prefer to indent Exceptions four spaces. No I prefer to indent them eight spaces" are cluttering the code.
Yes, I'm guilty of my own share of checking in such changes. However, I would _really_ urge you to not do it. It simply makes no sense.
- data.getResponse().setHeader( - "Expires", formatHttpDate(expiryDate)); + data.getResponse().setHeader("Expires", + formatHttpDate(expiryDate));
Same thing. I would even prefer the first version.
} } + }
Someone else will remove this blank line for sure.
public class BrowserDetector
{
+ public static final String MSIE = "MSIE";
+ public static final String OPERA = "Opera";
+ public static final String MOZILLA = "Mozilla";
+
+ public static final String WINDOWS = "Windows";
+ public static final String UNIX = "Unix";
+ public static final String MACINTOSH = "Macintosh";
+
/** The user agent string. */
private String userAgentString = "";
@@ -72,14 +78,6 @@
/** Whether or not file upload works in this browser. */
private boolean fileUploadOK = false;
- /** Constants used by this class. */
- public static final String MSIE = "MSIE";
- public static final String OPERA = "Opera";
- public static final String MOZILLA = "Mozilla";
- public static final String WINDOWS = "Windows";
- public static final String UNIX = "Unix";
- public static final String MACINTOSH = "Macintosh";
-
Senseless reordering (I would have done this, too :-) ). Your IDE will display them in an outline anyway.
Please read this as constructive critisism, not as flaming. As I said, I learned this the hard way in a project where I used my tool (emacs) and another developer used his tool (Eclipse). In the end, we decided to add a code formatter to the checkin scripts. ;-)
Some of the changes were force of habit on my part (0 == expiry, blank line at end of class - slap on the wrist for these), some were dragged in from commons-http (moving the constants to the top of the class - as you said, you would have done this too :-).
The line wrapping changes close to the setHeader() invocations are more in line with the Turbine coding standard IMO and there were other formatting changes that corrected violations of the standard that do in fact make things clearer (adding missing braces to if...else, moving || to beginning of line on wrapped if statement) so a formatting commit was in order for one of these two classes anyway.
I promise to be more careful in the future :-)
Cheers,
Scott
-- Scott Eade Backstage Technologies Pty. Ltd. http://www.backstagetech.com.au
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
