12

I was wondering about good object oriented class design. In particular, I have a hard time deciding between these options:

  1. static vs instance method
  2. method with no parameters or return value vs method with parameters and return value
  3. overlapping vs distinct method functionality
  4. private vs public method

Example 1:

This implementation uses instance methods, with no return value or parameters, with no overlapping functionality, and all methods public

XmlReader reader = new XmlReader(url);
reader.openUrl();
reader.readXml();
Document result = reader.getDocument();

Example 2:

This implementation uses static methods, with return values and parameters, with overlapping functionality and private methods

Document result = XmlReader.readXml(url); 

In example one, all methods are public instance, which makes them easy to unit test. Although all methods are distinct, readXml() is dependent on openUrl() in that openUrl() must be called first. All data is declared in instance fields, so there's no return values or parameters in any method, except in the constructor and accessors.

In example two, only one method is public, the rest are private static, which makes them hard to unit test. Methods are overlapping in that readXml() calls openUrl(). There's no fields, all data is passed as parameters in methods and the result is returned immediately.

What principles should I follow to do proper object oriented programming?

3
  • 3
    Static things are bad when you do multi-threading. The other day, I had a static XMLWriter, like XMLWriter.write(data, fileurl). However, since it had a private static FileStream, using this class from multiple threads at the same time, caused second thread to overwrite first threads FileStream, causing an error which would be very hard to find. Static classes with static members + multi-threading is a recipe for disaster. Aug 29, 2011 at 12:22
  • 3
    @Paxinum. The problem you describe is a state problem, not a "static" problem. If you used a singleton with non-static members you would have the same issue with multi-threading.
    – mike30
    Apr 25, 2013 at 17:49
  • 4
    @Per Alexandersson Static methods are not bad in relation to concurrency. Static state is bad. This is why functional programming, in which all methods are static, works very well in concurrent situations. Sep 27, 2019 at 19:47

8 Answers 8

11

Example 2 is quite bad for testing... and I don't mean that you can't test the internals. You also can't replace your XmlReader object by a mock object as you have no object at all.

Example 1 is needlessly hard to use. What about

XmlReader reader = new XmlReader(url);
Document result = reader.getDocument();

which is not any harder to use than your static method.

Things like opening the URL, reading XML, converting bytes to strings, parsing, closing sockets, and whatever, are uninteresting. Creating an object and using it is important.

So IMHO the proper OO Design is to make just the two things public (unless you really need the intermediate steps for some reason). Static is evil.

2
  • -1. YOu actually CAN replaced XmlReader with a mock object. Not witha brain dead open source moick framework, but with a good industrial grade one you can ;) Costs a couple of hundreds USD per developer but works wonders to test sealed functionality in API's you publish.
    – TomTom
    Feb 5, 2012 at 11:06
  • 2
    +1 for not lisening to TomTom's sales pitch. When I want a one liner I'd rather write something like Document result = new XmlReader(url).getDocument(); Why? So that I can upgrade it to Document result = whateverReader.getDocument(); and have whateverReader handed to me by something else. Sep 26, 2019 at 19:39
6

Here's the thing -- there is no one right answer, and there is no absolute definition of "proper object oriented design" (some people will offer you one, but they are naive ... give them time).

It all comes down to your GOALS.

You are an artist, and the paper is blank. You can draw a delicate, finely penciled black and white side portrait, or an abstract painting with huge gashes of mixed neons. Or anything in between.

So what FEELS RIGHT for the problem you are solving? What are the complaints of the people who need to use your classes to work with xml? What is hard about their job? What kind of code are they trying to write that surrounds the calls to your library, and how can you help that flow better for them?

Would they like more succinctness? Would they like it to be very clever at figuring out default values for parameters, so they don't have to specify much (or anything) and it guesses correctly? Can you automate the setup and cleanup tasks your library requires to that it is impossible for them to forget those steps? What else can you do for them?

Hell, what you probably need to do is code it 4 or 5 different ways, then put on your consumer hat and write code that uses all 5, and see which feels better. If you can't do that for your entire library, then do it for a subset. And you need to add some additional alternatives to your list too -- what about a fluent interface, or a more functional approach, or named parameters, or something based on DynamicObject so that you can make up meaningful "pseudo-methods" that help them out?

Why is jQuery the king right now? Because Resig and team followed this process, until they came across a syntactic principle that incredibly reduced the amount of JS code it takes to work with the dom and events. That syntactic principle wasn't clear to them or anyone else when they started. They FOUND it.

As a programmer, that's what your highest calling is. You grope around in the dark trying stuff till you find it. When you do, you'll know. And you'll be giving your users a huge productivity leap. And that is what design (in the software realm) is all about.

