• Post Reply Bookmark Topic Watch Topic
  • New Topic

please format the following code  RSS feed

 
Ranch Hand
Posts: 62
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
This is a method that takes an int[] and returns 1 if at least 3 of the contained numbers in the array can form a triangle (the sum of any 2 is bigger than the third one).

My boss is a readability formatting freak. He does not allow me methods greater than 10 - 15 lines. Please format this method for readability:

 
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Start by telling us whether the code is correct or not.

I shall take the liberty of moving this discussion because you will get more attention elsewhere.
 
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Lucian Whiteman wrote:if at least 3 of the contained numbers in the array can form a triangle (the sum of any 2 is bigger than the third one)
Are you sure about that? Does that reminds you something? a^2+b^2=c^2
Why are you using long, can't it fit to "int"? Are you sure you'll be enough "long" then?
 
Campbell Ritchie
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That means a triangle not a right‑angled triangle. The largest of the three numbers must be less than the sum of the other two. Because right angles are not used, there is no need to use Pythagoras.
 
Liutauras Vilda
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ok, I might made a wrong assumption about it. But is it possible to make a triangle from (1, 1, 9) ?

[edit]Ah, I see, interpreted wrongly.
 
Marshal
Posts: 4052
239
Clojure IntelliJ IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My first readability gripe would be the return type.

A method called 'isTrianglePossible' is a question, therefore I expect the return type to be a boolean.
 
Liutauras Vilda
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

That "if" check is redundant, you could check the same during the 1st "for" loop iteration.
Be aware, current "for" loop would cause "ArrayIndexOutOfBoundsException". Think how would you fix it.

If the intend of "for" loop were to start checking from the end of array, then "i" is wrongly initialised.
 
Lucian Whiteman
Ranch Hand
Posts: 62
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The initial method is indeed wrong, as it has been mentioned:
- NullPointerException on line 15
- name is confusing as user expects method to return a boolean
- if inside for is not needed

 
Campbell Ritchie
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Tim Cooke wrote:My first readability gripe would be the return type.

A method called 'isTrianglePossible' is a question, therefore I expect the return type to be a boolean.
Is that readability? I would think it is a more severe error than that. You might return an int in C or in most of the languages that preceded Java® but using an int when a boolean is available counts as incorrect code (or not writing Java® code) in my eyes.
 
Campbell Ritchie
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would suggest that your method should insist on receiving a non‑null 3‑element array. I would think it is incorrect code to return a result at all, unless that requirement is fulfilled. That requires two things (at least), one of which is the documentation comment.
 
Liutauras Vilda
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Lucian Whiteman wrote:if inside for is not needed
Not that one, but the one which is above "for" loop.

Because you're starting check with "for" loop from the beginning of the array, so, during the first iteration you'd check exactly the same elements as you would check in "if" statement.
 
Lucian Whiteman
Ranch Hand
Posts: 62
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Liutauras Vilda wrote:
Lucian Whiteman wrote:if inside for is not needed
Not that one, but the one which is above "for" loop.

Because you're starting check with "for" loop from the beginning of the array, so, during the first iteration you'd check exactly the same elements as you would check in "if" statement.


This is a very good observation. Here is the code after including your observation into it:

 
Campbell Ritchie
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Lucian Whiteman wrote: . . .
If you had a competition to make the readability worse, that would be up amongst the leaders for the prize.
 
Liutauras Vilda
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Lucian,

Can you explain what is this? What are you trying to accomplish with it? Stop guessing how the code should look like - you should be writing down the steps on piece of paper and trying to solve the problem on it first.

Campbell Ritchie might a bit sarcastic, but very right
 
Lucian Whiteman
Ranch Hand
Posts: 62
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Liutauras Vilda wrote:Lucian,

Can you explain what is this? What are you trying to accomplish with it? Stop guessing how the code should look like - you should be writing down the steps on piece of paper and trying to solve the problem on it first.

Campbell Ritchie might a bit sarcastic, but very right


Campbell Ritchie is very wrong, as anyone who criticizes without giving a solution is.

But your observation is good, I shall add it to the code:

 
Master Rancher
Posts: 2045
75
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hmm, with the topic of Paul Wheaton (from a couple of weeks ago) about 'be nice or be removed'
in mind the replies could indeed be a bit more 'nice'.

But I would like to give OP another point of view.

