I'm doing code review on a change my co-worker made to our Java application, and I've found something I'm not very familiar with - a nested class.

From reviewing the code, it seems like the nested class is being used as if it were a normal class - and asking my co-worker about it, the reason for putting it in as a nested class is because of a source control issue preventing her from creating a new class on the day she coded it.

Now this is bothering me - because while there's no reason to introduce this element into our code (very few classes use nested classes in our application), there's also no drawbacks to it that I can think of either. The nested class is, in a very loose way, related to the class it came from, and re-writing the code so that the nested class is an independent one would take some time.

Is there any good reason to have my co-worker redo the code so that this nested class is independent? Or would I just be asking them to waste their time on something that does not matter?

Note that there does not appear to be any functional affect on implementing the class this way - so any argument would have to be from best practice or bad structure, rather than trying to prove that it doesn't work (because it does work - I'm just not sure it's appropriate).

  • 1
    Are there any drawbacks to using a nested class instead of declaring a new one? -- Yes; you can't use it anywhere else. In practice, nested classes should be a very rare occurrence; they are an indicator that the containing class is already too complicated. – Robert Harvey Nov 9 at 16:40
  • Is the nested class marked as private? – David Arno Nov 9 at 16:42
  • @DavidArno No not at all - which to me indicated that there's really no need to use it. – Zibbobz Nov 9 at 16:51
  • @RobertHarvey I would agree with you - but the original class was actually very simple. – Zibbobz Nov 9 at 16:51
  • @NickAlexeev This definitely made me turn my head significantly - so I'm inclined to put this on the 'bad code' side of things. Especially since it's something we'd have to explain to someone further down the line if they need to fix it. – Zibbobz Nov 9 at 17:03

If the nested class is private then it is part of the implementation details of that class. There are various valid reasons why such a class might exist: data encapsulation, proving a private implementation of an interface to name two.

If the class is accessible outside of its containing class, then the single responsibility principle likely comes into play. Is that inner class genuinely a responsibility of the outer one? Since you say it’s only loosely related, then the answer is likely, no.

Nesting classes tightly couples them. It makes the outer class more complex as it now contains two class’s worth of functionality. And that outer class now has two responsibilities. And all this exists because of a problem with a check-in.

So I’d definitely recommend restructuring the code in this case too move that inner class out into its own file where it belongs.

Given that it was only created due to a problem with source control rarther than any design considerations then yes. You should probably refactor it.

However, if the code is functional and you don't have a "no nested classes" coding style rule then I would not recommend you fail the code review because of it.

I'm obligated to post this cartoon about the measure of code quality.

this cartoon about the measure of code quality

If the only reason for creating the nested class instead of a free-standing class was a clerical restriction [source control is an automated clerk], then the nested class should be refactored into a free-standing class. If there are no other reasons, this should be an easy refactoring.

  • Yeah. If the answer to "Why is this a nested class?" is "Because source control wouldn't allow me to create a new file" then it definitely garners one or two WTF's. And maybe an AYFKM!? ... and maybe a link to Let Me Google That For You with the search terms "how to use <Source Control X>" – Greg Burghardt Nov 9 at 17:36
  • 1
    Sarcasm aside, this is a teaching opportunity. Teach the coder to reach out for help in situations like this, so you don't fail a code review for such a silly reason. – Greg Burghardt Nov 9 at 17:37

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.