12
  • 1
    This implies that there are no differences between best practices and other practices other than how it "feels." This is how you wind up with unmaintainable balls of mud--because to many developers, reaching out across Class boundaries, etc., "feels" just freaking wonderful. Apr 25, 2013 at 20:39
  • @Amy Blankenship, I would say unequivocably that there is no one "best way" to make the choices the OP is asking about. It depends on a million things, and there are a million degrees of freedom. However, I do think there is a place for "Best Practices," and that is in a team environment, where certain choices have already been made, and we need the rest of the team to stay consistent with those earlier choices. In other words, in a specific context, there might be reasons to label certain things "best practices." But the OP hasn't given any context. He is building something and ... Apr 26, 2013 at 3:36
  • ... he faces all these possible choices. There is no one "right answer" to those choices. It is driven by the goals and the pain points of the system. I guarantee you that Haskell programmers don't think all methods should be instance methods. And Linux kernel programmers don't think making things accessible to TDD is very important at all. And C++ game programmers would often rather bundle their data into a tight data structure in memory than encapsulate everything into objects. Every "Best Practice" is only a "best practice" in a given context, and is an anti-pattern in some other context. Apr 26, 2013 at 3:43
  • @AmyBlankenship One more thing: I disagree that reaching out across Class boundaries "feels just freaking wonderful." It leads to unmaintainable balls of mud, which feel horrible. I think you're trying to solve the problem that some workers are sloppy/unmotivated/very inexperienced. In that case, someone who is careful, motivated & experienced should make the key choices, & call them "Best Practices." But, the person choosing those "Best Practices" is still making choices based on what "feels right," and there are no set right answers. You're just controlling who makes the choices. Apr 26, 2013 at 3:48
  • I have worked with several programmers who thought of themselves as senior level and were thought of that way by management who firmly believed that statics and Singletons were absolutely the right way to handle communication issues. The static part of this question would not have even been asked if reaching out across Class boundaries like that "feels" wrong to developers, nor would the answer that advocates the static alternative receive any up votes. Apr 26, 2013 at 14:52
3

The second option is better as it's simpler for people to use (even if it's just you), much simpler.

For unit testing I'd just test the interface not the internals if you really want to then move the internals from private to protected.

2
  • 2
    You might also make your methods package private (default), if your test cases are in the same package, for the purposes of unit testing.
    – jeshurun
    Jul 23, 2011 at 5:32
  • Good point - that's better.
    – Tom
    Jul 23, 2011 at 6:35
3

1. Static vs. instance

I think there are very clear guidelines about what is good OO design and what isn't. The problem is that the blogosphere makes it hard to separate the good from the bad and ugly. You can find some kind of reference supporting even the worst practice you can think of.

And the worst practice I can think of is Global State, including the statics you mentioned and everyone's favorite Singleton. Some excerpts from Misko Hevery's classic article on the subject.

To really understand the dependencies, developers must read every line of code. It causes Spooky Action at a Distance: when running test suites, global state mutated in one test can cause a subsequent or parallel test to fail unexpectedly. Break the static dependency using manual or Guice dependency injection.

Spooky Action at a Distance is when we run one thing that we believe is isolated (since we did not pass any references in) but unexpected interactions and state changes happen in distant locations of the system which we did not tell the object about. This can only happen via global state.

You may not have thought of it this way before, but whenever you use static state, you’re creating secret communication channels and not making them clear in the API. Spooky Action at a Distance forces developers to read every line of code to understand the potential interactions, lowers developer productivity, and confuses new team members.

What this boils down to is that you should not supply static references to anything that has some sort of stored state. The only place I use statics is for enumerated constants, and I have misgivings about even that.

2. Methods with input parameters and return values vs. methods with none

The thing you need to realize is that methods that have no input parameters and no output parameters are pretty much guaranteed to be operating on some sort of internally stored state (otherwise, what are they doing?). There are entire languages that are built on the idea of avoiding stored state.

Any time you have stored state, you have the possibility for side-effects, so make sure that you always use it mindfully. This implies that you should prefer functions with defined inputs and/or outputs.

And, in fact, functions that have defined inputs and outputs are much easier to test--you don't have to run a function here and go look over there to see what happened, and you don't have to set a property somewhere else before you run the function under test.

You can also safely use this type of function as statics. However, I wouldn't, because if I then later wanted to use a slightly different implementation of that function somewhere, rather than providing a different instance with the new implementation, I'm stuck with no way to replace the functionality.

3. Overlapping vs. Distinct

I don't understand the question. What would the advantage be in 2 overlapping methods?

4. Private vs. Public

