7

My project at current job use private access modifier for methods of MVC controllers:

@Controller
public class HelloWorldController {

    @RequestMapping("/helloWorld")
    private ModelAndView helloWorld() {

I have integrated PMD and his report has a lot of:

/src/main/java/com/web/controller/SignalController.java:91: Avoid unused private
                                            methods such as 'handleNewRequest()'.

So instead of disabling useful PMD rule I think to change controller's methods visibility to public.

Are there any reasons to keep controller's methods private?

gavenkoa
  • 45,285
  • 19
  • 251
  • 303
  • 4
    Not only is it pointless (at least in my opinion), but it also means that they're not being tested. Part of the value of Spring (and it's ability to return data the way that it does) is that you can very easily unit test your controllers without having to worry about raw HTML parsing (or other ugly tactics). – Colin M Jul 10 '13 at 14:56
  • Agree with Colin. Appears to be no value in keeping your controller methods private. In general testing private methods requires reflection and is a pain. If you have to test private methods you can use something like JMockit's [Deencapsulation](http://jmockit.googlecode.com/svn/trunk/www/javadoc/mockit/Deencapsulation.html) util. – Durandal Jul 10 '13 at 14:59
  • 1
    We use **SpringJUnit4ClassRunner** with **HandlerAdapter** and **MockHttpServletRequest** + **MockHttpServletResponse**. So keeping method private doesn't make testing impossible )) – gavenkoa Jul 10 '13 at 15:00
  • 2
    You're shooting yourself in the foot by making it private: 1. it's seen as unused by PMD and the IDEs (and, transitively, all the other private methods it calls, too). So you or a colleague might mistakenly remove private methods that are actually used. 2. It makes it much harder to **unit**-test them. 3. It's unconventional, making your code look weird to experiences Spring developers. 4. They're logically public,since they're called by code outside of the class and package. – JB Nizet Sep 02 '17 at 22:56
  • @JBNizet Do you mind to make comment as answer? – gavenkoa Sep 04 '17 at 06:43

1 Answers1

13

You're shooting yourself in the foot by making it private:

  1. it's seen as unused by PMD and the IDEs (and, transitively, all the other private methods it calls, too). So you or a collegue might mistakenly remove private methods that are actually used.
  2. It makes it much harder to unit-test them.
  3. It's unconventional, making your code look weird to experienced Spring developers.
  4. They're logically public, since they're called by code outside of the class and package.
JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • I have seen the auto-wired dependencies are `null` inside a private @RequestMapping method, and when I made it public the dependencies were available. – VPK Oct 12 '17 at 03:35
  • 1
    1. They would be seen as unused in an IDE regardless, since Spring executes them with reflection. 2. This is true, though personally I would use an Integration Test, not a Unit Test, to test a RestController, where again, they are executed with reflection. 3. Agreed 4. Agreed FYI, I came here to see about the downside of making them package-access, to discourage other devs from hard-coding direct calls to them. – Jason Hendriks Jan 20 '23 at 16:37