CheckStyle code review.



  • I think it's time to write about my recent code review.  It appears to be a combination of several different programmatic code review methodologies:  Checkstyle, PMD, FindBugs, and CPD.  I'm going to talk about the CheckStyle review in this post.

    I think someone turned CheckStyle all the way up to "cranky."

    Using the '.*' form of import should be avoided
    This I kind of understand, if only it weren't so whiny.  It was actually an "error" level issue for something that is not confusing at all given modern IDEs.  Using import java.io.* is borne of laziness but it's not a fucking code issue.  

    208 occurrencees of violating a regex (^[a-z_][a-zA-Z0-9]*$) that prevents me from ending a member with _, which is the previous coder's (he had me believing that it's an industry standard) methodology for designating a private member.  39 violations of the same regex with regards to method names.  This one is at the "warning" level.  I actually understand this specific complaint because again, with modern IDEs, you don't need anything special to designate private.  Looks like I switch them all to the front of the name?  

    Static variable definition in wrong order.

    Code involved:

    private static HashMap<String, String[ ][ ]> STATE_TABLE;

    Can you figure out what they're complaining about?  I can't. It's only informational level though.

    Constructor definition in wrong order.
    Yes, they have a special error made for if you don't put the constructor definition first.  Informational level.

    Variable 'resultsTruncated_' explicitly initialized to 'false' (default value for its type).
    I really don't understand what's the big deal.  Warning level.

    Return count is 4 (max allowed is 3).
    Nothing like arbitrary limits on what code can do.  Warning level.

    Declaring variables, return values or parameters of type 'Vector' is not allowed.
    What?  Warning level.

    Got an exception - java.lang.RuntimeException: Unable to get class information for @throws tag 'thrown'.
    Yeah, that's because you didn't import all the right libraries.  Error level.  Guess who's error?

    '=' is not preceded with whitespace.
    '=' is not followed by whitespace.
    '<' is not preceded with whitespace.
    '<' is not followed by whitespace.

    Code involved:

    for (int i=0; i<rsBcm.size(); i++)

    That's St. Louis Cardinals-quality whining.  Informational level.

    '403' is a magic number.
    You're actually right about this one, but if someone's reading my code and doesn't understand what a 403 error is, maybe they shouldn't be reading code for web applications.  Plus this is one of the last classes in the report and I'm sick of your shit.  Informational level.



  • @belgariontheking said:

    Static variable definition in wrong order.

    Code involved:

    private static HashMap<String, String[ ][ ]> STATE_TABLE;

    Can you figure out what they're complaining about?  I can't. It's only informational level though.

    Maybe they want:

        static private HashMap<String, String[ ][ ]> STATE_TABLE;

    which is pretty dumb to care about.

     

     



  • That is the most fascist code review I've ever seen.  My CTO would be up my ass if I tried to pull this crap with my developers - not that I would, because it's retarded.

    The coding guidelines I promote/enforce cannot be analyzed programmatically, i.e.:

    Does the code use proper OO?

    Is it reliable?

    Is it performant?

    Does it use a reasonable amount of resources?

    Are there errors (undisposed, unmanaged resources, etc.)?

    Does it use best practices in its design?

    Does it unneccessarily duplicate work?

    Is there some slight improvement or paradigm shift I can provide that will help this developer grow?

    The bullshit they're putting you through is preposterous.  The only things a programmaticly-generated analysis can tell me is crap I don't and shouldn't care about.  Besides, didn't we defeat the Nazis in 1945?

    ED: quoting the entire OP makes Verizon up your rates



  •  Meh, these are all reasonable things to expect from an automated review.  It's only a WTF if you are actually facing consequences for these 'problems'.


  • Garbage Person

    @belgariontheking said:

    Using the '.*' form of import should be avoided
    This is the part thaat I find most amusing. Over on the Microsoft side of the fence, we can ONLY import entire namespaces (and that's the way it should be. Importing specific classes is the compiler's fucking job)



  • I swear, you guys who post this kind of stuff have the shittiest programming jobs on earth. But at least it's entertaining on my end.



  • @Dankness said:

    It's only a WTF if you are actually facing consequences for these 'problems'.
    Well They only said that I have to fix the errors, which I'm taking to mean the error level issues.  They didn't further specify that I don't have to fix the warnings or informationals.  There were three other reports that came back as well that didn't have three different levels.  I'm unsure what to do about them because in one, the documentation admits that it gives false positives, and I found another case where it said I had an unread local variable but it was returned on the very next line.

    I'll keep you updated if you care (maybe even if you don't) in this thread and possibly start others on the other reports, but this one was the most aggravating to read.

    I also want to point out that 90% of the errors are in my predecessor's code, which accounts for 90% of the overall code (I'd say).  However, it's still aggravating to have to fix his stuff.



  • @belgariontheking said:

    Using import java.io.* is borne of laziness but it's not a fucking code issue.

    There can be an issuette. E.g., if you import java.sql.* you can get another Date class. In this particular case, it doesn't really matter, since you'll get a thousand error messages if you try to interchange it with the other Date class (java Date classes are TRWTF), but perhaps there are more modules with identical class names that can cause confusion.

    208 occurrencees of violating a regex (^[a-z_][a-zA-Z0-9]*$)

    That's a regexp that forbids underscores in names? Man, that's weird.

    These rules have just been written to give managers something to quantifictize (yes, that's a new word), so they can show their manager that after 12 hours of work, the number of messages at informational level went down by 703, and at warning level by 220, etc., and before you know it, you'll have the number of messages in your scrum planning. What a sad state of affairs.


  • I don't know, I can see there is some merit to pointing out code style issues as well as potential security vulns (which is always good to point out).  I've always subscribed (though never followed through) with the idea that simply by looking at the code, you should be able to tell whether you, or Bob, or Sam wrote the code.  This means similar (if not identical) coding style, variable/parameter/method/class naming, similar method parameter order, etc., etc.

    This makes reviewing someone elses code much easier because it's like looking at your own code.

    That being said, I challenge any self-respecting (.Net) developer worth their salt to NOT write a 27-line LINQ expression which returns an IEnumerable of 'a... :)



  • @hoodaticus said:

    ED: quoting the entire OP makes Verizon up your rates
    ... and it uglies up the forum.  Thanks!  I appreciate it.



  • @belgariontheking said:

    Variable 'resultsTruncated_' explicitly initialized to 'false' (default value for its type).
    How dare you to be explicit in your variable initialization?@belgariontheking said:
    '403' is a magic number.
    Kind of hard to avoid this one though. Can't expect them not to complain.



  • @C-Octothorpe said:

    I don't know, I can see there is some merit to pointing out code style issues as well as potential security vulns (which is always good to point out).  I've always subscribed (though never followed through) with the idea that simply by looking at the code, you should not be able to tell whether you, or Bob, or Sam wrote the code.  This means similar (if not identical) coding style, variable/parameter/method/class naming, similar method parameter order, etc., etc.

    This makes reviewing someone elses code much easier because it's like looking at your own code.

    That being said, I challenge any self-respecting (.Net) developer worth their salt to NOT write a 27-line LINQ expression which returns an IEnumerable of 'a... :)

    FTFY.

    Also, I don't understand the last sentence.



  • @C-Octothorpe said:

    That being said, I challenge any self-respecting (.Net) developer worth their salt to NOT write a 27-line LINQ expression which returns an IEnumerable of 'a... :)
    DONE.

     

    (and I didn't even have to open up my code editor)



  • @TGV said:

    @belgariontheking said:
    Using import java.io.* is borne of laziness but it's not a fucking code issue.
    There can be an issuette. E.g., if you import java.sql.* you can get another Date class. In this particular case, it doesn't really matter, since you'll get a thousand error messages if you try to interchange it with the other Date class (java Date classes are TRWTF), but perhaps there are more modules with identical class names that can cause confusion.
    Just for argument's sake, I'll respond to this one.

    If you're confused about which Date you're using, you can find in two ways: check which library is imported (this works even with .*) or hover over the class name with your mouse.  

    If you've only imported java.sql.* and not java.util.*, then you know.  Guess what you have to check if you've explicitly specified the class in the imports.  The imports!

    If you've imported them both, then you have to specify the full package name anyway, ala java.sql.Date myMotherfuckingDateAreYouHappyNowCheckStyle = new java.sql.Date("December penis, 1941");

    The only legit code issue that I found in googling this issue is that if you import two packages and then upgrade your libraries down the line.  The contents of those packages may collide, providing a compilation error that (hopefully you, hopefully not your successor), have to go figure out which one you meant in the first place.

    Not really worth it for me to argue though, since eclipse will nicely organize my imports for me, so that's 31 errors cut right off the top.  So it doesn't end up being a big deal.  Just whiny.

    The other 59 (at error level) were because I didn't include a copyright notice at the top.  There was one but apparently it wasn't good enough.  I'll give you the regex because it's just fucking awesome:

    (\*|//)(\s)*Copyright(\s)*\(C\)(\s)*\d\d\d\d(\s)*(\s)*(SS|SpiffySoft)(\s)*([\w\.])*(\s)*((\*|//)*|(\s)*)*All rights reserved
    I'll admit that I haven't created something that matches this.  This is the closest I got but either my little program to check it isn't working or there's something I don't understand.

     * Copyright (C) 2005 SpiffySoft All rights reserved

    ED: Goddamn that was rambly.  Hopefully it makes sense now.



  • @C-Octothorpe said:

    I've always subscribed (though never followed through) with the idea that simply by looking at the code, you should be able to tell whether you, or Bob, or Sam wrote the code.
    My view somewhat intersects yours.  Great minds think alike; great developers code alike. 

    If I am working with a bunch of great developers, our code will look alike.  It will probably follow the .NET conventions provided my MS as well as the best practices in each domain throughout the entire codebase, with optimal OO factorization.

    If, however, I am working with shit - as is often the case - I want to know I'm working with shit.  Letting shit hide behind a mask of intelligence will not keep me from stepping in poo, or wasting my time trying to find the hidden elegance when in fact there is none to be found.



  • @belgariontheking said:

    I think someone turned CheckStyle all the way up to "cranky."

    Using the '.' form of import should be avoided
    This I kind of understand, if only it weren't so whiny.  It was actually an "error" level issue for something that is not confusing at all given modern IDEs.  Using import java.io. is borne of laziness but it's not a fucking code issue.  

    Is using 'import java.*' a code issue?  It would seem to me there are places where this import form could be OK, and places where it would be problematic.  Furthermore, if you're importing any non-core modules in this manner, you're hiding your dependency, such that you could lose track of the fact you *have* the dependency.  Then, you ship code to someone who doesn't have that module, it doesn't work, and you likely have a somewhat obtuse stack trace to debug.

    @belgariontheking said:

    208 occurrencees of violating a regex (^[a-z_][a-zA-Z0-9]*$) that prevents me from ending a member with _, which is the previous coder's (he had me believing that it's an industry standard) methodology for designating a private member.  39 violations of the same regex with regards to method names.  This one is at the "warning" level.  I actually understand this specific complaint because again, with modern IDEs, you don't need anything special to designate private.  Looks like I switch them all to the front of the name?

    I can understand complaining about people ending identifiers with underscores.  However, those of us who dislike camelCaseWithAPassion generally like using underscore as a word separator within identifier names.

    @belgariontheking said:

    Variable 'resultsTruncated_' explicitly initialized to 'false' (default value for its type).
    I really don't understand what's the big deal.  Warning level.

    Well, if this were perl 5.6.x code, the big deal could be the minor performance penalty of actually making this assignment each time the variable is created.  In any reasonable compiler, however, I also don't understand the concern.  It's pedantic, but usually excessive pedantry is better than sloppiness.

    @Weng said:

    @belgariontheking said:

    Using the '.*' form of import should be avoided
    This is the part thaat I find most amusing. Over on the Microsoft side of the fence, we can ONLY import entire namespaces (and that's the way it should be. Importing specific classes is the compiler's fucking job)

    This explains things, a bit.



  • @Weng said:

    Over on the Microsoft side of the fence, we can ONLY import entire namespaces (and that's the way it should be. Importing specific classes is the compiler's fucking job)
    I guess you're ignoring VB (and that's for the best, really) where you can import specific classes.


  • Garbage Person

    @hoodaticus said:

    @Weng said:
    Over on the Microsoft side of the fence, we can ONLY import entire namespaces (and that's the way it should be. Importing specific classes is the compiler's fucking job)
    I guess you're ignoring VB (and that's for the best, really)
    VB.net is only there for legacy conversions. (Deciding whether it's for converting legacy software or legacy developers is an exercise left to the reader)


  • Garbage Person

    @tgape said:

    @Weng said:

    @belgariontheking said:

    Using the '.*' form of import should be avoided
    This is the part thaat I find most amusing. Over on the Microsoft side of the fence, we can ONLY import entire namespaces (and that's the way it should be. Importing specific classes is the compiler's fucking job)

    This explains things, a bit.

    It's also worth noting that you have to explicitly reference EVERYTHING (including the base framework modules) at the project level, so you can't lose module-level dependencies (and if you need to track dependencies on a class level, you're insane) - it doesn't just grab everything you have sitting in an include directory that matches your imported namespaces. It only grabs the libraries you told it to.


  • @belgariontheking said:

    Declaring variables, return values or parameters of type 'Vector' is not allowed.
    What?  Warning level.

     

     http://stackoverflow.com/questions/1386275/why-java-vector-class-is-considered-obsolete-or-deprecated

     



  • @Weng said:

    @belgariontheking said:

    Using the '.*' form of import should be avoided
    This is the part thaat I find most amusing. Over on the Microsoft side of the fence, we can ONLY import entire namespaces (and that's the way it should be. Importing specific classes is the compiler's fucking job)

    Not True!

    using myClassName = SomeNameSpace.ChildNameSpace.SpecificClass;



  • @TheCPUWizard said:

    @Weng said:

    @belgariontheking said:

    Using the '.*' form of import should be avoided
    This is the part thaat I find most amusing. Over on the Microsoft side of the fence, we can ONLY import entire namespaces (and that's the way it should be. Importing specific classes is the compiler's fucking job)

    Not True!

    using myClassName = SomeNameSpace.ChildNameSpace.SpecificClass;

    Yes, and how much is that used? I personally things it's a bad idea to rename classes.



  • @dtech said:

    Yes, and how much is that used? I personally things it's a bad idea to rename classes.

    I use it to rename XNA's color class as XNAColor, so its name doesn't conflict with the WinForms Color class.



  • @belgariontheking said:

    Not really worth it for me to argue though, since eclipse will nicely organize my imports for me, so that's 31 errors cut right off the top.  So it doesn't end up being a big deal.  Just whiny.

    Extremely whiny, I'm not going to disagree. It was just that there is a remote chance of the rule being useful, so I understand that somebody devised that rule. In contrast to, e.g., the underscore rule, or the copyright rule. A style checker that disapproves of open source software? And why does the rule escape the w and the dot in [\w\.]? What syntax is that? And why are there extra parentheses around it as well? Why is there (\s)*(\s)* in it? Anyway, I think you need a dot (or a w!) following SpiffySoft.



  • @dtech said:

    Yes, and how much is that used? I personally things it's a bad idea to rename classes.

    1) I use it regularly, and recommend it in a variety of circumstances. Keeping coupling to a minimum, and making it explicit has many advantages. Unless a person is using a significant number of classes from a given namespace within a specific class, importint the entire namespace makes it too eas to increase coupling (decreasing maintainability). There are even a few tools that will look at the number of distinct items used within a given source file and make the replacements is it is below a (Settable) threshold).

    2) Renaming is indeed generall bad (in fact, I would say very bad 99% of the time), the example was just an attempt at showing the ability. There is nothing to prevent

     using SpecificClass= SomeNameSpace.ChildNameSpace.SpecificClass;



  • @TGV said:

    And why does the rule escape the w and the dot in [\w.]?
    What syntax is that?

    It is not. \w is a shorthand for 'word characters', that is, letters numbers and diacritic accents. The dot is.

    That syntax is part of a regular expression. Don't swamp, learn regexps. They are useful (but not every time).



  • @tgape said:

    Is using 'import java.*' a code issue?  It would seem to me there are places where this import form could be OK, and places where it would be problematic.  Furthermore, if you're importing any non-core modules in this manner, you're hiding your dependency, such that you could lose track of the fact you *have* the dependency.  Then, you ship code to someone who doesn't have that module, it doesn't work, and you likely have a somewhat obtuse stack trace to debug.
    If you import java.*, I don't think you'll get any classes back. 

    If you meant something like java.util.* or java.io.*, it's really only an issue, like I mentioned, if you suddenly have two classes in two packages that have the same name.  

    I'm not sure you understand how the Java works.  If the JVM can't find a class, it throws a very recognizable ClassNotFoundException at runtime with the name of the missing class, complete with the package name.  If you can't find the associated jar at that point, that's an issue that's outside of your source code itself.  If the compiler can't find a class, it'll throw a compiler error.  No terribly complicated stack trace here. 

    I really have no idea how you lose track of the fact that you have a dependency.  That's really just beyond me.  Can you provide a detailed set of steps where that might happen?



  • @TGV said:

    Extremely whiny, I'm not going to disagree. It was just that there is a remote chance of the rule being useful, so I understand that somebody devised that rule. In contrast to, e.g., the underscore rule, or the copyright rule. A style checker that disapproves of open source software? And why does the rule escape the w and the dot in [\w\.]? What syntax is that? And why are there extra parentheses around it as well? Why is there (\s)*(\s)* in it? Anyway, I think you need a dot (or a w!) following SpiffySoft.

    I think we agree with each other on the import question.  I just wanted to make a couple points.  and ramble.

    (\s)*(\s)* is retarded.  It means it's going to check twice to make sure there is any amount (including 0) of whitespace.

    (\s)*([\w\.])*(\s)* means it's going to allow any amount (including 0) of whitespace, followed by any number (including 0) of word characters(alphanumeric + space) and periods, followed again by any amount (including 0) of whitespace.



  • @belgariontheking said:

    I really have no idea how you lose track of the fact that you have a dependency.  That's really just beyond me.  Can you provide a detailed set of steps where that might happen?

    Consider, if I give you a source package with 100 files. Now I want you to tell me which specific java.io classes are used in which specific files. I you have an IDE available, you can probably do it in under an hour. If you are browsing the source package through a repository, it is unlikely that you could get a 100% accurate list in under a day.

     However if each file explicitly imports just the classes it needs, then about 30 seconds with "grep" (or equiv) and you have the answer.

     To more specifically address the comment "how you lose track of the fact that you have a dependency"...can you pull out a program you wrote years ago, and provice a list of which of your source files depend on which specific java classes within a minute or tow? Even if you "knew them by heart" at the time you wrote the code, I would be willing to bet that would have "lost track" (aka forgotten the exact details).


  • Garbage Person

    @blakeyrat said:

    I use it to rename XNA's color class as XNAColor, so its name doesn't conflict with the WinForms Color class.
    Personally, I use the mechanism to alias the namespaces and qualify my shit as Forms.Color or XNA.Color. 



  • @TheCPUWizard said:

    @belgariontheking said:
    I really have no idea how you lose track of the fact that you have a dependency.  That's really just beyond me.  Can you provide a detailed set of steps where that might happen?

    Consider, if I give you a source package with 100 files. Now I want you to tell me which specific java.io classes are used in which specific files. I you have an IDE available, you can probably do it in under an hour. If you are browsing the source package through a repository, it is unlikely that you could get a 100% accurate list in under a day.

     However if each file explicitly imports just the classes it needs, then about 30 seconds with "grep" (or equiv) and you have the answer.

     To more specifically address the comment "how you lose track of the fact that you have a dependency"...can you pull out a program you wrote years ago, and provice a list of which of your source files depend on which specific java classes within a minute or tow? Even if you "knew them by heart" at the time you wrote the code, I would be willing to bet that would have "lost track" (aka forgotten the exact details)

    The way I see it, I can check it out and try to compile.  I include all the jars I need in the repo itself, along with eclipse's project settings files.  There's no reason it shouldn't compile unless I've changed something. 

    Then my job is to figure out what I've changed, and importing .* vs fully qualified classes isn't likely to help me towards that.  I just read the compile errors and start digging.

    I guess my point is that even if you import com.sample.project.*, it's still just as useful as importing the fully qualified class names when you're trying to figure out which jars you need.  The only thing that importing the fully qualified class names simplifies is trying to figure out which package a certain class came out of, which shouldn't be a problem except in some weird cases.

    I see the value (however small) of importing the fully qualified class names, but I'm certainly not about to type them out myself.  I'll just import the .*, write the class, then tell eclipse to change them to the fully qualified class names.  I'm not about do deal with that shit.

     



  • belgariontheking, you seem to be missing the point entirely. It is not about "to figure out what I've changed", or "compile errors", or " trying to figure out which jars trying to figure out which jars". Those elements are trivial, and account for only a very small fraction of what is important to solid software engineering principals (I would estimate it at 2%-5%). The other 95%-98% is what I am referring to.

     As I aluded to previously, if you need to do an auit of the work you (or a company) has sone, that may involved hundreds of project and tens of thousands of source files and determine exactly which source files across all of those items (which could easily exceed a million filles) actually use specific classes within the various namespaces, then you are basically "up a creek without a paddle" - and we won't even mention which creek!

     Keeping the coupling as low as possible (by providing access to only specific classes, or if necessary very granular namespaces) will reduce this effort (and others) by multiple orders of magnitude.



  • @TheCPUWizard said:

    Consider, if I give you a source package with 100 files. Now I want you to tell me which specific java.io classes are used in which specific files. I you have an IDE available, you can probably do it in under an hour. If you are browsing the source package through a repository, it is unlikely that you could get a 100% accurate list in under a day.

    However if each file explicitly imports just the classes it needs, then about 30 seconds with "grep" (or equiv) and you have the answer.

    In Eclipse, I would select a class name and press Ctrl+Shift+G to get a list of all references in the whole workspace. This works for almost any identifier. The Java Search supports more cool stuff; for example, searching for java.lang.Exception in catch clauses only. My point is: Java-aware tools will probably yield far better results than plain text search.

    @TheCPUWizard said:

    Keeping the coupling as low as possible (by providing access to only specific classes, or if necessary very granular namespaces) will reduce this effort (and others) by multiple orders of magnitude.

    I don't get it. Consider:

    import java.util.*;
    public class Test {
    	public static void main(String[] args) {
    		System.out.println(Arrays.toString(args));
    	}
    }
    import java.util.Arrays;
    public class Test {
    	public static void main(String[] args) {
    		System.out.println(Arrays.toString(args));
    	}
    }
    public class Test {
    	public static void main(String[] args) {
    		System.out.println(java.util.Arrays.toString(args));
    	}
    }
    public class Test {
    	public static void main(java.lang.String[] args) {
    		java.lang.System.out.println(java.util.Arrays.toString(args));
    	}
    }

    So you argue that you cannot `grep` for "java.util.Arrays" in the first example. Fair enough. But what does that have to do with coupling?



  • @TheCPUWizard said:

    As I aluded to previously, if you need to do an auit of the work you (or a company) has sone, that may involved hundreds of project and tens of thousands of source files and determine exactly which source files across all of those items (which could easily exceed a million filles) actually use specific classes within the various namespaces
    I've never had to do an audit quite like that, so I'll just have to trust you that they exist and concede the point. 

    If I could get away with just listing all the libraries, as opposed to all the classes, I'd just list all the jars used and save myself some trouble.  There's never really any sense in listing all the native Java 



  • There is no question what when woulding with a single solution [.NEt term, feel free to substitute the equivilant Eclipse term] that using the language aware tools is a much better choice as they provide immense power that is not available with "text" based approaches. Also, much depends on your situation. If you are a lone developer, or a member of a small team that works on relatively few projects over the course of a year and these tend to be "run of the mill applications", then the scenarios I am talking about are unlikely to be important (and may not occur at all).

     On the other hand, if your are a company dealing with dozens of clients, each with multiple projects, and a combined development staff (your own firm + the developers at each company - all aggregted) that number in the hundreds, then things change. Add to that, dealing with critical systems (e.g. medical devices, industrial autiomatioc - both cases where injury or death can occur, or core financial systems - massive economic loss) and you enter a whole different world than that described in the firt paragraph.

    I happended to pick on the "references issue" because it was recently a real issue for my firm, but many of the other CheckStyle type code review rules (not necessary the ones of the OP) start to make alot of sense when considering this type o fenvironment.



  • @TheCPUWizard said:

    I happended to pick on the "references issue" because it was recently a real issue for my firm, but many of the other CheckStyle type code review rules (not necessary the ones of the OP) start to make alot of sense when considering this type o fenvironment.
    Yeah maybe to be fair I could have posted the legit complaints it had too (like complaining about == string comparison -- I still shudder that that made sense to my predecessor), but those didn't make me go WTF so they stayed out.



  • @belgariontheking said:

    Yeah maybe to be fair I could have posted the legit complaints it had too (like complaining about == string comparison -- I still shudder that that made sense to my predecessor), but those didn't make me go WTF so they stayed out.

    Without knowing the context, it is imposible to make an accurate judgment...but, doing equality compares on strings (i.e. byte level) is quite often wrong. It very well may make sense to have a warning where a specific comparision method that is explicit about how the strings are compared.




  • const FouR_Oh_FrweE = 403;

    const FouR_Oh_FrweE_DoT_tOo = 403.2;

     



  • ♿ (Parody)

    @TheCPUWizard said:

    @belgariontheking said:
    Yeah maybe to be fair I could have posted the legit complaints it had too (like complaining about == string comparison -- I still shudder that that made sense to my predecessor), but those didn't make me go WTF so they stayed out.

    Without knowing the context, it is imposible to make an accurate judgment...but, doing equality compares on strings (i.e. byte level) is quite often wrong. It very well may make sense to have a warning where a specific comparision method that is explicit about how the strings are compared.

    The context was pretty obviously java, where the == comparison of any sort of object simply checks to see if the two references are the same object, which is to say that the pointers of the two objects are the same. It doesn't care at all about any lower level data. It's a relatively common mistake, especially if you code in stuff besides java. You have to use the equals() method. About the only time this is really useful is if you know that one of the objects is null. It's practically guaranteed that's not what the OC was doing.


  • Considered Harmful

    @boomzilla said:

    The context was pretty obviously java, where the == comparison of any sort of object simply checks to see if the two references are the same object, which is to say that the pointers of the two objects are the same. It doesn't care at all about any lower level data. It's a relatively common mistake, especially if you code in stuff besides java. You have to use the equals() method. About the only time this is really useful is if you know that one of the objects is null. It's practically guaranteed that's not what the OC was doing.

    I come from a .NET background - does Java not do string interning? So equivalent string instances usually are the same instance.

    (I still use .Equals in .NET, but usually when a developer on my team slips up and uses ==, the code still works fine.)


  • ♿ (Parody)

    @joe.edwards said:

    I come from a .NET background - does Java not do string interning? So equivalent string instances usually are the same instance.

    (I still use .Equals in .NET, but usually when a developer on my team slips up and uses ==, the code still works fine.)

    You can do that, but it doesn't happen by default. Does .Net really intern all strings by default? Hmmm....the documentation for the String intern method leads me to believe that it doesn't. It seems like doing that for every single string would be a stupendous example of premature optimization.



  • .Net automatically interns all string LITERALS. Other than that, only strings you explicitly intern are.



  •  @belgariontheking said:

    Got an exception - java.lang.RuntimeException: Unable to get class information for @throws tag 'thrown'.
    Yeah, that's because you didn't import all the right libraries.  Error level.  Guess who's error?


    Are you sure? Looks to me like a badly formatted @throws tag which is confusing it about what is actually thrown. Is it looking for a class  thrown extends Exception, or is it a really badly worded error message?

     

    @joe.edwards said:
    @boomzilla said:
    The context was pretty obviously java, where the == comparison of any sort of object simply checks to see if the two references are the same object, which is to say that the pointers of the two objects are the same. It doesn't care at all about any lower level data. It's a relatively common mistake, especially if you code in stuff besides java. You have to use the equals() method. About the only time this is really useful is if you know that one of the objects is null. It's practically guaranteed that's not what the OC was doing.


    I come from a .NET background - does Java not do string interning? So equivalent string instances usually are the same instance.

    (I still use .Equals in .NET, but usually when a developer on my team slips up and uses ==, the code still works fine.)


     Java does string interning for string literals, but for a string which is read from a file or built with a StringBuilder you have to intern it yourself if that's what you want. However, I think you're over-simplifying the .Net situation a bit. C# has a compiler hack where if the compiler can tell you're comparing two strings with == it compiles that to a call to string.Equals.


  • 🚽 Regular

    Is there any way you can set options (especially for those silly things like regular expressions for property names, max return counts, and other arbitrary limits/standards)? I would think any descent code review application would let you set those kinds of things.

    I used to have a human-version of CheckStyle who always ranted every time I would make a mistake with white space or naming. There's nothing more annoying than being called into his office just for him to point out "On this file on line 67, you didn't put a space after the '='. Why? And you named it rowCount when it should be numberOfRows."



  • @boomzilla said:

    @joe.edwards said:
    I come from a .NET background - does Java not do string interning? So equivalent string instances usually are the same instance.

    (I still use .Equals in .NET, but usually when a developer on my team slips up and uses ==, the code still works fine.)

    You can do that, but it doesn't happen by default. Does .Net really intern all strings by default? Hmmm....the documentation for the String intern method leads me to believe that it doesn't. It seems like doing that for every single string would be a stupendous example of premature optimization.

    It doesn't, but it does intern strings "when it thinks it needs to." From my understanding. Maybe it does it during garbage collection or something, I dunno. Edit: Actually reading that article instead of skimming it, MSDN does claim that the CLR always interns strings:

    @MSDN said:

    The common language runtime conserves string storage by maintaining a table, called the intern pool, that contains a single reference to each unique literal string declared or created programmatically in your program. Consequently, an instance of a literal string with a particular value only exists once in the system.

    There's no occurrences of "can" or "sometimes" there, it's pretty unambiguous. Which raises the obvious question: what the shit does String.Intern actually do!? Because if the bit quoted above is true, then it does nothing in all cases. Of course looking at the example, bam, there's a quick and easy way to create a non-interned String, which means the quoted part contradicts the example... ugh.

    The real point is that C# will compile a==b as a.Equals(b) if it needs to. So you don't need to know or care whether the string is, was, or could be interned.



  • @belgariontheking said:

    '403' is a magic number.
    You're actually right about this one, but if someone's reading my code and doesn't understand what a 403 error is, maybe they shouldn't be reading code for web applications.  Plus this is one of the last classes in the report and I'm sick of your shit.  Informational level.

     Yes, and it's generally a bad idea to use magic numbers.  At least declare it as a constant.

    If this is in a Servlet, you should be using HttpServletResponse.SC_FORBIDDEN (which I assume has the value 403 (since it's an int), but I haven't actually checked).  If this is in an outgoing connection, you should be using [url=http://download.oracle.com/javase/6/docs/api/java/net/HttpURLConnection.html#HTTP_FORBIDDEN]HttpURLConnection.HTTP_FORBIDDEN[/url]




  • @blakeyrat said:

    It doesn't, but it does intern strings "when it thinks it needs to." From my understanding. Maybe it does it during garbage collection or something, I dunno. Edit: Actually reading that article instead of skimming it, MSDN does claim that the CLR always interns strings: @MSDN said:
    The common language runtime conserves string storage by maintaining a table, called the intern pool, that contains a single reference to each unique literal string declared or created programmatically in your program. Consequently, an instance of a literal string with a particular value only exists once in the system.
    There's no occurrences of "can" or "sometimes" there, it's pretty unambiguous. Which raises the obvious question: what the shit does String.Intern actually do!? Because if the bit quoted above is true, then it does nothing in all cases. Of course looking at the example, bam, there's a quick and easy way to create a non-interned String, which means the quoted part contradicts the example... ugh.

    The real point is that C# will compile a==b as a.Equals(b) if it needs to. So you don't need to know or care whether the string is, was, or could be interned.

    Bolded for you. 

    string a = "Some string"; //Interned

    string b = Console.ReadLine(); //Not interned



  • @Sutherlands said:

    Bolded for you.

    Yeah, yeah, I know. I got that a few minutes after reading my post again, and then it was too late to edit, and I didn't want to reply to myself and spam-up the forums. I missed the word "literal" on my first read-through, oops.

    I still think it could be rewritten to be clearer.


  • ♿ (Parody)

    @blakeyrat said:

    The real point is that C# will compile a==b as a.Equals(b) if it needs to. So you don't need to know or care whether the string is, was, or could be interned.

    This is one of those things that sounds like a really convenient idea, but seems like it could lead to some hard to find "bugs." I'd assume that the compiler makes sure that a can't be null, which is the most obvious problem. It seems like there's little need to check for the actual same reference, though I suppose someone developing some sort of ORM might want to. I dunno. No doubt it saves waaaaay more sloppy coders than it burns, if any. Its biggest negative effect might just be that it clouds the understanding of what's actually going on in the minds of many C# developers.



  • @frits said:

    @belgariontheking said:

    Static variable definition in wrong order.

    Code involved:

    private static HashMap<String, String[ ][ ]> STATE_TABLE;

    Can you figure out what they're complaining about?  I can't. It's only informational level though.

    Maybe they want:

        static private HashMap<String, String[ ][ ]> STATE_TABLE;

    which is pretty dumb to care about.

     

     

    They're complaining about the order of the declaration in the module.  Theory being, all statics (and static initializer blocks) should be before all regular fields.That *should* make statics easier to find if you're reviewing code, perhaps outside an IDE.

     

    Most of these errors are there for a reason.  They're not pretty, but usually have some sense behind them. For instance, leading and trailing underscores have traditionally been reserved for code generators, or special system variables (__LINE__ ?).  For code generators, it's so that they can pump out variables that don't conflict with the user's variables.

    I'd treat these all as a learning experience.  Why are they that way?  Will my code be easier for someone else to read if I follow the requirements?

    For instance, I'm sure you'd be horror-struck if you came upon variable names starting with upper case letters in Java.  This too was initially just a coding convention, one that you've learned to appreciate and that makes your job of untold hours of staring at the screen a little bit easier.


Log in to reply