• 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 Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Code Review

 
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I am currently reading a book Think like a programmer by V. Anton Spraul(as someone suggested me).i have attempted the very first 2 problems of a book.i am mentioning first one:
1st problem from the mentioned book:
print a pattern:

Here is the approach i have used:
Number of linesNumber of spaces corresponding to a lineNumber of Hash symbol
132
224
316
408
508
616
724
832
i have used a concept of coordinate geometry to find a relation b/w the lineNum and Number of spaces.i am assuming the Number of lines as a "X" co-ordinate and the number of spaces as Y.i have found the equation of lines with coordinates (1,3),(2,2) is "Y = -X + 4" but only the first 4 coordinates are lying on it.the equation for the last 4 coordinates is "Y = X -5".here is the graph:
.
here i am interested in the bold portion and as the absolute value of slope of both the lines is 1 and they are intersecting at point(9/2,-1/2) so i can find a equation for a portion in term of modulus.with the concept of transformation(or shifting origin) i have the equation as "Y -(-1/2) = |X - 9/2|" or simply "Y = (|2X - 9|-1)/2".each coordinate is lying on this equation.
similarly for printing hashes i have assumed the number of lines as X coordinate and number of hash symbols as Y-coordinate and found the relation as "Y = 9 - |2X - 9|".
I have explained these things because if i had not then you people may find it hard to understand the logic inside the for loop in my code.here is my code below now:


I have my results as expected but i just want to get some reviews for my code and please let me know if there is any other better approach to solve it.

Kind Regards,
Praveen.
 
Bartender
Posts: 2911
150
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Is a singleton necessary in the above program ?

Even if it is, you are eagerly initializing it here:

Why do you need to use instanceCounter to ensure it's a singleton ?

 
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
1. Don't add features that weren't asked for unless there's a very compelling reason to do so. This is what engineers call gold-plating, which is not a good thing. Adding code to make the class a Singleton only opens you up to questions about your implementation and the necessity of it at all. Your use of the counter variable to guard against creating more than one instance is a questionable approach and seems unnecessary.

2. Your comments don't follow a consistent pattern and most of them are not really necessary. Line 12 for instance is only stating the obvious. Line 15 is also misleading in that it claims to introduce a "static factory" where the method doesn't really fit the definition of a factory method. (EDIT: Bloch does in fact call getInstance() a static factory. See my other reply below)

3. Your use of hard-coded values on lines 29, 36, and 46 is questionable.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

salvin wrote:Is a singleton necessary in the above program ?

may be not but since the class is stateless i thought it should be and also the joshua bloch has mentioned it to enforce this pattern for stateless classes.

salvin wrote:Even if it is, you are eagerly initializing it here:
?
1
private static final DiamondPrinter printer = new DiamondPrinter();

Why do you need to use instanceCounter to ensure it's a singleton ?


what's wrong in iniltializing it there.suppose by mistake if i instantiate the class anywhere else in my code then the counter will help me to remind this property by throwing an exception.

Praveen
 
Sheriff
Posts: 7125
184
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I like that you indented your code.  Four spaces is a little more standard, though.

You have long lines in your code.  Pick either 80 or 120 columns and (almost) never go over it.

I like that you have your code broken into methods.  I question the necessity of one method: print().  Typing System.out.print() a couple of times isn't worth a method.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

praveen kumaar wrote:what's wrong in iniltializing it there.suppose by mistake if i instantiate the class anywhere else in my code then the counter will help me to remind this property by throwing an exception.


That seems like a pretty far-fetched supposition. The constructor is private so why would you be in here, see that the constructor is private, see the static getInstance() method and then think to yourself, "Hmmm, I think I'll just instantiate another one of these over here..."

Sound kind of like being on a diet and saying that you slipped and "accidentally" ate the cake that was sitting on the kitchen counter.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I, too question the need for that print() method, especially since it leads you to write line 49 which tends to make the reader think, "Huh, that looks weird. Could have just written System.out.println()"  -- Try not to violate the Principle of Least Surprise, even when it comes to following programming idioms.

