• Post Reply Bookmark Topic Watch Topic
  • New Topic

Is this function written in a good way  RSS feed

 
Vineeth Menon
Ranch Hand
Posts: 79
Eclipse IDE Java Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi folks,

I'need to write a function which can get the environment variables value. The input parameter would be as follows

${machine}/bin or ${machine/bin} or ${machine}/bin${path${OS}}/path
I would need to check the values which start with ${}
I've written a function which is kinda recursive in nature, but would appreciate it if anyone can review the code for me and tell me what can be done better

 
Jelle Klap
Bartender
Posts: 1952
7
Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Unless this is for educational purposes I wouldn't reinvent the wheel at all, and just use a library that handles variable substitution and interpolation. Something like StrSubstitutor from Appache Commons Lang 3.
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Vineeth Menon wrote:but would appreciate it if anyone can review the code for me and tell me what can be done better

I'm your huckleberry...

These are just for starters:

Problem #1:
This method is way too long and too convoluted. It would take someone coming in here maybe ten minutes to understand what is going on, despite your attempts to explain it. For me it would be like the door on the right in the graphic shown here (Edit: if that link doesn't work for you, try this one). And adding comments is NOT the solution here. Refactor to a Composed Method.

Problem #2:
The names loopBreaker, dollarCheck, and slashCheck are poorly chosen. They mislead the reader and bend their minds, like the Stroop Effect. When I see the words "check" and "break", I think of loop control and conditionals, not char literals. You're making my brain hurt by showing it names that make it bend over backwards to reconcile what the name implies, what the thing really is, and what you intend to do with it. If you really want to use symbolic names instead of character literals, then declare private static finals just above the method: TOKEN_END, TOKEN_START, PATH_SEPARATOR. This follows normal convention and gives the char literals names that correspond to their actual purpose.

Problem #3:
Don't make your code catch NullPointerException. NPE is a runtime exception and you should treat that as a bug or problem in the code that you as the programmer should fix. Your other catch block also smells fishy because all you're doing is printing the stack trace. That tells me you could probably put more effort into thinking about the exceptions that could be thrown by your code and what you can do about them.

I'll stop here for now because this is probably quite a bit for you to digest already. I would start refactoring to address Problem #2. This is the lowest hanging fruit and easiest one to resolve. Then I would address Problem #1. That will take a bit more effort to extract the details out of the method and leave the "essential abstract story" of what is happening. I would judge being "done" with this refactoring if you can get someone who has never seen the code read the refactored version and understand what it's doing in one minute or less. See the Composed Method example on the Industrial Logic website. That's how composed your method would ideally be. I require less than 10 seconds in my code reviews but 1 minute or less for your purposes will be a good start.
 
Vineeth Menon
Ranch Hand
Posts: 79
Eclipse IDE Java Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hey Junilu,

Thanks a lot for the inputs.. I'll try to make the changes as much as I can and then post it again...
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Vineeth Menon wrote:Thanks a lot for the inputs..

No problem.

I'll try to make the changes as much as I can and then post it again...

Sure, it should be a good exercise for you but mind what Jelle said before, don't reinvent the wheel if this isn't just for self-improvement.
 
fred rosenberger
lowercase baba
Bartender
Posts: 12563
49
Chrome Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Vineeth Menon wrote:...tell me what can be done better

The simple fact that this method has ZERO comments makes it unacceptable, IMHO. I try very hard to comment EVERYTHING except the most trivial of methods.
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
fred rosenberger wrote:I try very hard to comment EVERYTHING except the most trivial of methods.

Hmmm... I go the other way. I try very hard to refactor to a point where the code is expressive enough to make comments redundant. Go look at that Industrial Logic page for Composed Method. The refactored code needs no comment whatsoever. I reserve comments for the fairly rare occasions where I feel a real need to explain why the design is the way it is.
 
Vineeth Menon
Ranch Hand
Posts: 79
Eclipse IDE Java Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The simple fact that this method has ZERO comments makes it unacceptable,


When I did my masters, my professor used to tell us that programs should not have comments. As per him if anyone would read the code they should be able to understand what it's all about. But that's his opinion. When a code review is done especially in forums, it is necessary. I'll keep it in mind to add comments from now on . But I did very much like the suggestion that Junilu has about Composed methods, that seems like a great concept.
 
Paul Clapham
Sheriff
Posts: 22823
43
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Vineeth Menon wrote:When I did my masters, my professor used to tell us that programs should not have comments. As per him if anyone would read the code they should be able to understand what it's all about.


There's a lot of truth in that attitude. Here's an article by an author who doesn't care for comments: Why I don't read your code comments.
 
Jelle Klap
Bartender
Posts: 1952
7
Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There's a difference between comments and documentation, though. A method like this does beg for extensive API documentation.
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jelle Klap wrote:There's a difference between comments and documentation, though. A method like this does beg for extensive API documentation.

When you put it this way, I can wholeheartedly agree. Good documentation, priceless. Most comments, useless.

The following are links to threads with common problems I find with comments.

Comments are too long - I totally space out after reading the first line of those long comments. Also, the programmer was trying to explain things that more expressive code should be self-explaining (see Redundant comments below)

Comment clutter - I see this a lot in "professionally-written" code. These are comments that "hoarder" programmers make. They don't want to throw away anything so they just comment old lines out. This is all just clutter and it gets in the way of finding out the truth about the code.

Pointless/redundant comments - the comments are redundant or can be made redundant by renaming variables to be more expressive of their purpose. The code could be made much better just by Rename, Extract Method, and Composed Method refactorings.

Here's an example of the kind of developer comment I write when I feel the need to do so:

These and JavaDoc API Documentation are the only comments I deem acceptable when we do code reviews at work. Everything else needs to go into our supplementary documentation on the project wiki.
 
Dave Tolls
Ranch Foreman
Posts: 3056
37
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Non-Javadoc comments tend to exist in my code for only as long as I'm doing my dev, so a method would look something like:

as a first pass, which would become something like:

If there's comments still there after I've written the code then it's likely I've forgotten to do something.
There are the odd exception, for example explaining some logic that really doesn't belong in Javadoc in an environment that has no pre-existing wiki (as a contractor you tend to have to work with what you're given), but that's quite rare.
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Dave Tolls wrote:Non-Javadoc comments tend to exist in my code for only as long as I'm doing my dev, so a method would look something like:

