SessionFactoryImpl - take it to the dry cleaner?

Obfuscator

Was debugging today, ended up in the Hibernate source code. Turns out that the constructor of SessionFactoryImpl weighs in at >325 lines and contains nuggets such as a local class and sections where the comments give a perfect suggestion of a suitable method name for them. Lo and behold:

http://grepcode.com/file/repository.jboss.org/nexus/content/repositories/releases/org.hibernate/hibernate-core/4.1.10.Final/org/hibernate/internal/SessionFactoryImpl.java?av=f

keigezellig

And I always thought that you should keep your constructors (any method actually) as small as possible..

JimLahey

wow. I don't know how this translates to NHibernate but I wouldn't have expected all that in Hibernate.

Severity_One

126 lines of imports don't exactly bode well either.

mikeTheLiar

@Severity One said:

126 lines of imports don't exactly bode well either.

No to mention the warning suppressions on the constructor.

morbiuswilters

@mikeTheLiar said:

@Severity One said:
126 lines of imports don't exactly bode well either.

No to mention the warning suppressions on the constructor.

And I get newbie Java developers asking me why I won't use Hibernate..

FrostCat

@mikeTheLiar said:

@Severity One said:
126 lines of imports don't exactly bode well either.

No to mention the warning suppressions on the constructor.

Hey, some guy in 1998 said "import package.*;" is bad without saying why, therefore it must be true forever.

morbiuswilters

@FrostCat said:

@mikeTheLiar said:
@Severity One said:
126 lines of imports don't exactly bode well either.

No to mention the warning suppressions on the constructor.

Hey, some guy in 1998 said "import package.*;" is bad without saying why, therefore it must be true forever.

Pfft.. import's for pikers. Just use fully-qualified class names everywhere.

FrostCat

@morbiuswilters said:

@FrostCat said:
@mikeTheLiar said:
@Severity One said:
126 lines of imports don't exactly bode well either.

No to mention the warning suppressions on the constructor.

Hey, some guy in 1998 said "import package.*;" is bad without saying why, therefore it must be true forever.

Pfft.. import's for pikers. Just use fully-qualified class names everywhere.

What are you, an Ada programmer?

morbiuswilters

@FrostCat said:

What are you, an Ada programmer?