If you really wanted to extract something to a method, it should have been those weird looking expressions like numOfSpace <= (((Math.abs(((2 * lineNum) - 9))) - 1)/2) on line 29.  Someone reading that has to sit there and really analyze what the intent of that formula is. If you had put it behind an expressive method name instead, then your code would be clearer. Something like numOfSpace <= leadingSpacesFor(lineNum) is far more expressive and easy to understand.
 
salvin francis
Bartender
Posts: 2911
150
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

praveen kumaar wrote:may be not but since the class is stateless i thought it should be and also the joshua bloch has mentioned it to enforce this pattern for stateless classes.


I am really curious about this. Could you quote your source where bloch has asked to enforce this ? The way I always thought about stateless class was a class which had static methods and didn't use instance variables like the way you did. Maybe I am wrong here ?
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
In fact, I would probably have been inclined to refactor to this instead:

The calculations on lines 66 and 70 for the number of leading spaces and number hash marks would be relative to the lineNum parameter and the constant HEIGHT.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

salvin francis wrote:I am really curious about this. Could you quote your source where bloch has asked to enforce this ? The way I always thought about stateless class was a class which had static methods and didn't use instance variables like the way you did.


I wrote:Line 15 is also misleading in that it claims to introduce a "static factory" where the method doesn't really fit the definition of a factory method.


Ok, I dug through Bloch 2nd Ed. and Item 3 does refer to the getInstance() method in the singleton example as a static factory method. Bloch writes further, however, stating that "One advantage of the factory-method approach is that it gives you the flexibility to change your mind about whether the class should be a singleton without changing its API." This an important nuance to note, something I have to admit I never really thought much about before but it does make sense to call it a factory method if you're thinking along those lines. You learn something new everyday.  

I've always viewed those getInstance() methods that return a singleton instance as a "static accessor" and nothing more.

The only reference to "stateless" that I found in the 2nd Ed that might apply to this discussion was under Generics (Item 27) where Bloch writes, "It would be wasteful to create a new one each time it’s required, as it’s stateless. If generics were reified, you would need one identity function per type, but since they’re erased you need only a generic singleton."  I don't know if it really applies in this situation though.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
This is more a matter of style/form than anything but it also has to do with readability. I read code from top to bottom. I expect high-level public code to be up top and detailed private code to be down towards the bottom. I also like to keep static members together towards the top of the class and instance members below them. I find that this organization generally allows me to scan a class and get a general feel for its capabilities much faster.  Thus, in my code, where there is a main() method, it is generally one of the first methods encountered when reading from top to bottom, followed by whatever other static methods there may be, followed by constructors, then high-level public methods, and then the more detail/implementation-focused private methods to round out the bottom of the class.

OP's code is organized differently but having main() way down there at the bottom makes me scan the entire class before I can get my bearings on where to start tracing through the code. I think this is not as friendly to the reader user than the way I like to organize my code.
 
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Good night all

There is much being said here that i don't yet really understand because i don't have much expirience on coding but... i can´t resist on asking something Praveen about the aproach on the problem itself.

There are some patterns in the shape:
1 - the total number of lines is equal to the maximum number of # on a single line;
2 - in the top half of the shape the number of leading spaces in first line is equal to half the total lines/# - 1, and then decreases by one in every next line;
3 - in the bottom half it's the opposite;
4 - in all lines the number of #s for the particular line is equal to the total lines - 2 * number of leading spaces.

Knowing this we can print the shape with one simple loop...
Don't get me wrong, your math is exact!
But i would rather do it simpler...

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Knute Snortum wrote:I like that you ...


Following Knutes lead, I like that you:

1. Put yourself and your code out there for people from all over the world to review -- that takes more guts than what a lot of people might care or dare to admit.

2. Got yourself a copy of V. Anton Spraul's book, Think Like a Programmer, and are putting what you learned from it into practice.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Carlos Reves wrote:
Knowing this we can print the shape with one simple loop...
Don't get me wrong, your math is exact!
But i would rather do it simpler...


Carlos, would you care to share how you would have coded it?

One thing that Praveen left out is a clear explanation of the constraints for this problem. I'll adapt them for Java since the book that this problem comes from (V. Anton Spraul's Think Like a Programmer) uses C++.

These are the constraints you have to honor: Using only these statements to produce output: System.out.print(' '), System.out.print('#'), and System.out.println(), write a program that produces the following shape: ... (various shapes given)