Whoa... deja vu. Yeah, I used to do this. Nowadays, I find that sketching out a rough design on a whiteboard then TDD-ing the heck out of it makes inline pseudocode unnecessary. But yeah, I think this is a good use of comments as long as they eventually get replaced by code.

There are the odd exception, for example explaining some logic that really doesn't belong in Javadoc in an environment that has no pre-existing wiki (as a contractor you tend to have to work with what you're given), but that's quite rare.

Yup, but like you say, these are odd exceptions and I always take care to explain why rather than what. Most definitely not how because that's the code's job.
 
Dave Tolls
Ranch Foreman
Posts: 3056
37
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:
Dave Tolls wrote:Non-Javadoc comments tend to exist in my code for only as long as I'm doing my dev, so a method would look something like:

Whoa... deja vu. Yeah, I used to do this. Nowadays, I find that sketching out a rough design on a whiteboard then TDD-ing the heck out of it makes inline pseudocode unnecessary. But yeah, I think this is a good use of comments as long as they eventually get replaced by code.


Whiteboard?
Luxury...

I consider myself lucky if I've not been stuck in a basement somewhere with no phone signal.
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Dave Tolls wrote:I consider myself lucky if I've not been stuck in a basement somewhere with no phone signal.

Have you not experienced the kind of freedom to do whatever the heck you want that a basement with no phone signal affords you? Consider yourself blessed if you ever find yourself in one, my friend. I assume you're not a Milton Waddams, of course
 
Vineeth Menon
Ranch Hand
Posts: 79
Eclipse IDE Java Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Non-Javadoc comments tend to exist in my code for only as long as I'm doing my dev, so a method would look something like:




Deja vu... I' do the same thing....
 
Junilu Lacar
Sheriff
Posts: 11477
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Vineeth, just curious but what did you end up doing to improve the code you originally posted?
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!