• Post Reply Bookmark Topic Watch Topic
  • New Topic

Cleaner way to make code  RSS feed

 
Nikki Smith
Ranch Hand
Posts: 65
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I don't know if there's a better way to write that, and even asked my prof, but she said no. With where we're at and what we're "supposed to know", that's the best that can be done. I'm required to have that constructor with 9 parameters and because of how the data is being passed into that method she said that if I wanted to shorted the code a bit I could just leave field[0] etc. as is and pass those values into the constructor instead, but for the sake of readability while being forced to go this route, I chose to add in Strings to give each of the field indexes some sort of meaning. So this works, but is there a neater way of writing this?

 
salvin francis
Bartender
Posts: 1664
37
Eclipse IDE Google Web Toolkit Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If you want a cleaner way to build objects, you could look at the Builder design pattern.
Have a look at Tim's example here : https://coderanch.com/t/678116/java/classes
 
Randall Twede
Ranch Hand
Posts: 4696
8
Java Scala
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
looks fine to me
 
Rajith Pemabandu
Greenhorn
Posts: 23
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Suppose you code is ok and do the job . You can reduce the lines of codes more if you want.


 
Norm Radder
Rancher
Posts: 2240
28
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'd pass str to the constructor and require the constructor to know the String's format for parsing and saving its contents.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
As you can see, there are lots of ways you can clean up code and many different levels of "clean".  Here are some of the approaches I'd try:

1. Principles: Clarify, simplify, separate concerns, and assign responsibilities.

Norm's suggestion to pass the string to the Contact constructor would be the first to consider. It seems reasonable but here's the problem with it: it ties the domain object to the data store representation too tightly. Why would a domain object like Contact care how it is stored in a file? I'd prefer to have a separate class that's responsible for knowing the format of the data in the text file, say a ContactFileParser class.

But first, let's clean up the code you have so that it's at a higher level of abstraction. Simpler code is more abstract code, without the noise of implementation details.

The parameter / name could use some rethinking. Do you want to pass in a Scanner or the file name? I'd prefer the file name. I'd also change the method signature to return a List<Contact> instead.

That means that the high-level code would look like this:

But looking at that, the importContacts() method seems like an unnecessary intermediate step now. So maybe I'd eliminate it so I can just write this in the high-level code:

There, that's a lot cleaner at this level. Now we deal with the mess of code that used to be at this level, only we push it down deeper, into the ContactFileParser class.

(continued in my next reply)
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So, now we're going to push all those details about the file format and mapping to a Contacts constructor down into a class whose sole responsibility is to know the format and how to map the format to a Contact object.

The high-level method has very little code and it just defines the main looping logic. It delegates to another method to do the actual parsing of each line.

This is what's called functional decomposition - break big tasks into smaller tasks and just delegate. This way, even a big task like parseAllAsList doesn't need to be loaded up with a lot of code. You just keep pushing down implementation details into other methods.
(continued in next post)
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Now we get to the fun part. How do we get rid of the hard-code field numbers? You don't want to hard-code those field numbers, so you create some symbolic constants:

This is one way. It uses a little "trick" with the variable on line 17. That's a legal variable name but one that's not commonly used. In fact, in Scala, that's a special token that means "I don't care what this is called, it's just a placeholder variable."  The Go language has something similar in its iota and that one is even easier to use. But this is Java...

Anyway, that's basically a throwaway variable. It gets initialized when the class is loaded. Then it gets incremented each time you initialize the constants on the lines after it. Finally, you set the FIELD_COUNT to whatever its value is after the last field constant was initialized. This allows you to check if each line has the expected number of fields.

The advantage of doing it this way is that it's more flexible and amenable to changes in the file format.  Need to add or delete a field? Simple, just add or delete a constant in the correct position. The formatting of the Contact constructor call is also purposely done that way to facilitate maintenance. You simply insert or delete a line.

 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You could also do this differently with another Scanner.  You will just need to tell the scanner to useDelimiter("\\|") so that it recognizes the pipe character as the token separator. Then you just call the next() method. So basically, you'll just have:

But that's not very readable because you have no idea what contact field each f.next() call maps to. You'd have to use comments in that case to clarify the code:

or
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
But go back to what someone said earlier about using the Builder pattern if you have a constructor that has that many parameters. The Builder pattern is a much better approach, I think, but anyway it was a fun exercise to clean up the code the way I showed above.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:
Norm's suggestion to pass the string to the Contact constructor would be the first to consider. It seems reasonable but here's the problem with it: it ties the domain object to the data store representation too tightly. Why would a domain object like Contact care how it is stored in a file? I'd prefer to have a separate class that's responsible for knowing the format of the data in the text file, say a ContactFileParser class.

This is a demonstration of using design principles to guide your decision making.  There is a set of design principles represented by the acronym SOLID. The "S" stands for "Single Responsibility Principle" and it states in part that a class should have only one reason to change.  That change happens when there's a change in its responsibility.  If a class has multiple relatively independent responsibilities, then it will have multiple, unrelated reasons to change. That's not good because when the class changes, you need to test everything that is dependent on that class. If you mix responsibilities, there is also a greater risk of inadvertent bugs getting created when a change is made to one responsibility. So, you want to keep each responsibility isolated and encapsulated in a class. That's why you want to keep the responsibility of knowing how to parse a text file and map the parsed data to a Contact object separate from the Contact object itself.  A Contact object should only have domain knowledge (business logic and how it is related to other domain objects).  Knowledge of file formats is infrastructure knowledge. That's why you keep it separate from the domain object.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!