• Post Reply Bookmark Topic Watch Topic
  • New Topic

Code For Review: Dummy Data Generator  RSS feed

 
Simon Ritchie
Ranch Hand
Posts: 134
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi folks,

I've written a small application that generates junk/mock/dummy data for relational database tables.  It works by reading in the SQL description (DESC) statement of whatever table the user wishes to generate data for and using this statement to secure information on the table's fields - their data types, whether they are nullable, their lengths and names, etc.  Based on this data structure the app will then generate a row of random alphnumeric/numeric/date data for each field.  Users can specify how many rows of data they want generated.  Once this data has been generated it is written to a file (location specified by the user) in the form of a number of SQL INSERT statements that can be run in any SQL development IDE of the user's choosing.

In as many places as possible I've tried to use a design pattern that allows for dependency injection further down the line.  Interfaces are used to abstract away the fundamental functionality of the implementing classes.

Any and all feedback is welcome.  I plan to start work on a GUI version of the tool at a later stage.

Thanks

Main.java (Main class)



Data.java



Field.Java



Reader.java



Writer.java



Parsing.java



ParsingImpl



Preparing.java



PreparingImpl.java



Processing.java



ProcessingImpl



Randomise.java



RandomiseImpl.java

 
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
I have a number of things to give feedback on so I'll post one reply per point.

First, formatting is good. You might think this is trivial and maybe even condescending but way too many programmers are sloppy with their code formatting and indentation.

Your choices for names in your program are pretty decent: almost no abbreviations or cryptic names. This makes your code clear and expressive. The one exception that jumps out at me are the pl() methods. You seemed to go out of your way to avoid writing System.out.println() but you also polluted many of your classes with duplicated code. I personally wouldn't have made that tradeoff.

You also have a number of places where you call pl() in a catch clause. A better practice would be to use a logging framework like Log4J or Logback instead of System.out.

These are small things but they do bring up a bit of concern. If I was evaluating you as a potential hire, I would ask you to explain your motivations for doing these things. So, as a way to practice, why did you choose to do these things?
 
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
Another thing I would ask you to explain is your choice to have interfaces like Parsing and Preparing and implementations for each interface. What was your thought process behind this aspect of your design? What was the motivation behind this?
 
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
Also, that's a heck of a lot of code to write without a single line of unit tests. Many professional developers consider any code that doesn't have unit tests to be legacy code, even if it was written just a few moments ago. I would ask you why you don't have any unit tests. Your answer here would determine what the next questions would be.
 
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
One thing about your interface names though: Type names should generally be nouns or noun phrases; the names Processing, Parsing, and Randomize are verbs. I would ask you to try to find better names for these interfaces, ones that are nouns instead of verbs.
 
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 also have small sections of code that are commented out and you have a number of // TODO line comments left in. I would ask why you left them there. These may seem trivial but they're like misspelled words on a resume: they are off-putting and they can give the reader the impression that the author is either sloppy or doesn't pay attention to details.
 
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
Why is Field.length declared as a String? Most people expect "length" to be a numeric value so declaring it as a String is counterintuitive. It also leads you to write this kind of code in your ProcessingImpl class:

First of all, prefer to format that code like this:

This is more readable and it's also easier to maintain because you can easily add statements to the if or else part of the statement. The bigger problem with this statement is that all that casting and converting of types from String to numeric is unnecessary if Field.length was declared as an int in the first place. You probably wouldn't even need the size variable at all.

A variable or field should be declared as the type that best fits its purpose and usage.
 
Simon Ritchie
Ranch Hand
Posts: 134
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks for all the feedback Junilu

Junilu Lacar wrote:The one exception that jumps out at me are the pl() methods. You seemed to go out of your way to avoid writing System.out.println() but you also polluted many of your classes with duplicated code. I personally wouldn't have made that tradeoff.

You also have a number of places where you call pl() in a catch clause. A better practice would be to use a logging framework like Log4J or Logback instead of System.out.