This is an exercise in pattern analysis and algorithm development so keep that in mind as you formulate your solutions.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Well sorry then. I don't have the Book and I only saw what praveen put here.
For that particular it's simple. The other ones not that simple.
I'm on my phone. But when on computer i can code my aproach to that figure.

Best regards
Carlos
 
Bartender
Posts: 5465
212
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hmm, looks tedious with all these different figures. But give it your best.

To me, more interesting would be to first draw the figure in a JPanel,  indeed, by simply drawing these asterisk lines as strings, then click with the mouse along the imaginary outline and turn the mouse points into a closed Path2D. That can even be done without having all these asterisks,  just make an approaching figure. Then it is easy to draw (fill) the figure anywhere in any position and at any size, by using that Path2D as a clip and any texture as Paint, or by using throughout sop("#"). Well, it's a plan,  like the author of the book requires.

By the way, you can download a pdf version of the book (I just did), but I had no time to check if that download is complete, or just a chapter.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

salvin wrote:I am really curious about this. Could you quote your source where bloch has asked to enforce this ? The way I always thought about stateless class was a class which had static methods and didn't use instance variables like the way you did. Maybe I am wrong here ?

Apart from the things junilu mentioned in his post,read item 21 in the joshua book(2nd edition,effective java) where he uses the "singleton pattern" for the "functional object".part of the his quote is

joshua bloch wrote:As is typical for concrete strategy classes, the StringLengthComparator
class is stateless: it has no fields, hence all instances of the class are functionally equivalent. Thus it should be a singleton to save on unnecessary object creation
costs (Item 3, Item 5):....

For a reference the page number is 103 and 104 in the 2nd edition.
you can see there that the class has an instance method and still it is singleton.
Though,i admit that my class should not be a singleton as i have not any compelling reason to enforce the same.

Junilu wrote:Ok, I dug through Bloch 2nd Ed. and Item 3 does refer to the getInstance() method in the singleton example as a static factory method. Bloch writes further, however, stating that "One advantage of the factory-method approach is that it gives you the flexibility to change your mind about whether the class should be a singleton without changing its API." This an important nuance to note, something I have to admit I never really thought much about before but it does make sense to call it a factory method if you're thinking along those lines. You learn something new everyday.

Yes i have called it a static factory in the same sense joshua mentioned and even i have use the same name(for method) and pattern he mentioned.

kind regards,
praveen.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
The point of the constraints to use only statements that output a single character or newline was to force the use of for-loops to generate the pattern. Like I mentioned before, these were meant to be exercises around pattern analysis and algorithm development. You could totally Kobayashi Maru these by writing something like:

But that's not the point of the exercise. By the way, you know it's a geeky book when the author mentions Kobayashi Maru in Chapter 1.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Here is my new code with corrections(done accordingly mentioned)

Junilu wrote:2. Your comments don't follow a consistent pattern and most of them are not really necessary. Line 12 for instance is only stating the obvious. Line 15 is also misleading in that it claims to introduce a "static factory" where the method doesn't really fit the definition of a factory method. (EDIT: Bloch does in fact call getInstance() a static factory. See my other reply below)

In fact when i am posting my code,i do not want to add comments.despite of adding comments i want my code should be appealing itself(every line).the part of my program which is the difficult to parse is the logic of printing spaces and hash symbols at each line which i have already explained at the very first in my post.and other part,i think,is obvious.i will keep it in mind next time that i should comment only those lines which are less appealing.

junilu wrote:This is more a matter of style/form than anything but it also has to do with readability. I read code from top to bottom. I expect high-level public code to be up top and detailed private code to be down towards the bottom. I also like to keep static members together towards the top of the class and instance members below them. I find that this organization generally allows me to scan a class and get a general feel for its capabilities much faster.  Thus, in my code, where there is a main() method, it is generally one of the first methods encountered when reading from top to bottom, followed by whatever other static methods there may be, followed by constructors, then high-level public methods, and then the more detail/implementation-focused private methods to round out the bottom of the class.

I have read a kind of same thing in a "Clean Code".actually i take a look at the part in a book where it is advised to organize the code just in a way the things are happening in it(so that we can translate the code in english sentence as we going down the code).Also i had read a little about the SLAP(single level of abstraction of rinciple),mentioned by you in some other thread and it is also there in clean code.does my code follows it.