Don't expose anything you don't need to expose. However, I'm not a big fan of private, either. I'm not a C# developer, but an ActionScript developer. I've spent a lot of time in Adobe's Flex Framework code, which was written circa 2007. And they made some really bad choices of what to make private, which makes it kind of a nightmare trying to extend their Classes.

So unless you think you're a better architect than the Adobe developers circa 2007 (from your question, I'd say you have a few more years before you have a chance to make that claim), you probably want to just default to protected.


There are some problems with your code examples which mean that they're not well-architected, so it's not possible to pick A or B.

For one thing, you probably should separate your object creation from its use. So you usually wouldn't have your new XMLReader() right next to where it is used.

Also, as @djna says, you should encapsulate the methods used in your XML Reader uses, so your API (instance example) might be simplified to:

_document Document = reader.read(info);

I don't know how C# works, but since I've worked with a number of web technologies, I'd be suspicious that you wouldn't always be able to return an XML document immediately (except maybe as a promise or future type object), but I can't give you advice on how to handle an Asynchronous load in C#.

Note that with this approach, you could create several implementations that can take a parameter that tells them where/what to read and return an XML object, and swap them based on your project needs. For example, you might be reading directly from a database, from a local store, or, as in your original example, from a URL. You can't do that if you use a static method.

2

Focus on the client's perspective.

IReader reader = new XmlReader.readXml(url);  // or injection, or factory or ...
Document document = reader.read();

Static methods tend to limit future evolution, our client is working in terms of an interface provided by possibly many different implementations.

The major problem with your open/read idiom is that the client needs to know the ordering in which to call methods, when he only wants to get simple job done. Obvious here, but in a larger Class it's far from obvious.

The principle method to test is read(). Internal methods can be made visible to test programs by making them neither public nor private and putting tests in the same package - the tests can still be kept separate from the released code.

2
  • Are methods with default visibility still visible, if the test suite is in a different project?
    – siamii
    Jul 23, 2011 at 23:41
  • Java does not know about projects. Projects are an IDE construct. The compiler and JVM look at the packages that the tested and tester classes are in - same package, default visibility is permitted. In Eclipse I use a single project, with two different source directories. I've just tried with two projects, it does work.
    – djna
    Jul 24, 2011 at 7:09
2

static vs instance method

In practice you'll find static methods are generally confined to utility classes and shouldn't be cluttering your domain objects, managers, controllers or DAOs. Static methods are most useful when all the necessary references can reasonably be passed in as parameters and profide some functionality that can be reused across many classes. If you find yourself using a static method as a workaround for having a reference to an instance object, ask yourself why you don't just have that reference instead.

method with no parameters or return value vs method with parameters and return value

If you don't need parameters on the method, don't add them. Same goes with the return value. Keeping this in mind will simplify your code and ensure you aren't coding for a multitude of scenarios which never end up happening.

overlapping vs distinct method functionality

It's a good idea to try to avoid overlapping functionality. Sometimes it can be difficult, but when a change in logic is necessary, it's a lot easier to change one method which is reused, than changing a whole bunch of methods with similar functionality

private vs public method

Commonly getters, setters and constructors should be public. Everything else you'll want to try to keep private unless there is a case where another class needs to execute it. Keeping methods defaulting to private will help maintain Encapsulation. Same goes for fields, get used to private as default

1

I won't answer to your question, but I think that an issue is created by the terms you have used. E.g.

XmlReader.read => twice "read"

I think you need an XML so I'll create an object XML that can be created from a textual type (I don't know C#... in Java it's called String). E.g.

class XML {
    XML(String text) { [...] }
}

You can test it and it's clear. Then if you need a factory, you can add a factory method (and it can be static like your 2nd example). E.g.

class XML {
    XML(String text) { [...] }

    static XML fromUrl(url) { [...] }

}
0

You can follow some simple rules. It helps if you understand the reasons for the rules.

static vs instance method

For methods you do not have to make this decision consciously. If it appears your method is not using any field members (your favorite analyser should tell you so), you should add the static keyword.

method with no parameters or return value vs method with parameters and return value

The second option is better because of scope. You should always keep your scope tight. Reaching out for stuff you need is bad, you should have input, work on it locally and return a result. Your first option is bad for the same reason global variables are typically bad: they are meaningful to just a part of your code but they are visible (and thus noise) everywhere else and may be tampered with from anywhere. This makes it hard to get a complete picture of your logic.

overlapping vs distinct method functionality

I do not think this is an issue. Methods calling other methods are fine if this chops up tasks into smaller distinctly functional chunks.

private vs public method

Make everything private unless you need it to be public. The user of your class can do without the noise, he wants to see only what matters to him.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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