• (nodebb)

    the person who developed the data access layer chose not to raise exceptions and instead returned nulls when data couldn't be found

    And presumably in other, actually exception-worthy, conditions.

    If data can't be found, and it's just a "where" that matches no records, that's not exception-worthy, and shouldn't return a null instead.

    If the query asks for data from a non-existent database / table / column, that's exception-worthy.

    But returning nulls instead is a good candidate for TRWTF.

  • (nodebb)

    Variety is the spice of life... use one approach exclusively and the other techniques will become forgotten [95% snide remark, but 5% truth]

  • Naomi (unregistered)

    Before anyone asks, no, there's no reason (good or otherwise) to use a capital-b Boolean here. This isn't even a case where an ancient version of Java would have required it. It's just kinda there.

  • Prime Mover (unregistered)

    And this:

    ArrayList<RepDefinitionBean> arrayList = new ArrayList();

    instead of this:

    List<RepDefinitionBean> reportList = new ArrayList();
  • (nodebb)

    Today I learned something new about Java. You can do assignment inside an if condition.

    I would argue that returning null (or some other signifier of "no results" e.g. an empty array) when a search fails to produce a result is perfectly fine and throwing an exception is the wrong thing to do. However, the code that uses the result must do whatever is appropriate in context. In this case, it should log an error.

    Finally, this code snippet provides an excellent example of why the K&R brace style is a WTF. I initially read the second line of the if as part of the body of the if because it starts with an assignment but it is still part of the condition. Using practically any other brace style e.g. Allman, removes the possibility of misreading it.

    		if ((reportList = ReportListApp.loadByName(str = iterator.next())) != null && (         
			reportDefinition = ReportDefinitionApp.loadReportDefinition(reportList.getReportNo())) != null) 
               {
			RepDefinitionBean repDefinitionBean = new RepDefinitionBean(str, reportDefinition.getReportLabel());
			arrayList.add(repDefinitionBean);
		}
  • Naomi (unregistered)

    Today I learned something new about Java. You can do assignment inside an if condition.

    Assignment can be either an expression or a statement, just like in C. Using it that way is a lot like labeled break statements - it occasionally makes for code that's easier to read, but usually doesn't.

    Finally, this code snippet provides an excellent example of why the K&R brace style is a WTF.

    This code snippet is a huge WTF in any language, and I'd be really hesitant to use it as an example of anything.

  • ZZartin (unregistered)

    Definitely some WTF's in there, doing assignment intentionally in an if statement is bleh.

    So someone misspelled a report name in the web.xml and thus the report couldn't be run, but there were no errors to indicate what might have gone wrong.

    But errors to where? Spam ever user every time they try to search for a report because something in web.xml isn't right? That seems far worse than what should be an obvious problem with a very easy fix.

  • (nodebb) in reply to ZZartin

    That seems far worse than what should be an obvious problem with a very easy fix.

    Then why leave it broken in the first place? If something like that is bust, the app should shout about it so that the original author of the problem deals with it. The right place to shout about it is probably in the log though, not directly to the end user (who should just get a 500 error or something like that). If the log isn't visible enough for how people are deploying, that's a totally separate problem.

  • Rob (unregistered) in reply to Naomi

    The use of Boolean, Boolean.valueOf and booleanValue() was absolutely valid before auto-boxing was introduced in Java 5.0. The second argument to request.setAttribute is Object, and without auto-boxing, a boolean simply doesn't fit. The boxing could have been limited to the call to request.setAttribute, but boxing would still be needed.

    Of course, is this code was written after 2004, then there is indeed no excuse to use Boolean, as request.setAttribute can take a boolean thanks to auto-boxing.

  • Airdrik (unregistered) in reply to dkf

    Unless you have good reason to let those sorts of problems slip into the production version of the app then make it fail loudly and immediately, as close to the making of the mistake as possible. No need to log an error that may or may not be seen any time soon if it can be checked at build (unit/integration test) or app startup (like other fat-fingering mistakes in the web.xml file). (at least I hope that the web.xml file is checked in and bundled with the application like any sane web service; though considering the suffix on this site might not be a safe assumption)

  • Airdrik (unregistered) in reply to Rob

    Java 5 also introduced Generics which are clearly employed here, as well as the for-each loop construct Remy pointed out. Perhaps it may have been ported from an earlier version of java, adding generic types where the compiler complained about them (except not completely, since the ArrayList constructor should have had the generic type; though omitting that is legal, only garnering a compiler warning about a raw type). More likely, imo due to the mixed use of constructs old and new, was that this was written some time after Java 5 by someone who was new to the language (and programming? I'm not sure what other language background would have produced code like this) and hadn't taken the time to learn all of the basics yet; just enough to make it work.

  • Naomi (unregistered) in reply to Rob

    The second argument to request.setAttribute is Object, and without auto-boxing, a boolean simply doesn't fit.

    Okay, that's true. I think I missed that call because I was annoyed at it being used in the if (well, and at the variable name).

  • Prime Mover (unregistered) in reply to Jeremy Pereira

    "Finally, this code snippet provides an excellent example of why the K&R brace style is a WTF. "

    No, the problem here is just you not being used to it.

    Alright, to be charitable, the real WTF here is indenting the continuation of the if statement to the same level as its contents.

  • Greg (unregistered) in reply to ZZartin

    I don't know about java, but in C++ there can be cases where it makes a lot of sense to do an assignment in the if statement: if( auto const * getResult = Get(); getResult && getResult->… )

    if that Get() function is costly it makes a lot of sense to store the value in a variable instead of calling it twice. putting the variable in the if statement limits its scope to the scope of the if.

  • nasch (unregistered) in reply to Steve_The_Cynic

    "If data can't be found, and it's just a "where" that matches no records, that's not exception-worthy, and shouldn't return a null instead."

    That depends. If the business rules state that this thing should always be found, without exception, and it's a bug if it's not there, then raising an exception seems completely reasonable. And since this is looking up a type of report, if you're looking for a kind of report that the system doesn't know how to generate, that's an entirely different class of problem than the customer name wasn't found in the database.

  • Mithyla Maria (unregistered)
    Comment held for moderation.
  • Myhinditracks (unregistered)
    Comment held for moderation.

Leave a comment on “Getting Reported”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article