Kind regards,
Praveen.
 
Knute Snortum
Sheriff
Posts: 7125
184
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
In lines 26 and 36, 9 is still a "Magic number".  Change HEIGHT and the program breaks.
 
salvin francis
Bartender
Posts: 2911
150
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

praveen kumaar wrote:Apart from the things junilu mentioned in his post,read item 21 in the joshua book(2nd edition,effective java) where he uses the "singleton pattern" for the "functional object".part of the his quote is

joshua bloch wrote:As is typical for concrete strategy classes, the StringLengthComparator
class is stateless: it has no fields, hence all instances of the class are functionally equivalent. Thus it should be a singleton to save on unnecessary object creation
costs (Item 3, Item 5):....


Thanks for pointing it out, I read through it and here is my understanding...

in the above example, X is an object. You need to pass an object to this method for it to work. What he has suggested is that since StringLengthComparator class does nothing but comparison and does not have any "state", it would be better to make it a singleton and implement an interface. That way, you can change your "strategy" to any other class (eg a custom implementation called StringCaseInsensitiveComparator) as long as it honors the same contract. Additionally, you wouldn't need to create unnecessary new objects of this class since all implementations would essentially do the same task.

I don't see any such design in your implementation, thus a singleton would not be required in this case.
However, if you extend your program such that this "strategy" prints a diamond and some other "strategy" prints a square or a traingle, maybe you could define an interface and a class to call the interface method without worrying about its implementation. That's the beauty of abstraction.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Knute Snortum wrote:In lines 26 and 36, 9 is still a "Magic number".  Change HEIGHT and the program breaks.


Praveen, Knute is right about this. You seem to have missed my note that those methods should be implemented relative to the parameter and the constant HEIGHT.
 
salvin francis
Bartender
Posts: 2911
150
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
This would be my suggestion if you indeed want to go ahead with a singleton strategy based implementation :
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
That thing with Singleton is just gold-plating; it has nothing to do with the point of the exercise. The thing with Strategy might be an interesting exercise in itself though.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Likes 2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Ok here is the new code:

The best thing to use Height and lineNum in the methods is now i can print a diamond of any size just by altering the constant variable HEIGHT or even i can ask a client to set HEIGHT by providing a single int parameter constructor in my printer class.

Kind regards,
Praveen.
 
salvin francis
Bartender
Posts: 2911
150
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
The code looks a lot better now

praveen kumaar wrote:... i can ask a client to set HEIGHT by providing a single int parameter constructor in my printer class.



or you could do this :
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I have attempted an another question this time i had to print the star like pattern with the same constraints as for the previous problem.

In my code what i have done is:

I have used the similar logic like i did in my previous problem.
Here is my code:

i have tried to implement all the advice mentioned by you people in the previous post.so plese let me know if their is any other advice.

Kind regards,
Praveen.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Good morning all.

Since Junilu asked to show how i would code the second shape here is my code.
It's not very elaborated, i know, but it does the job.



Due to the restritions on the print statements it's not possible to do it with only one loop as i said before. For that it would be necessary some string formatting.

Best regards
Carlos
 
Marshal
Posts: 79153
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I would prefer PK's approach with two methods. I prefer your for loop form. I prefer to start the loop variable at 0 and use < rather than <=.
 
Campbell Ritchie
Marshal
Posts: 79153
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
What about using a StringBuilder and repeatedly appending ' ' or '#' to it.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
@Carlos & Praveen: cows to both of you for effort and contributions to the discussion.

@Campbell: the StringBuilder idea is fine but it's a departure from the given constraints.

The differences in the two solutions offered is quite stark and it represents somewhat of a fork in the road for each person as to the type of programmer he wants to be.

The version that is shorter is also a little more cryptic and makes you sit there and analyze it carefully for correctness. Sure, you can just run the program to see that it produces the correct results but you are still left in the dark as to how and why it works until you trace through it line by line and analyze each part.

The version that is longer and more verbose may seem to go through more ceremony and gyrations than is really necessary, but it helps the reader clear up any doubts as to the correctness of the code. If there's anywhere a reader might get stuck for a while in analysis, it's going to be in those small methods that calculate how many times a space or hash should be repeated on a line.

