• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Paul Clapham
  • Ron McLeod
  • Bear Bibeault
  • Liutauras Vilda
Sheriffs:
  • Jeanne Boyarsky
  • Junilu Lacar
  • Henry Wong
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Jj Roberts
  • Tim Holloway
  • Piet Souris
Bartenders:
  • Himai Minh
  • Carey Brown
  • salvin francis

Design review

 
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would like to get some reviews for the following code. The application generates payslips after computing tax based on tax slabs, given an annual income. I've posted one or two bits of this in a different post in this forum, but I would like to post the complete code for review here in one place.





























 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here are some test classes:













 
Marshal
Posts: 71019
291
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Why isn't your validation method static?
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Why isn't your validation method static?



Interface won't allow me to make it static.
 
Campbell Ritchie
Marshal
Posts: 71019
291
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Where are you using that interface? I can't see any Validator<XYZ> declarations anywhere. That casts doubt on the wisdom of having that interface at all.
Please check the regex in name validator carefully. I think it may not work. What happens if you feed, “  ” to it?
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Where are you using that interface? I can't see any Validator<XYZ> declarations anywhere. That casts doubt on the wisdom of having that interface at all.
Please check the regex in name validator carefully. I think it may not work. What happens if you feed, “  ” to it?



All 3 validation classes implement that interface. Is that not a good thing to do?

Yes, the name validator will break in that case. I will correct that.
 