I had a cat named Ada once (technically named after the language's namesake and not the language itself.) She was a tiny cat with a small frame and when I took her in, she was thin. However, the sedentary, indoor life soon caused her to get fatter and fatter. Eventually it looked like she had swallowed a cantaloupe whole. At some point I thought to myself "I should have named her Java.."

Mcoder

@morbiuswilters said:

@FrostCat said:
@mikeTheLiar said:
@Severity One said:
126 lines of imports don't exactly bode well either.
No to mention the warning suppressions on the constructor.

Hey, some guy in 1998 said "import package.*;" is bad without saying why, therefore it must be true forever.

Pfft.. import's for pikers. Just use fully-qualified class names everywhere.

Nahn. Still doesn't beat the C++ way of creating an import file, where you put all your imports, and just import it everywhere.

And, of course, soon enough you'll have an import file for each module of your project, thus you'll need an import file's import file... And for big enough projects an import file's import file's import file.

ekolis

@Mcoder said:

And for big enough projects an import file's import file's import file.

It's import files all the way down...

Ben L.

@Mcoder said:

Nahn. Still doesn't beat the C++ way of creating a~~n import~~ header file, where you put all your ~~imports~~declarations, and just ~~import~~include it everywhere.

And, of course, soon enough you'll have a~~n import~~ header file for each module of your project, thus you'll need ~~an import file's import file~~to include a header that includes a header... And for ~~big enough projects~~ any project that uses any of the standard library ~~an import file's import file's import file~~ the preprocessor will have to churn through layer after layer of includes.

FTFY

FrostCat

@Mcoder said:

@morbiuswilters said:
@FrostCat said:
@mikeTheLiar said:
@Severity One said:
126 lines of imports don't exactly bode well either.

No to mention the warning suppressions on the constructor.

Hey, some guy in 1998 said "import package.*;" is bad without saying why, therefore it must be true forever.

Pfft.. import's for pikers. Just use fully-qualified class names everywhere.

Nahn. Still doesn't beat the C++ way of creating an import file, where you put all your imports, and just import it everywhere.
And, of course, soon enough you'll have an import file for each module of your project, thus you'll need an import file's import file... And for big enough projects an import file's import file's import file.

And that neatly links to the Go thread where I mentioned performance enhancements. Precompiled headers, once you get the hang of them, are so awesome it's not even funny.

But no, let's make up still yet another new language instead.

FrostCat

@Ben L. said:

@Mcoder said:
Nahn. Still doesn't beat the C++ way of creating a~~n import~~ header file, where you put all your ~~imports~~declarations, and just ~~import~~include it everywhere.

And, of course, soon enough you'll have a~~n import~~ header file for each module of your project, thus you'll need ~~an import file's import file~~to include a header that includes a header... And for ~~big enough projects~~ any project that uses any of the standard library ~~an import file's import file's import file~~ the preprocessor will have to churn through layer after layer of includes.
FTFY

PRECOMPILED HEADERS. I wish I'd seen this before I replied to the previous comment.

It may only be a local maximum, but I don't think you should let the perfect be the enemy of the good, and it's ridiculous how much compilation time you save even with semi-trivial projects.

Severity_One

@mikeTheLiar said:

@Severity One said:
126 lines of imports don't exactly bode well either.
No to mention the warning suppressions on the constructor.

Actually, that's not even that bad. Given the way that generics are implemented in Java, occasionally you can't do otherwise than ignore "unchecked" warnings. And the other warning looks like it comes from some static source code analysis tool, which is a Good Thing.

But the school of thought that classes (and constructors) should do as little as possible has apparently not reached whoever implemented this class.

dkf

@Severity One said:

Actually, that's not even that bad. Given the way that generics are implemented in Java, occasionally you can't do otherwise than ignore "unchecked" warnings.

You can enable such ignorance on the level of a single local variable declaration. That's probably about the right level too; it's a “shut up stupid tool; this really is correct”. Putting it on large constructors is much more troublesome.

Ben L.

@dkf said:

@Severity One said:
Actually, that's not even that bad. Given the way that generics are implemented in Java, occasionally you can't do otherwise than ignore "unchecked" warnings.
You can enable such ignorance on the level of a single local variable declaration. That's probably about the right level too; it's a “shut up stupid tool; this really is correct”. Putting it on large constructors is much more troublesome.

Or do it the right way: fix the bug (in your code or get the vendor to fix the compiler) and enable your compiler's version of -Werror

error

@Ben L. said:

do it the right way: fix the bug

It often makes sense to throw up a warning for something that's wrong 95%+ of the time, and right occasionally. That doesn't make it a bug in the times when it is right.

Ben L.

@joe.edwards said:

@Ben L. said:
do it the right way: fix the bug

It often makes sense to throw up a warning for something that's wrong 95%+ of the time, and right occasionally. That doesn't make it a bug in the times when it is right.

If it's wrong 95% of the time, you're detecting it incorrectly.

error

@Ben L. said:

@joe.edwards said:
@Ben L. said:
do it the right way: fix the bug

It often makes sense to throw up a warning for something that's wrong 95%+ of the time, and right occasionally. That doesn't make it a bug in the times when it is right.
If it's wrong 95% of the time, you're detecting it incorrectly.

It's only useful to warn about something if it's wrong 100% of the time? That's what errors are for.

Ben L.

@joe.edwards said:

@Ben L. said:
@joe.edwards said:
@Ben L. said:
do it the right way: fix the bug

It often makes sense to throw up a warning for something that's wrong 95%+ of the time, and right occasionally. That doesn't make it a bug in the times when it is right.
If it's wrong 95% of the time, you're detecting it incorrectly.

It's only useful to warn about something if it's wrong 100% of the time? That's what errors are for.

Sorry, I thought "it" refered to the compiler.

Why do we need warnings at all? If it's worth mentioning, it's worth reporting as an error.

error

Quick glance at my team's solution warnings:

SQL71502: Procedure: [dbo].[sp_GetDownloadLicenseCounts] has an unresolved reference to object [dbo].[sp_executesql].

Well, that one's a system procedure so of course it's not defined in our solution.

The version '1.0.0.*' specified for the 'file version' is not in the normal 'major.minor.build.revision' format

This was actually done at the advice of a Microsoft KB article.

There was a mismatch between the processor architecture of the project being built "MSIL" and the processor architecture of the reference "Oracle.DataAccess, Version=4.112.3.0, Culture=neutral, PublicKeyToken=89b483f429c47342, processorArchitecture=AMD64", "AMD64".

This one might be legit. Oracle stuff links off into native code so it's not properly portable; but all of our dev boxen and the web servers we're deploying to are x64 architecture, so it's not a problem in practice. We might be able to burn some hours (- who am I kidding? This is Oracle - days) getting an x86 binary and an x64 binary and bundling up the right one based on the solution properties... But luckily, it's just a warning, and we don't have to waste time for no real good.

PJH

@Ben L. said:

@joe.edwards said:
@Ben L. said:
@joe.edwards said:
@Ben L. said:
do it the right way: fix the bug

It often makes sense to throw up a warning for something that's wrong 95%+ of the time, and right occasionally. That doesn't make it a bug in the times when it is right.
If it's wrong 95% of the time, you're detecting it incorrectly.

It's only useful to warn about something if it's wrong 100% of the time? That's what errors are for.
Sorry, I thought "it" refered to the compiler.
Why do we need warnings at all? If it's worth mentioning, it's worth reporting as an error.

Something that some C compilers throw up a warning for, which is usually wrong most of the time, but is correct in this first example:

FILE* fp;
if (fp = fopen("Filename", "r)){

// do stuff with fp

}else{

// report file problem

}

This is to forestall problems with code such as

if (variable = otherVariableToCheckAgainst){...

where it is a logical error to assign the value, rather than compare for equality.

(Of course there's a way to silence that warning where the usage of = is correct, so this example's probably a bit meh.)

Ben L.

@PJH said:

@Ben L. said:
@joe.edwards said:
@Ben L. said:
@joe.edwards said:
@Ben L. said:
do it the right way: fix the bug

It often makes sense to throw up a warning for something that's wrong 95%+ of the time, and right occasionally. That doesn't make it a bug in the times when it is right.
If it's wrong 95% of the time, you're detecting it incorrectly.

It's only useful to warn about something if it's wrong 100% of the time? That's what errors are for.
Sorry, I thought "it" refered to the compiler.
Why do we need warnings at all? If it's worth mentioning, it's worth reporting as an error.
Something that some C compilers throw up a warning for, which is usually wrong most of the time, but is correct in this first example:
FILE* fp;
if (fp = fopen("Filename", "r)){

// do stuff with fp

}else{

// report file problem

}
This is to forestall problems with code such as
if (variable = otherVariableToCheckAgainst){...
where it is a logical error to assign the value, rather than compare for equality.

(Of course there's a way to silence that warning where the usage of = is correct, so this example's probably a bit meh.)

I see two problems with the first example that cause this problem:

The = operator returns a value
Errors are (presumably) returned in some global variable (leading to Error: Success)

PJH

@Ben L. said:

I see two problems with the first example that cause this problem:

The = operator returns a value

But that's the intended/desired behaviour in the first example. That's the point - it's warning about something that isn't a logical error. And neither example is a syntactic error, which is why they aren't errors.

Ben L.

@PJH said:

@Ben L. said:
I see two problems with the first example that cause this problem:

The = operator returns a value
But that's the intended/desired behaviour in the first example. That's the point - it's warning about something that isn't a logical error. And neither example is a syntactic error, which is why they aren't errors.

FILE *f = fopen("whatever", "rb");
if (f);
else;

= returning a value causes any use of that value to be either a hack or a bug. Solution: fix =

error

@Ben L. said:

= returning a value causes any use of that value to be either a hack or a bug. Solution: fix =

I don't think x = y = z = 0 is a hack or a bug.

Ben L.

@joe.edwards said:

@Ben L. said:
= returning a value causes any use of that value to be either a hack or a bug. Solution: fix =

I don't think x = y = z = 0 is a hack or a bug.

x, y, z = 0, 0, 0 or x = 0, y = 0, z = 0 depending on your language of choice would be correct. x = y = z is a hack.

morbiuswilters

@Ben L. said:

Why do we need warnings at all? If it's worth mentioning, it's worth reporting as an error.

I actually kind of agree, in a very theoretical sense. I think there's a problem in your reasoning, though, which is that there are conditions which we may want to bring to somebody's attention, but which are not true errors. So I would want to keep warnings and errors, but I think warnings should halt compilation/execution, just like errors. The difference is that a warning can be properly suppressed should the programmer determine that the condition is reasonable.

Speaking practically, though, you don't know what you're talking about. A lot of warnings exist due to historical behavior (C permitting assignment in a conditional, for example). Is it ideal that C lets this happen? No, but it's not going to change; there are billions of lines of code out there which would be impacted.

It's the same thing concerning the Java generics example which spawned this conversation: due to the way they are implemented, they aren't type-safe in certain circumstances. This was a deliberate choice by Sun, to maintain binary compatibility. We can bitch and moan, but it's not changing. It's not even an issue of just "fixing the compiler", but you'd have to get hundreds of thousands of libraries to re-compile using the new compiler. Now, I believe that Sun could have introduced the feature in a way so that by today nearly every library would have proper type information for generics, but they didn't.

PJH

@Ben L. said:

x, y, z = 0, 0, 0 or x = 0, y = 0, z = 0 depending on your language of choice would be correct. x = y = z is a hack.

No, it really isn't. However YM clearly does V.

morbiuswilters

@PJH said:

However YM clearly does V.

"Your Mom" clearly does "Vulcans"?

I don't think assignment returning a value is a hack, but I could see an argument for not allowing it.

I can also see an argument for keeping it, namely that it makes some constructs easier. And if we're going to eliminate every feature that somebody might goof up, then we're not going to have any features left. Personally, I have never, ever made the "did an assignment when I meant to do a comparison" bug. For one, it stands out like such a sore thumb that how could you not notice it?

Maybe I'm a freak, though, because I can pick out errors like that in code I've never seen just by glancing at someone's screen as I walk by. I also get a bit physically ill from poorly-formatted code.

Still, even if someone isn't a freak, shouldn't they catch the "assignment instead of comparison" bug in testing? I never understood understand stories like "Large Software Company had a major embarrassment and lost millions due to an assignment-instead-of-comparison bug." Did nobody actually test the code? You know, see what happens if that code path was executed? I'm hardly the most disciplined guy at writing unit tests (I quite hate it, honestly) but I do actually run my code once before committing.

PJH

@morbiuswilters said:

For one, it stands out like such a sore thumb that how could you not notice it?

Well, quite. On the other hand I find the following 'defense' quite jarring when I see it:

if (CONSTANT == variable)

As in

if ( 0 == fp){ /* handle null pointer */...

morbiuswilters

@PJH said:

@morbiuswilters said:
For one, it stands out like such a sore thumb that how could you not notice it?
Well, quite. On the other hand I find the following 'defense' quite jarring when I see it:
if (CONSTANT == variable)
As in
if ( 0 == fp){ /* handle null pointer */...

As do I. Although, in the example you provided, I'd write:

if (!fp) { ... }

Ben L.

@morbiuswilters said:

@PJH said:
@morbiuswilters said:
For one, it stands out like such a sore thumb that how could you not notice it?
Well, quite. On the other hand I find the following 'defense' quite jarring when I see it:
if (CONSTANT == variable)
As in
if ( 0 == fp){ /* handle null pointer */...
As do I. Although, in the example you provided, I'd write:
if (!fp) { ... }

You just converted a pointer to a boolean in an allegedly strongly-typed language. I HOPE YOU'RE HAPPY

morbiuswilters

@Ben L. said:

You just converted a pointer to a boolean in an allegedly strongly-typed language. I HOPE YOU'RE HAPPY

No, I didn't. I converted a pointer to an int in a weakly-typed language.

morbiuswilters

@morbiuswilters said:

@Ben L. said:
You just converted a pointer to a boolean in an allegedly strongly-typed language. I HOPE YOU'RE HAPPY

No, I didn't. I converted a pointer to an int in a weakly-typed language.

Actually, no I didn't. I compared a pointer to a pointer value in a weakly-typed language.

Severity_One

@Ben L. said:

@dkf said:
@Severity One said:
Actually, that's not even that bad. Given the way that generics are implemented in Java, occasionally you can't do otherwise than ignore "unchecked" warnings.
You can enable such ignorance on the level of a single local variable declaration. That's probably about the right level too; it's a “shut up stupid tool; this really is correct”. Putting it on large constructors is much more troublesome.
Or do it the right way: fix the bug (in your code or get the vendor to fix the compiler) and enable your compiler's version of -Werror

Just to get the point across, I stressed the essential point. It's a problem with Java, or its libraries. They had a choice on whether to remain backwards compatible, or do it the right way, when introducing generics in Java. No prize for guessing what choice they made.

morbiuswilters

@Severity One said:

Just to get the point across, I stressed the essential point. It's a problem with Java, or its libraries. They had a choice on whether to remain backwards compatible, or do it the right way, when introducing generics in Java. No prize for guessing what choice they made.

False dichotomy, I think.

Why couldn't they have included the type information for generics in new class files? So if you use an old compiler, you don't get type-safety. If you use a new compiler with old class files, you don't get type safety and you get a warning. If you're using both the new compiler and new libraries, you get type safety. Then the feature could have been slowly introduced, while maintaining backwards compatibility. And then by today nearly every library would support type-safe generics.

I mean, it is actually possible to integrate new features into a platform without breaking backwards compatibility, despite what Sun wanted you to believe. Hell, Apple included binaries and libraries for an entirely new architecture without breaking backwards compatibility. And this is Apple; they're not exactly know for engineering expertise.

beginner_

@Obfuscator said:

Was debugging today, ended up in the Hibernate source code. Turns out that the constructor of SessionFactoryImpl weighs in at >325 lines and contains nuggets such as a local class and sections where the comments give a perfect suggestion of a suitable method name for them. Lo and behold:

http://grepcode.com/file/repository.jboss.org/nexus/content/repositories/releases/org.hibernate/hibernate-core/4.1.10.Final/org/hibernate/internal/SessionFactoryImpl.java?av=f

Yeah I've had my fair share of looking at hibernate source code and after every time my inner rating of my own code and commenting and documenting style raises about 5-fold.

Severity_One

@morbiuswilters said:

Why couldn't they have included the type information for generics in new class files?

Good question, and I went digging a little. Apparently, the reasoning for using type erasure comes down to this:

The Java programming language implements generics using erasure, which ensures that legacy and generic versions usually generate identical class files, except for some auxiliary information about types. Binary compatibility is not broken because it is possible to replace a legacy class file with a generic class file without changing or recompiling any client code.

However, I fail to see why they couldn't change the class file structure for that. It's not like you can use a 1.5 class file with a 1.4 JVM. Then, there's this:

To facilitate interfacing with non-generic legacy code, it is also possible to use the erasure of a parameterized type as a type.
Such a type is called a raw type (Java Language Specification 3/4.8). Allowing the raw type also ensures backward compatibility for source code.

Fine, but the compiler could have easiy changed raw types to <Object> parameterised types.

So I still don't know what possessed Sun.

Source: http://docs.oracle.com/javame/test-tools/sigtest/2_2/html/a-compat.html#Z4000c521043377

morbiuswilters

@Severity One said:

However, I fail to see why they couldn't change the class file structure for that. It's not like you can use a 1.5 class file with a 1.4 JVM.

Yeah, and if the .class format were properly-designed, it shouldn't even matter. They should be able to add new sections with additional features that are skipped by old compilers/runtimes can't take advantage of the new features. It's pretty standard to add optional flags and fields for future expansion to binary formats; I don't know if Sun decided not to do it that way or if they just screwed up the .class format so badly it couldn't possibly work. And even if they couldn't extend the format without breaking it, the very least they could have done is just included a new file along with the .class that contains the type information for generics. It's how they implemented inner classes.

Severity_One

Found another bit:

It has been mentioned that implementing generics through erasure leads to some annoying limitations (e.g. no new T[42]). It has also been mentioned that the primary reason for doing things this way was backwards compatibility in the bytecode. This is also (mostly) true. The bytecode generated -target 1.5 is somewhat different from just de-sugared casting -target 1.4. Technically, it's even possible (through immense trickery) to gain access to generic type instantiations at runtime, proving that there really is something in the bytecode.

The more interesting point (which has not been raised) is that implementing generics using erasure offers quite a bit more flexibility in what the high-level type system can accomplish. A good example of this would be Scala's JVM implementation vs CLR. On the JVM, it is possible to implement higher-kinds directly due to the fact that the JVM itself imposes no restrictions on generic types (since these "types" are effectively absent). This contrasts with the CLR, which has runtime knowledge of parameter instantiations. Because of this, the CLR itself must have some concept of how generics should be used, nullifying attempts to extend the system with unanticipated rules. As a result, Scala's higher-kinds on the CLR are implemented using a weird form of erasure emulated within the compiler itself, making them not-entirely-compatible with plain-old .NET generics.

Erasure may be inconvenient when you want to do naughty things at runtime, but it does offer the most flexibility to the compiler writers. I'm guessing that's part of why it's not going away any time soon.

Source: http://stackoverflow.com/questions/313584/what-is-the-concept-of-erasure-in-generics-in-java

morbiuswilters

@Severity One said:

Found another bit:
It has been mentioned that implementing generics through erasure leads to some annoying limitations (e.g. no new T[42]).
It has also been mentioned that the primary reason for doing things
this way was backwards compatibility in the bytecode. This is also
(mostly) true. The bytecode generated -target 1.5 is somewhat different
from just de-sugared casting -target 1.4. Technically, it's even
possible (through immense trickery) to gain access to generic type
instantiations at runtime, proving that there really is something in the bytecode.

The more interesting point (which has not been raised) is that implementing generics using erasure offers quite a bit more flexibility in what the high-level type system can accomplish. A good example of this would be Scala's JVM implementation vs CLR. On the JVM, it is possible to implement higher-kinds directly due to the fact that the JVM itself imposes no restrictions on generic types (since these "types" are effectively absent). This contrasts with the CLR, which has runtime knowledge of parameter instantiations. Because of this, the CLR itself must have some concept of how generics should be used, nullifying attempts to extend the system with unanticipated rules. As a result, Scala's higher-kinds on the CLR are implemented using a weird form of erasure emulated within the compiler itself, making them not-entirely-compatible with plain-old .NET generics.

Erasure may be inconvenient when you want to do naughty things at runtime, but it does offer the most flexibility to the compiler writers. I'm guessing that's part of why it's not going away any time soon.
Source: http://stackoverflow.com/questions/313584/what-is-the-concept-of-erasure-in-generics-in-java

What the hell does he mean by "implementing higher kinds"? Oh, and it's great that Java's generics are basically crippled but, hey, Scala was easy to implement. "Sorry, sir, we had to amputate your legs, but at least that's one less thing you'll have to wash in the shower!"

Ben L.

@morbiuswilters said:

What the hell does he mean by "implementing higher kinds"? Oh, and it's great that Java's generics are basically crippled but, hey, Scala was easy to implement. "Sorry, sir, we had to amputate your legs, but at least ~~that's one less thing you'll have to wash in the shower~~ we're not making you use Java!"

FTFY