Say, we are gven three sides a, b and c. Take two of them,, doesn't matter which two.
Imagine these two sides starting at the origin, both lying on the positive x axis.
You see that to make a triangle, the third side needs to be of length |a - b|, the absolute
value of the difference in lengths of a and b. Of course, we are talking about a degenerated
triangle here, but that's not important.

Now, imagine that we rotate one of the two sides, while maintaining the third side connecting
the two endpoints. Agree that that third side attains maximum length when that rotated
side is lying on the negative x axis? What length would that third side have?

So, what is the minimum and maximum value for the length of the third side?

Greetz,
Piet
 
Liutauras Vilda
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well, Lucian, you took it too serious, I'm pretty sure he didn't want anything bad. Just trying to help, as everyone else in this forum.
Solutions are not given over here, just observations, hints, so the people who comes could come up with their own solutions, not necessarily correct.
And this is the best part of being criticized (as it is done in friendly way over here always), so you could defend your opinion, explain why you did in this way, and you might would convince your "opponent". At least you'd get better understanding about it.
 
Liutauras Vilda
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Lets get back to your problem.
Is your code produces the output you are expecting?
 
Campbell Ritchie
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sorry for being sarcastic. But that line of code I quoted was absolutely dreadful.

You say your boss wants you to improve the readability of your code. Are you on some sort of probation period at your job? It is your job to create the code, not our job to provide you with code. You have to find the solution, not I. In fact if we did provide you with code, your boss will find it and know that is was not your code, and your probation period will end suddenly. And unhappily.
You called your boss
a readability formatting freak.
You said that
He does not allow me methods greater than 10 - 15 lines.
I read that as meaning
My boss knows how to program.
He is obviously trying to find out whether you can program.

You have been given all sorts of hints about how to make your program better. You appear not to have taken notice of them.
 
Campbell Ritchie
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I can see another problem in that code, I am afraid, which makes it not correct in my opinion. I can see a side‑effect which might cause incorrect behaviour of other code.
 
Lucian Whiteman
Ranch Hand
Posts: 62
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:I can see another problem in that code, I am afraid, which makes it not correct in my opinion. I can see a side‑effect which might cause incorrect behaviour of other code.


The array gets sorted inside the function, and this is a side effect that may cause bugs in other parts. Got that, thank you.

I shall add this observation to the code:



Now the method name is containing an "and", thus no more single responsibility principle and no more job for me. What to do ? How can I rewrite it better ?
 
Liutauras Vilda
Sheriff
Posts: 4935
334
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Lucian, please have a look at your for loop. The "for" loop body contains nothing, even there's no an empty statement ";" which presence is considered as a good practice if your intend is to do nothing within the body.
Also, keep in mind, that "i" is initialized within "for" loop, so the scope of visibility ends right before closing brace of "for" loop body.
 
Campbell Ritchie
Marshal
Posts: 56608
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Lucian Whiteman wrote: . . .
The array gets sorted inside the function, and this is a side effect that may cause bugs in other parts. Got that, thank you.
. . .
Now the method name is containing an "and", thus no more single responsibility principle and no more job for me. What to do ? How can I rewrite it better ?
At least you can see the problem . Seeing the problem is half the battle.

