I am looking at some legacy code in java that uses the Service Locator pattern as a way of passing global references as opposed to passing them over and over again through method arguments and I am looking to refactor this approach. First of all this is a good approach or does it indicate code smell ? If it is code smell Would the best way to refactor this code is by introducing Dependency Injection either constructor injection or setter injection ?
Yes, many people consider Service Locator to be an anti-pattern. Depending on how it is implemented it can be difficult to unit test code that uses it, it reduces flexibility because clients can only have a single implementation of each interface, and it relies on run-time type testing so various errors can only be detected at run-time (often only during execution, if clients defer locating services until they are needed).
Dependency injection via constructor parameters is the most common preference. It makes the dependencies of a class an explicit part of its interface. In a statically typed language it allows the compiler to verify that all dependencies are satisfied (unless using a framework that uses reflection, in which case the language runtime ensures this during the container initialisation). And testing in isolation is trivial.
You probably don't need a DI framework. They can save some work, but doing DI by hand gives you the benefit of static type checking, which is worth the extra work a lot of the time.