• Post Reply Bookmark Topic Watch Topic
  • New Topic

Trying not to overdo interfaces, abstract classes and concrete classes  RSS feed

 
Jon Swanson
Ranch Hand
Posts: 230
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I am trying to refactor some code so that it can be used for a new set of requirements. The original code was written by someone else who was not worrying at all about re-use (or documentation). Here is what I have found.

The program has several components that read and write session directories. The new requirements add a bunch of functions that are supposed to work with any of these sessions. Right now there are independent classes and methods for every component.

So I started here (checkers are component specific classes that implement isValid() and the session is valid if all the checkers return true):






Which was looking good until I discovered that all the original classes had only static methods and that isValidSession() was called all over the place as MySession.isValidSession( checkers ) at times when it wasn't known specifically which component was involved (a generic checker was used). The result is that I believe, I have to support the ability to use isValidSession( checkers ) in places where loadSession() is not implemented.

The production environment is Java 7 or I would be tempted to make isValidSession a static method of the interface.

What I did was:



Which allows



and using some generic non-specific checker. But it just doesn't feel quite right (I don't like that stub for loadSession()). I'm wondering if I should just drop the interface and go back to an abstract class, but with static methods (at least for the generic functions).

Could I get some comments on whether my approach is making sense or there is a better way of thinking about this?
 
Jesper de Jong
Java Cowboy
Sheriff
Posts: 16060
88
Android IntelliJ IDE Java Scala Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jon Swanson wrote:Which was looking good until I discovered that all the original classes had only static methods ...

If those classes had only static methods, then it wasn't originally designed in an object oriented way. I would try to refactor it to get rid of those static methods. What you did until now looks good.

Jon Swanson wrote:The result is that I believe, I have to support the ability to use isValidSession( checkers ) in places where loadSession() is not implemented.

The isValidSession() method that you showed doesn't depend on the loadSession() method. I don't understand why you need to make SessionClass non-abstract and have an empty implementation of loadSession().

You'll, in the end, always want to use a specific implementation of class Session. Why do you want to be able to instantiate the non-specific class SessionClass - why can't you just call isValidSession() on an instance of the specific implementation of Session that you're going to use?


Maybe the method isValidSession() doesn't belong in class SessionClass, but in a separate class.

 
Jon Swanson
Ranch Hand
Posts: 230
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Jesper,

There are places in the existing code where the following is done:


And other places where the following is done



Exactly how much of the session needs to be in place depends on the context, i.e., sometimes only a subset of the session files need to be in place and sometimes all of them. So the latter is used where specific things must be there and the former, when you just don't want to be pointing to totally the wrong thing. For example, rewriting a session only checks you aren't about to rewrite your home directory. But for analysis, building the session had to have worked. Exactly what files are valid depends on the type of analysis being done.

I think the code was written while the author was still of two minds about how to implement this.

I think part of my problem is trying not to change all of the existing code. In the current code isValid( checkers ) is a call to a static method. There is no indication as to what type of session is being checked. Sometimes it does not matter, the same checker applies to any session type. I suppose I could have a GenericSession class, that extends the abstract class. But it would still have an empty load method. Is this any better?

I don't feel good about having isValid() in one class and isValid( ArrayList<SessionChecker> ) in another. Some new request will come along and I will forget about the existence of one or the other of these if they aren't together.

 
Tim Harris
Ranch Hand
Posts: 57
3
Chrome Eclipse IDE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'd agree with you about not having the isValid() check in multiple places.

While I definitely feel like I don't see the entire scope of your problem, this feels like it should take a modular approach. So how I would do it is to have the outermost class loads its context relative components, each component implementing whatever checkers it needs as it's loaded in. Then, when the isValid() method is invoked, the class checks on validity based on the loaded components.

Calling a class level (static) implementation of isValid(checkers) seems to be treating isValid() as a sort of utility method. You could make it work either way, but in the end I'd dispose of one or the other.

Hope this helps.
 
Jon Swanson
Ranch Hand
Posts: 230
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks. I am thinking I would rather not have isValid in two classes, so since I think Session.isValid() is going to stay, which makes me lean toward having an overloaded isValid() in the Session class. The logic that seems to be making this hard is something like this (in pseudo-code).



There isn't a one-one mapping between checkers and session type. Otherwise the checkers for a session would only need to be provided once. Sometimes the session will be used for several things, the ability to do each one needing to be checked. I think it's the fact the current code separates looking for a session of any kind from finding out what kind of session will be used that is the complication. But I can't change the work flow at this stage, so I'm trying to make the most logical choices given the constraints.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!