At least two things you can do
  • 1: Don't sort the array. Use it unsorted.
  • 2: Don't sort the array. Sort something else.
  • By the way. I would look on that method as incomplete if you don't have a documentation comment.
     
    Campbell Ritchie
    Marshal
    Posts: 56608
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Why are you changing the name of the method? What is it supposed to return? If you were told to return an int, what does that int represent?
     
    Lucian Whiteman
    Ranch Hand
    Posts: 62
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Campbell Ritchie wrote:
    Lucian Whiteman wrote: . . .
    The array gets sorted inside the function, and this is a side effect that may cause bugs in other parts. Got that, thank you.
    . . .
    Now the method name is containing an "and", thus no more single responsibility principle and no more job for me. What to do ? How can I rewrite it better ?
    At least you can see the problem . Seeing the problem is half the battle.

    At least two things you can do
  • 1: Don't sort the array. Use it unsorted.
  • 2: Don't sort the array. Sort something else.
  • By the way. I would look on that method as incomplete if you don't have a documentation comment.


    Holly cow, this is true !

     
    Campbell Ritchie
    Marshal
    Posts: 56608
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    You seem to have changed the instructions from your first post. As I said earlier, what do the numbers returned mean?
     
    Lucian Whiteman
    Ranch Hand
    Posts: 62
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Now that this method no longer sorts the array, the return i-2 has no meaning. Thus it needs to be changed:





     
    Liutauras Vilda
    Sheriff
    Posts: 4935
    334
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Lucian,

    As Campbell Ritchie rightly pointed out, you keep changing the instructions/requirements.
    It is quite difficult to do the things without knowing what has to be in a final version, and these changes are not minimal or something extra - it changes all things in general.
    Very 1st program you posted were intended to do completely different thing. Please clarify.
     
    Campbell Ritchie
    Marshal
    Posts: 56608
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    At least the methods are getting shorter.

    As for legibility, I would give that 4 out of 10.
    Have you any evidence that your method is correct?
     
    Lucian Whiteman
    Ranch Hand
    Posts: 62
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Liutauras Vilda wrote:Lucian,

    As Campbell Ritchie rightly pointed out, you keep changing the instructions/requirements.
    It is quite difficult to do the things without knowing what has to be in a final version, and these changes are not minimal or something extra - it changes all things in general.
    Very 1st program you posted were intended to do completely different thing. Please clarify.


    I do not have written specification, or any of such guidance. So I write the code, and pray it works with minimal bugs.

    Anyway, Designing the code, writing some sort of interface/specification could be useful. Here is the code in its actual form:



    Do you think that if I initially started with some interface where I had defined the method signature, would that have helped ?


    Please feel free to give advice, the findTriangle() method is 15 lines long, how can I shorten it ?

     
    Campbell Ritchie
    Marshal
    Posts: 56608
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Stop changing the requirements.
    You were told to do something last week and we can't see that you have done that at all. One of the requirements for a job is that you carry out the task you have been assigned.
     
    Bartender
    Posts: 2087
    44
    Firefox Browser IntelliJ IDE Java Linux Spring
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    One note.
    While it is technically possible to have a public method return an instance of private nested class, it is not a good idea.
    Callers of such method will not be able to assign it to anything other that Object and will not be able to access fields (!) of Triangle.
    By the way, fields should be private and values should be accessible via getters methods.
     
    Liutauras Vilda
    Sheriff
    Posts: 4935
    334
    BSD
    • Likes 3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Lucian Whiteman wrote:I do not have written specification, or any of such guidance. So I write the code, and pray it works with minimal bugs.

    Ok, at this point you need to step away from computer and take a short brake. Lets start over.

    Please explain, what have you been asked to do? Write the method? Write the class?
    Please explain in one or two sentences without using any technical terms, what has to be done?
    If there is a restriction for a method length, please state clearly: "method/-s cannot be longer than 15 lines", "code has to satisfy Java coding convention, by having empty lines between methods.. and so forth".

    For instance:
    1. Write the method, which takes the collection of numbers and determines whether possible to construct the triangle from given sides or not.
    2. The method should return "yes" if it is possible, and "no" if it's not.
    or
    3. Method should return the index/-es of the side/-s in a given collection, which could create a triangle.

    Without knowing all this, you even shouldn't start writing any lines of code. Even then, when you know, you need to write down the "plan" in English or other preferred language, what are you going to do, and it should be in a form of pseudo-code.
    Preferred way is to write down the steps (still omitting the technical terminology) on a piece of paper, so you'd likely have a better chances to catch and think about the corner cases.

    And only then, when everything is clear and you are convinced yourself what has to be done - you can start by thinking how you're going to do it in Java.

    At the moment you're shooting to a wrong directions. And it don't seem anyone can help you at the moment, because it is not clear, what is the problem itself. And naturally you're getting frustrated, because you're not getting what you expecting.
    And it will not change, until you change the way you think about programming.
     
    Ranch Hand
    Posts: 1143
    9
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Lucian Whiteman wrote:


    Should that be greater than or equal to? I've just been looking through quite a handful of supposedly authoritative sources and some say that three collinear points define a triangle and some say they don't. Use whichever definition fits your application. In the absence of application specific requirements, I tend to accept polygons with zero area, such as triangles with collinear points.
     
    Campbell Ritchie
    Marshal
    Posts: 56608
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I think I would stick to > rather than accepting collinear points.
     
    It is sorta covered in the JavaRanch Style Guide.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!