• 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
  • Bear Bibeault
  • Paul Clapham
  • Jeanne Boyarsky
  • Knute Snortum
Sheriffs:
  • Liutauras Vilda
  • Tim Cooke
  • Junilu Lacar
Saloon Keepers:
  • Ron McLeod
  • Stephan van Hulst
  • Tim Moores
  • Tim Holloway
  • Carey Brown
Bartenders:
  • Joe Ess
  • salvin francis
  • fred rosenberger

Code Review

 
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Praveen: i want to apologise. I really don't want to highjack your thread, but it has been a very interesting one untill now.

@Junilu: I konw that i'm in a very early stage in my development. To teel the truth my mind is yet very much formatted to structured programming rather than Object Oriented (OOP). It's not as easy as it could seem at first glance, to grasp all the nuances in OOP.

But i'll try to improve and the advices here are great for that.

I've tried to put the things that all have said here into action and came up with this:



It's not as straightforward as Praveens (it has one more variable declaration, if statement and method call) but also gets to the final desired output.

Best regards
carlos
 
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
First of all a much thanks to everyone here for your precious replies and suggestions.and thanks for the cow,Junilu.

@Carlos: I appreciate your reply,Thanks for it.Actually i am reading a book "Think like a programmer@Author:V. Ant Spraul".at the very first i tried to solve the very first example(it's a simple pattern,simply number of hashes is decrementing) in 2nd chapter and i succeed but when i match my solution with the one given by Ant,i thought(like you) Ant has redundantly used a verbose solution.but when i move on to the next example,i had realize why did he used the verbose one.actually what he want for his readers is whenever we see a problem,their can be plenty of constraints associated with it with a lengthy description so that it is too much difficult to find it's solution at a one go(because the complexity will make the solution less intuitive and have a great probability to render frustration to a solver).so he suggest that generally a problem(lengthy) has it's sub problems and we should have a very first task to find them,next is to solve the each part and finally we have to integrate those solution in parts to build our complete solution(probably the concept is called divide and conquer).I had realize that this concept is helpful in drawing almost any pattern.

Also,since i have already the solution in parts it's much easier for me to follow the single responsibility principle and thus i can refactor my code so easily.

As far as symmetry is concerned i had also used it in the term of modulus function which is itself symmetric and if you look at my post where i had my code for the star,see the 2nd code tag,you can see i have used the same triangle there which is helping me in code reuse.the other thing is middle spaces are also symmetrical about a verticle line passing through the centre of star,i have used it in the middleSpaces code(notice the factor 2).

Kind regards,
Praveen.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos wrote:@Praveen: i want to apologise...

You shouldn't be..At least on Ranch.however,if you think your problem is different from the given thread topic then feel free to create a new topic(just like a single responsibility principle).

Kind Regards,
Praveen.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Praveen: I'm glad you're not mad at me for all the posts i've made so far...

Thanks for the explanation from the book you're reading. Maybe sometime i'll get it also... When i gather a bit more knowledge.
I saw that you used the symmetry in your star printer... But honestly there are 2 symmetries there: one vertical (the one you used) and one horizontal: the bottom half is a symetry of the top one.

Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

praveen kumaar wrote:You shouldn't be..At least on Ranch.however,if you think your problem is different from the given thread topic then feel free to create a new topic(just like a single responsibility principle).


Well the problem is the same. But i've been using your thread to learn a bit hence the apologise... Maybe was a bad choice of words...

Best regards
Carlos
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Carlos: the variable totalLines should be final and may be not static.as if you would use it as a instance variable then different printer object can print the diamond of different sizes at a same time.and your code is not correctly indented,their shouldn't be a space b/w the method name and the brackets(you have done it at each line where ever you have declare a method).

Carlos wrote:It's not as straightforward as Praveens (it has one more variable declaration, if statement and method call) but also gets to the final desired output.

Now you have already one task to make it as staightforward like mine.

Kind regards,
Praveen.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos wrote:I saw that you used the symmetry in your star printer... But honestly there are 2 symmetries there: one vertical (the one you used) and one horizontal: the bottom half is a symetry of the top one.

I would not agree.i know their are 2 symmetries and i have used both of them.actually their is some math indulged here in my case the horizontal symmetry is like below:
I am attaining this symmetry with my modulus function("abs" in java,see the graph in my entry post).
you are thinking it with negative numbers like below
Think about it.Take your time,In fact in your later code you had also used the modulus function to attain the horizontal symmetry.

Actually i have not used the negative numbers because using index(starting from 0) is a more standard used in java(though i start with 1 in my code but it should be from 0,i note it after campbell mentioned it).
Kind regards,
Praveen.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Praveen: Yeah... Now i saw your use of the horizontal symmetry also.
I've used it in both my samples if you look at them. That's why my loop in first sample starts at a negative index.

praveen kumaar wrote:Now you have already one task to make it as staightforward like mine.


Honestly i think i'm forced to rest my case in this one...
The problem is line 5 that has zero spaces like line 4. To achieve this with my aproach there will allways be necessary to have a conditional statement somewhere.
It's true we could come up with a formula that could describe this point set but you used the best one already. A polinomial one would have to be like at least of 7th degree and that is an insane work to acomplish when a simple linear modulus function like yours does the job.

The best i came upon was this:



But honestly in my opinion this is even worse. Don't like it at all.

(By the way, i've put my totalLines as final and corrected the identation as you said)

Best regards
Carlos
 
Sheriff
Posts: 14755
245
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos Reves wrote:
But honestly in my opinion this is even worse. Don't like it at all.


What about that code don't you like? What makes it worse? Besides choosing betters names for some of those methods and following standard naming conventions (method names should start with a lowercase letter), what would you write instead?
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Still your code is not indented you have mixed two indent styles.look at line 13 there should be a space between the angled and the curly brackets.look at line 15 and 19 you will note the change in indent style.also use the {} braces in for loop(also in if else) even if it has single statement,it will render readability to your code.Have a look here→Code Formating.
class names should be a noun so change the name of the class to DiamondPrinter(or something which is relevant).also,the method name should be a verb but methods spacesNum and hashesNum should be spacesAt and hashesAt respectively(or something similar to it).also change the name of the print method to printDiamond(as it is appealing its work more better).
you can find these things in the later link i have given.

Kind regards,
Praveen.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Junilu: don't like those return statements with the condicionals. I may be wrong but i find them harder to understand.
I've eliminated the variable and the method call but for that now i have 2 conditionals.

@Praveen: missed that one on line 13... oops...

As for style, naming convention and so forth, yeah need to improve. Thanks for the advices!

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 14755
245
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos Reves wrote:@Junilu: don't like those return statements with the condicionals. I may be wrong but i find them harder to understand.
I've eliminated the variable and the method call but for that now i have 2 conditionals.


These decisions are judgement calls. Some people can't stand to read code like that while others are fine. I have a low tolerance for cryptic code and when I'm doing a code review, I demand that I should be able to understand the code as I am reading it. I don't want to spend more than 5 seconds deciphering what some piece of code is doing unless the algorithm is inherently complex and complicated.

In reality, I might try to factor out to the max until I can fully understand what the code does, then try to find a happy medium between being too verbose and too cryptic, essentially undoing some of the refactoring, usually by un-extracting some detailed calculations and inlining them back to where they originally were. Another refactoring I might try would be to introduce an explaining variable or two, like this:


Changes like this, of course, must be discussed with your teammates who will also work with the code. Giving them a chance to provide feedback and say, "Yeah, that makes more sense" or "Nah, that's not necessary, it was fine the way it was" helps you get a sense of the likelihood of someone later on either thanking you for considerately clarifying your intent or cursing you for not doing so.

Edit: Of course, I'd also make sure the method name itself was more expressive and followed conventions:

Another edit: or try this:

 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks to all for the valuable advices you gave me.
I'll keep them in mind in the future!

@Junilu: regarding the Shu Ha Ri... I'm definitely in the Shu phase!

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 14755
245
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carlos Reves wrote:@Junilu: regarding the Shu Ha Ri... I'm definitely in the Shu phase!


That may be true in terms of programming but think about other things you're good at. How did you get good at those things? Did you not go through a similar process of Shu Ha Ri?

I can easily relate to Shu Ha Ri because I've been a programmer for over twenty years and have studied Aikido and its related martial arts for over twelve years. I've also been a student and avid practitioner of Agile Software Development for over 15 years now. There's a common thread that runs through my learning paths in all these. I've found that Shu Ha Ri applies to any kind of learning that involves skill and creativity and therefore, the things that allow you to master one skill often come in handy when learning to master another.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
@Junilu: I was, obviously, refering to programming. (by the way, i didn't know this Shu Ha Ri. It's indeed interesting and yes, i do agree with its aproach to the learning process).
For now i'm learning and applying technics from others. I can't, yet, "think outside the shell" so to speak.

But it has been, so far, great to be here in this forum. It's, definitely, different from many others!

Thank you all for that!
 
Junilu Lacar
Sheriff
Posts: 14755
245
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
By the way, if you're not so big on the martial arts-related ideas, the Dreyfus Model of Skill Acquisition is a 5-stage model that's similar in concept to Shu Ha Ri.
 
Heroic work plunger man. Please allow me to introduce you to this tiny ad:
Sauce Labs - World's Largest Continuous Testing Cloud for Websites and Mobile Apps
https://coderanch.com/t/722574/Sauce-Labs-World-Largest-Continuous
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!