For requirements that are as relatively simple as these, you might be inclined to favor the shorter code. In fact, I have to admit that the verbosity of the longer code does kind of make me think about making a compromise and perhaps choosing to go with a slightly refactored version where you just extract the formula for the loop termination values out to spacesFor(int) and hashesFor(int) methods.

However, when you have a larger and significantly more complex set of requirements and constraints, the incremental, divide-and-conquer approach that produced the long form solution here often results in more clear, organized, testable, and maintainable code.

The lesson? There are many things that factor in to the quality of code. Context matters. Clarity and expressiveness is a good thing and if you can write code that has it, great. "Short and sweet" also has its merits but you should be careful to balanced it against other quality attributes as well.

Good job to all for great contributions!
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Campbell the start of the loop with the negative value is there to perform the symmetry in the shape...
But of course we could do like this:



The result would be the same.

Forgot to mention that wiith this we would be able to print any diamod with a shape similar to this one provided that the total number of lines is even.
When i said just one loop was necessary was thinking on a stringbluider or format options with printf. But Junilu said that it was only possible to use print(' '), print('#') and println()...

Best regards
Carlos

 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thanks Junilu for the cow! Depply appreciate it! (it's the first one )

Lesson to be taken from me: Still have a lot to learn!

Best regards
Carlos
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
not "from me" but "for me" of course.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I realise that the code that i'm going to put here is, again as Junilu said, as cryptic as the one before, but i was coding the star and got this:



It also prints correctly stars with even total number of lines and it also uses the symmetry in it.

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
One more note before I get ready to go out for an appointment...

For me, this exercise represents a microcosm of the development process. You have a set of requirements, you lay out a plan, you start incrementing, refactoring, testing, then finally arrive at an acceptable solution.

Others may be focused only on that last part, in arriving at an acceptable solution.

For requirements as small and relatively simple as these, many people will probably see the longer solution as way too much work and ceremony, and justifiably so. However, keep in mind that getting in the habit of choosing the expedient over careful diligence can set you up for a lot of pain and suffering in the future. To me, it's best to learn how to work at both ends of the spectrum and then figure out how to get a feel for how to make a good compromise between the two extremes in situations where it's critical to balance both time and quality. The most useful skills to have that allow you to walk that fine line are refactoring and recognizing code smells so don't pass up any opportunity to learn how to do those better.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:I would prefer PK's approach with two methods. I prefer your for loop form.


Yeah, this comes from a more finely tuned sense of balance and proportion of compromise that I just mentioned.  
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Can't resist making an obvious plug for my favorite development technique...

I wrote:To me, it's best to learn how to work at both ends of the spectrum and then figure out how to get a feel for how to make a good compromise between the two extremes in situations where it's critical to balance both time and quality. The most useful skills to have that allow you to walk that fine line are refactoring and recognizing code smells so don't pass up any opportunity to learn how to do those better.


Test-Driven Development (TDD) is exactly this: working in both extremes within the span of a few minutes. You start with a small failing test, then step into expedient mode and write a quick and dirty version just to get your failing test to pass. When you get a green bar, you step out to the other end and carefully and diligently look for code smells and opportunities to refactor. You keep doing this over and over until you get to a robust solution. This is a really good way of learning how to work at both ends and find that sweet spot in the middle.

I guess my bottom line here is this: Perfect Practice is just as much about the way you get there as it is actually getting there.
 
Carlos Reves
Ranch Hand
Posts: 116
10
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
@Junilu:
Thanks for your insights. I'll keep them in mind.
But just say that, despite the simple form of my code, i just saw that there's a linear correlation between the line number and the number of hashes and spaces and a symmetry that can be used.
So i agree that the code could be more elaborated to make it more clear about its correctness, but still there was a plan behind it.

By the way, in my Star code the last for loop isn't needed at all...

Best regards
Carlos
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
@Carlos: I don't doubt you had some kind of plan and systematic way of attacking the problem. It's difficult to come up with a solution like yours just by dumb luck.

Read about shu ha ri - it gives you an idea why it helps to learn in stages and why good solutions seem to elude some people while they seem to come naturally and instinctively to others. You need to figure out which stage you are in in your development and practice accordingly.
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
reply
    Bookmark Topic Watch Topic
  • New Topic