These are small things but they do bring up a bit of concern. If I was evaluating you as a potential hire, I would ask you to explain your motivations for doing these things. So, as a way to practice, why did you choose to do these things?


The only reason I included those pl() methods was simply laziness, I'm afraid - it's a convention I've used for a while to save me having to type System.out.println() all the time.  The Log4J suggestion you've made is good.  At present I don't know how to implement Log4J but I'll look up some tutorials and find out!
 
Simon Ritchie
Ranch Hand
Posts: 134
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Another thing I would ask you to explain is your choice to have interfaces like Parsing and Preparing and implementations for each interface. What was your thought process behind this aspect of your design? What was the motivation behind this?


I'm glad you've raised this as it's something I'm not entirely clear on myself and I'd appreciate input on the "correct" way to do things.  The motivation behind using lots of interfaces with implementations behind them came from a design pattern principle I read in one of the Head First book series that dealt with polymorphism and inheritance using the example of a "Duck" class.  In the book's example, it provided a "Bird" superclass with a subclass "Duck".  Rather than writing the implementations of Duck behaviours (flying, quacking) into the Duck class it recommended using interfaces to represent these behaviours (e.g.: "interface Flying", "interface Quacking" and then write implementations for those interfaces (e.g. "class FlyingImpl, class QuackingImpl") so that you have loose coupling.

This is what I was trying to achieve in my design here.  I might come up with a better way of parsing, preparing or processing data in future and I'd rather not strip out or rewrite the whole class.  I figured that if I created interfaces that have methods representing the different behaviours I need of my classes then I'd be able to chop and change implementations as I pleased.

If this is the wrong approach, please let me know.

As for the interface names - I thought it was okay to name them after behaviours but if nouns are considered better practice I'll refactor them.
 
Simon Ritchie
Ranch Hand
Posts: 134
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Also, that's a heck of a lot of code to write without a single line of unit tests. Many professional developers consider any code that doesn't have unit tests to be legacy code, even if it was written just a few moments ago. I would ask you why you don't have any unit tests. Your answer here would determine what the next questions would be.


I wrote several Junit tests, I just forgot to include them in the original post - sorry!

I'll post them here later though as I don't think I did an especially good job with them (I'm still learning the essentials of Test Driven Development).
 
Simon Ritchie
Ranch Hand
Posts: 134
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Why is Field.length declared as a String? Most people expect "length" to be a numeric value so declaring it as a String is counterintuitive.


You're quite right - I simply forgot to change these variables to their numeric equivalents!
 
Liutauras Vilda
Sheriff
Posts: 4928
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I find too much empty spaces in your formatting. But that's might be the matter of taste.

1. Variables: retval - isn't instantly clear what it represents. String quot - typo.

2. Offending code below. 2.1 Instead equals("") you could have used isEmpty() - fits better to the context. [/tt] 2.2. Why do you use System.exit(0) there? 2.3 What's the story about else there dangling? Where braces gone?

3. validateArguments(args[0], args[1], args[2], args[3]); You could have passed args array and extract elements within validateArguments method, that way you'd shorten the amount of parameters you need to pass. In case more parameters appear, you wouldn't need to change caller part.

4. Again, braces.. What you win by that? Readability? - Thanks but no.

5.
That is a very long list of parameters. Consider passing them in java.util.Properties object, so whenever new parameter comes in or gets removed, you don't need to change constructor and fix everywhere else it was used.

6. Reader class. Uses old techniques reading file. Discover what Java 8 has to offer for you.
this.readFile = false; - very subtle, but I'd find more clear if it would have been named fileIsRead, while your chosen readFile sounds more like a method name as isn't clear which form of read is used until you go and understand context.

Probably there are more things could be mentioned. But you putted a lot of effort - congratulations on job.
 
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
Simon Ritchie wrote:
Junilu Lacar wrote:Another thing I would ask you to explain is your choice to have interfaces like Parsing and Preparing and implementations for each interface. What was your thought process behind this aspect of your design? What was the motivation behind this?

I'm glad you've raised this as it's something I'm not entirely clear on myself and I'd appreciate input on the "correct" way to do things.  ... This is what I was trying to achieve in my design here.  I might come up with a better way of parsing, preparing or processing data in future and I'd rather not strip out or rewrite the whole class.  I figured that if I created interfaces that have methods representing the different behaviours I need of my classes then I'd be able to chop and change implementations as I pleased.

If this is the wrong approach, please let me know.

It's good that you're trying to learn TDD - it's an excellent practice, IMO, and I wish more developers tried to learn how to do it properly. One thing TDD teaches you to do is to be more sensitive to what your code/design is saying and respond appropriately to it instead of anticipating what your code/design needs. One motivation for introducing an interface into a design is to facilitate testing. Take a Data Access Object (DAO) for example. A DAO interface sits between the business layer and the persistence layer. Since unit tests need to be fast, you don't really want to use a DAO implementation that connects to the real database. An interface makes it easier to unit test business layer objects that need to interact with a DAO because mocking frameworks like Mokito can easily create a mock implementation that can be controlled and configured to simulate what a real DAO implementation might do.

The added complexity of an interface is worth it in cases like this and experienced developers will automatically create one for a DAO because they already know for sure that it's needed. It's worthwhile for less experienced developers to go through the longer process of discovering the need for an interface though, so you can understand the context better. When you start with a unit test and find that there's a collaborator of the class being tested that is difficult to control and configure, that's when you might start thinking about introducing an interface so you can mock out the hard-to-use collaborator.

My rule of thumb is that interfaces are probably worthwhile if they sit between two or more architectural layers. Something like a Writer might warrant an interface. However, a Parser is kind of in the same architectural layer as the classes that will use it so introducing an interface is probably not something I'd do until I have a test that tells me it would benefit from a different implementation or something that can be more easily controlled and configured.

An alternative to interfaces is an extensible class like an abstract base class. Choosing between an interface vs. abstract class is nuanced by whether or not an inheritance hierarchy of implementation objects makes sense. You can get a sense for when to choose one over another if you look at examples like the List interface and its different implementations versus something like AbstractMap and its subclasses in the Collections API. See https://docs.oracle.com/javase/tutorial/java/IandI/abstract.html for more about considerations for choosing one over another. Also search for articles about "when to use abstract base class versus interface"
 
Simon Ritchie
Ranch Hand
Posts: 134
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Luitauras.  My responses to your feedback:

1. Have changed variable name "retval" to "uniqueString" as this better represents what the method is returning.  Have changed the variable name "quot" to "quote".

2.1 Have now used the isEmpty() method. 
2.2 I was just using System.exit(0) to end the programme in the event of validation of arguments being unsuccessful.  There's currently no Text User Interface in the programme that allows for keyboard input but in a future version I plan to replace the System.exit(0) calls with calls to a method that will allow users to enter new arguments.
2.3 Have now updated this

3. Have updated the "validateArguments" method to now take a String[] parameter and I am passing the arguments array.

4. Have updated this.

5. I've not used the Properties object before and in the case of this programme I'm reluctant to do so as potentially many different Field objects will need to be created dynamically so I'd rather have a plain old constructor available.  Is this poor practice?

6. Have updated the "Reader" class to now use Streams to get file input and have changed the variable "readFile" to "fileWasRead".

Main.java


Reader.java



ParserImpl.java



PreparerImpl.java



ProcessorImpl.java



RandomerImpl.java

 
Simon Ritchie
Ranch Hand
Posts: 134
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Also, that's a heck of a lot of code to write without a single line of unit tests. Many professional developers consider any code that doesn't have unit tests to be legacy code, even if it was written just a few moments ago. I would ask you why you don't have any unit tests. Your answer here would determine what the next questions would be.


Here are the Junit tests I wrote.  These are probably not ideal tests - they were written after I wrote the code.

ReaderTest.java



WriterTest.java



ParserTest.java



PerparerTest.java



ProcessorTest.java



RandomerTest.java

 
Consider Paul's rocket mass heater.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!