tags:

views:

464

answers:

10
+2  Q: 

Anything wrong with this kind/type of coding style?

Is there anything wrong with the below code? It is java codes, but somehow it looked like a C program to me. If there is something incorrect with the OO implementation, can you tell me the name of the violation? looking at S.O.L.I.D or CodeSmell but I still have no idea why the below codes looked weird to me. 

public void root() {
    BufferedWriter bw=new BufferedWriter ()
    writeHTMLHeader(bw);
    writeBody(bw);
}
public void writeHTMLHeader(BufferedWriter bw) {
    bw.write("<html><head>");
    ...
    bw.write("</head>");
}
public void writeBody(BufferedWriter bw) {
    bw.write("<body>");
    ...
    //pull some information 
    writeMenu(br);
    ...
    bw.write("</body></html>");
}
public void writeMenu(BufferedWriter bw) {
    Integer count=0;
    writeSubMenu(bw, count);

    bw.write("Some other menu items");
}
public void writeSubMenu(BufferedWriter bw, Integer count) {
    bw.write("Some submenu items");
        count=count+1;
}

so it is OK to pass bw and other variables (and changing them) while passing down the chain of methods call?

A: 

Writing on a buffered reader seems a trifle malodorous.

The question has now been corrected to use a buffered writer throughout.

The triple-letter names are not very meaningful.

The inconsistency without obvious reason of having ddd() called from one of the others, instead of having them all called by root() is a little odd.

The blank lines in (some but not all) of the triple-letter functions are not needed.

As illustrated, frankly, none of the triple-letter methods buy anything. However, we should probably assume the 'real' code would have more activity per method.

Jonathan Leffler  2009-03-13 13:01:02
A: 

I think your method are suppose to take a BufferedWriter, not a BufferedReader

You should write the methods so that they take the least specific type necessary to do their job. In this case you'd want to define the methods to take a Writer as this will allow you to pass any type of Writer to the methods.

Sean  2009-03-13 13:02:10
A: 

Too little code to make a proper statement. However you might like the Visitor Design Pattern

You could think of making bufferedWriter an instance variable and use it directly instead of passing it as an argument.

siddhadev  2009-03-13 13:02:10
+3  A: 

Apart from the fact that it's br (implying "buffered reader") instead of bw, it's hard to say given the information you've provided (assuming, of course, that "Some stuff" is actually something useful, and the methods have more helpful names than aaa, bbb, ccc and ddd.

I can imagine situations where this sort of code is exactly the right way to do it, and situations where this sort of code could not be more wrong.

David Wolever  2009-03-13 13:02:32
A: 

I would use a "try{} finally{}" to close the reader / writer object in the finally block.

-> always ensure anything that can be closed is closed

also your aaa ... ddd methods may need to be set as private, but that depends of the other part of the code.

-> Less exposure implies less problems

chburd  2009-03-13 13:05:42
A: 

It's a lot of code bloat!

Try to generalize aaa,bbb ... and replace the inner writes etc. with if{}else{} paths depending on your (generalisation-)controlling parameter list.

The visitor design pattern is also a way, but it offers complexity.

BufferedWriter br=new BufferedWriter ()

Isn't globaly defined! You should do it in the class!

Martin K.  2009-03-13 13:07:07
A: 

Meaningful method names?

Craig Warren  2009-03-13 13:07:23
A: 

Having ddd() appear before ccc() seems counter-intuitive at least to me.

I guess the method names are not real, but shortened for the example. This, however, makes the question impossible to answer for me.

If the methods names in the real code are descriptive, this code might be the result of some well-executed refactoring, at some stage. Perhaps ddd() is a long step, refactored out of bbb() for some valid reason?

unwind  2009-03-13 13:12:10
A: 

I don't see anything wrong based on what you have given us. While there are other design patterns that could solve this problem let's look at a better example. Assume we are rendering some Html. (This is example will be loosley based on C#)

public void Render(StreamWriter stream)
{
   WriteTableBegin(stream);

   DataSource.ForEach(row=>WriteRow(stream,row));
}

private void WriteTableBegin(StreamWriter stream)
{

   //Cut this logic to keep it simple but we are building attributes etc...;
   string tableBeginTag=
   stream.Write(tableBeginTag);
}

private void WriteRow(StreamWriter, object row)
{
  //etc...
}

In my opinion this code is easy to write and easy to read and understand. The point of Render is to write it's Html to the stream. This is using a Composite Method refactoring to provide well named scoped methods which do one thing and do it well.

JoshBerke  2009-03-13 13:20:01
A: 

IMO, using raw Java code to make HTML markup doesn't properly exercise a separation of concerns. JSF, Wicket, etc. were all designed to allow the markup to be markup and the logic to be logic. Sure, there may be times where it is better to use code to generate markup but those are few and far between.

Keeping your Markup separate allows you to quickly design/redesign the interface when the functionality doesn't change and allows you to change the functionality when the interface doesn't change.

As far as the code itself, from a code smell point of view, I'd want to see the html/root tag being emitted in root rather than subfunctions. Each function would emit the tags appropriate to its use and nothing else.

Drew  2009-03-13 13:40:28

related questions

Interface Segregation in Qt
Is the set of SOLID principles missing an extra 'D'?
is this violating the SOLID principles ?
SOLID principles
How such an important principle "OCP" will be the reason of massive code duplication practice?
Single Responsibility Principle vs Anemic Domain Model anti-pattern
What is your best example of a violation of the Single Responsibility Principle?
Cancel pattern for background thread
Confused about Single Responsibility Principle in the following example
How does this "Programming to Interfaces" thing work?
How to implement SOLID principals into an existing project
It this an example of the Single Responsibility Principle?
How to build the ViewModel in MVVM not to violate the Single Responsibility Principle?
What are the OSS .net/java projects which has utilised most SOLID Principles in a rightway?
Open source projects that demonstrate TDD and SOLID priciples
Examples of great software design and implementation
SOLID and the user interface ?
Do you put Linq2SQL queries all over the place or in dedicated DAL classes?
Data Transfer Object Pattern??
Patterns to implement SOLID principles
How to separate data validation from my simple domain objects (POCOs)?
Monkey-patching Vs. S.O.L.I.D. principles?
How do you define a Single Responsibility?
The Open/Closed Principle