Sheriff
Posts: 15989
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
First comment (haven't looked at anything else): the NameValidator tests are still lacking, in my opinion. They tell a very vague story. Basically, if it's valid, the validator returns true, false otherwise. Not very helpful. What makes an input valid or invalid? That should be very clear from reading the test but it's not. I see that "#" is apparently not allowed but is that all? Will it reject a name with any special character? Many people have hyphenated names so will it accept "Zeta-Jones," for example? What about ", Jr."? Again, the test just does not spell it out.

A better test name might be should_only_have_letters_and_space -- that tells the reader exactly what will be accepted and what will be rejected.
 
Junilu Lacar
Sheriff
Posts: 15989
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
What the name should_accept_only_letters_and_space also does is it makes you think of special cases that you might have missed. Would it be acceptable to have a name like "A B C D E F G"? That kind of string will be accepted by the validator.
 
Sheriff
Posts: 7108
184
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One thing I noticed is that InputSifterUtils and InputValidator use the same String to compile a pattern.  I would rather put this near the top of InputSirterUtils:
and then use this in InputValidator instead of the local variable.
 
Junilu Lacar
Sheriff
Posts: 15989
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
PayslipFrequency is a strange-looking (smelly) enum. It only has one value and it's dependent on the Pay class.
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:What the name should_accept_only_letters_and_space also does is it makes you think of special cases that you might have missed. Would it be acceptable to have a name like "A B C D E F G"? That kind of string will be accepted by the validator.



Should the name validate be changed to this? Or should I change the name of the variable to something like this?

I have the interface as validate, that's why I had to call this validate.
 
Junilu Lacar
Sheriff
Posts: 15989
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I wasn't suggesting to change the method name from validate. Since that's an interface method, it should be generic enough to hide details specific to each implementation. It is the test method that doesn't give a specific enough story when it should.

It seems you didnt get what I meant in my first reply. It's not enough to say that validate returns true if valid and false if invalid. The test should make it clear exactly why you expect true or false to be the result. Reading the test code still makes me guess as to exactly why a name that happens to contain # would be rejected. Is it just # that gets rejected? What other characters would get rejected? Those questions should be answered by tests, not by having to go and read the production code.
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you, Junilu. I've updated those. Is the name for the false case good enough? Or too wordy? What's the next thing I can look at? I intend to take this as an involved learning experience, so please point out whatever things you feel are not right. I will work on one thing at a time.



Also, I can see that using underscores rather than camel case here has improved readability significantly. Is that OK to do for tests? I know Java follows camel case.
 
Marshal
Posts: 7839
538
Mac OS X VI Editor BSD Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Prasanna Raman wrote:


Which implementation you had in mind when created this interface?
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Liutauras Vilda wrote:

Prasanna Raman wrote:


Which implementation you had in mind when created this interface?



Sorry, I don't understand what you mean by which implementation?
 
Liutauras Vilda
Marshal
Posts: 7839
538
Mac OS X VI Editor BSD Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

PaySlipGeneratorApplication


I don't like how the PaySlip is spelt. I'd prefer reading it PayslipGenerator, and I'd remove Application word from it. Whad kind of application that is if it contains only static methods inside?

As a result of static methods and no internal state of a class, you have such calls:

As you see you pass the same variable 3 times: input, input, input to the same utility classe's methods.
 
Liutauras Vilda
Marshal
Posts: 7839
538
Mac OS X VI Editor BSD Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Prasanna Raman wrote:

Liutauras Vilda wrote:

Prasanna Raman wrote:


Which implementation you had in mind when created this interface?



Sorry, I don't understand what you mean by which implementation?


Probably not everyone would find it important...

NameValidator you could relate well with interface's method parameter validate(T name), but for MonthlyPayslipFrequencyValidator when the input is really a frequency, argument "name" seems to be off. I'd have called it simply input in the interface's method's validate parameter.
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Liutauras Vilda wrote:

PaySlipGeneratorApplication



As you see you pass the same variable 3 times: input, input, input to the same utility classe's methods.



I've changed the spelling, but I don't know what to do about the class name, because there is another class named PayslipGenerator.

Also, how can I change it so that I don't have to pass the same input thrice?
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Liutauras Vilda wrote:

Prasanna Raman wrote:

Liutauras Vilda wrote:

Prasanna Raman wrote:


Which implementation you had in mind when created this interface?



Sorry, I don't understand what you mean by which implementation?


Probably not everyone would find it important...

NameValidator you could relate well with interface's method parameter validate(T name), but for MonthlyPayslipFrequencyValidator when the input is really a frequency, argument "name" seems to be off. I'd have called it simply input in the interface's method's validate parameter.



Yes, valid point. In fact, I did change it but I think I might have forgotten to save it. It didn't read natural for me either when I was looking at my classes so I remember changing it to input. I've changed it again now (and saved it too!)
 
Ranch Hand
Posts: 143
6
IntelliJ IDE Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Unrelated, but do you use version control like Git ? Is this code from a book ?
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Tom Joe wrote:Unrelated, but do you use version control like Git ? Is this code from a book ?



No, it's not from a book. It's just my code on my computer. I am using IntelliJ which usually saves it but I may have undone it unintentionally while I was doing some refactoring.
 
Liutauras Vilda
Marshal
Posts: 7839
538
Mac OS X VI Editor BSD Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Why PayslipFrequency enum has method generate()? That looks to me like incorrectly assigned responsibility.
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Liutauras Vilda wrote:Why PayslipFrequency enum has method generate()? That looks to me like incorrectly assigned responsibility.



Yes, I was debating this too but probably ended up making the wrong choice. I initially had it in the generate method itself of the PayslipGenerator class?

Should it be there? Also, how do I get rid of passing the same input multiple times?
 
Junilu Lacar
Sheriff
Posts: 15989
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Prasanna Raman wrote:Is that OK to do for tests? I know Java follows camel case.


Yes, it's fine to do it for tests. If you're on a team that follows certain coding standards, be sure to discuss it with them though. This is a convention I first saw the author Neal Ford use and I liked it so much more than camel caps that I've used it ever since.

As I was saying before, the names are not only clear and exact about what the tests are trying to document, but they also help identify possible gaps in your tests. For example, does the validation accept all spaces? What about single words? And again, what about hyphenated names?
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you Junilu. I'll keep this mind. Please comment on the next aspect you'd like me to look into.
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Where are you using that interface? I can't see any Validator<XYZ> declarations anywhere. That casts doubt on the wisdom of having that interface at all.



Do you mean other than just implementing the interface? No, it's never used outside of that because I just directly used new AnnualSalary() etc.
 
Campbell Ritchie
Marshal
Posts: 71019
291
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Wouldn't it read something like this?
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Wouldn't it read something like this?



Yes, but I've just used it like this:

 
Junilu Lacar
Sheriff
Posts: 15989
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would look over the rest of the tests and make them more specific. For example, the InputValidatorTest seems to overlap the NameValidator test in that it's detecting an invalid name. This in turn seems to overlap the InputSifterTest in that it seems the InputValidator needs to extract the Name in order to do its job. None of the tests are specific enough so you have to bounce from one class to another to fully understand what the test name seems to only hint at. Again, a generic name like "shouldReturnTrueWhenValid" is not useful and leaves too much to question.
 
Junilu Lacar
Sheriff
Posts: 15989
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
More specific feedback about the InputValidatorTest.

To reiterate, the main problem is lack of specificity. Basically the tests are saying "If the input is valid, return true, otherwise return false." Well, duh. But what makes the input invalid or valid? One thing you can only infer from looking at the tests is that "Mary Song123" looks like it's supposed to be invalid. Why isn't this in the NameValidatorTest then? The input looks like it's made up of three distinct values that need to be parsed out. Does the order matter? Does the spacing matter? Can there be a \n in between? Should the delimiter always be space? Should any item that contains space be delimited with double quote? I'd expect the tests to give examples that clearly spell out the answer to some of these questions.
 
Prasanna Raman
Ranch Hand
Posts: 539
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you Junilu. Are these better?

NameValidatorTest:


AnnualSalaryValidatorTest:


MonthlyPayslipFrequencyValidatorTest:


InputValidatorTest:


I don't like the fact that I am just repeating what it's in the individual tests in the InputValidatorTest, possible smell for bad design? But I am not sure how to solve that yet. I don't know if I should test just the format separately and then have the other validations separately or combine them like I've done in the InputValidator.

I'll wait for your inputs on these before I do any more refactoring.
 
Never trust an airline that limits their passengers to one carry on iguana. Put this tiny ad in your shoe:
the value of filler advertising in 2020
https://coderanch.com/t/730886/filler-advertising
reply
    Bookmark Topic Watch Topic
  • New Topic