• 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
  • Liutauras Vilda
  • Bear Bibeault
Sheriffs:
  • Jeanne Boyarsky
  • Tim Cooke
  • Devaka Cooray
Saloon Keepers:
  • Tim Moores
  • Tim Holloway
  • Piet Souris
  • salvin francis
  • Stephan van Hulst
Bartenders:
  • Frits Walraven
  • Carey Brown
  • Jj Roberts

Is this code the correct way to solve this problem?

 
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Problem:
There is a file that contains a list of actors and the movies in which they acted. We wants to know the top 3 ranked actors from her list whom have acted/appeared in the most movies.

Write code in Java to display the top 3 ranked actors appearing in the most movies based on the count of movies in which they have acted. If there are less than 3 actors in her list, display all of them.


Input Explanation

The first line of input is always an integer denoting how many lines to read after the first line.
In each data line, the actor name and movie name are separated by a ','(comma).

Input

8
Leonardo DiCaprio,The Revenant
Christian Bale,Vice
Morgan Freeman,Shawshank Redemption
Leonardo DiCaprio,The Great Gatsby
Christian Bale,American Psycho
Morgan Freeman,The Dark Knight
Christian Bale,The Dark Knight
Samuel L. Jackson,Pulp Fiction



Output Explanation

Print the top actor names to standard output . You should not have counts in the output, only actor names

Output

Christian Bale
Leonardo DiCaprio
Morgan Freeman



For this problem, I wrote the below code which is working fine :




While the above code is working fine, is it the correct way to code this ? thanks










 
Sheriff
Posts: 15996
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:While the above code is working fine, is it the correct way to code this ?


No, this program is very poorly written: it's not readable and it's poorly organized.
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks for the feedback. It will help me improve. Also, based on my self review  1) no comments 2) instead of all code in main method ,there should be small methods with single responsibility and those should be called. 3) variables should have better self explanatory names.
 
Bartender
Posts: 7437
66
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If your code is self explanatory you shouldn't need comments. Only use comments when this is not possible.
 
Carey Brown
Bartender
Posts: 7437
66
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Use Scanner, not BufferedReader. Your iterator loop should be an enhanced for-loop. You should be displaying prompts before asking for input.
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carey Brown wrote:If your code is self explanatory you shouldn't need comments. Only use comments when this is not possible.



Thanks. But what may be self explanatory to one developer may not be self explanatory to the other. developer So is not right to have comments which would cover both these cases?
 
Marshal
Posts: 71098
292
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carey Brown wrote:Use Scanner, not BufferedReader. . . . .

Agree. But use one Scanner object and don't close it. You are cluttering your heap memory with readers which have only been used once (line 20). You only need one Reader in that application.
Please explain what lines 24‑29 do. Why haven't you got {} around line 26? Why are you using a null test rather than containsKey()?
How did you work out the regex to use in line 23?
Why have you got so much code in the main() method?
Why didn't you design a class to encapsulate actor's name and movie count?
 
Campbell Ritchie
Marshal
Posts: 71098
292
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:. . . But what may be self explanatory to one developer may not be self explanatory to the other. developer . . .

I would have thought the following would be self‑explanatory:-The following does no more than repeat what the code says:-The following is really confusingIf you really want to add to the confusion, you can write this:-There is only one way to make that worse: I shall leave guessing the circumstances where the following can occur as an “exercise for the reader”“exercise for MS”.You might rescue that code like this:-...but the code would be clearer if you moved that line to before the first input.
 
lowercase baba
Posts: 12935
65
Chrome Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm surprised Campbell didn't mention this  [edit - he did, I just didn't see it].  You have a monolithic "main" method that does everything.  That is generally a poor design. It is certainly not OOP.

You should think about writing code using many small methods, each of which does one thing, then assemble them into a larger program.  at minimum, i would probably have the following method:

readFile();
findTopThree();
printTopActors();

Note that each of these methods may be comprised of other, smaller methods.  You want each method to do something simple enough that the name tells you exactly what it does - and it does no more than that.

Also, as you think through this, consider how you may want to make the program more general.  As written above, it's clearly going to find ONLY the top three. But what if later my spec changes, and I want to find the top five? or 30?  Maybe the findTopThree method should take an integer parameter, and then it finds that many. Obviously we'd have to rename it to something like findTopRanked(int howMany);

just food for thought.

[edit 2 - corrected spelling/wrong word]
 
Saloon Keeper
Posts: 2741
133
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would be interested in the source of the question as well. Is this homework assignment or is this some code challenge given by a company to solve offline as a part of an interview process ?
In either case, I would consider it unethical to use the ideas of all those helping you out there and post it as your own.

Even if you do use BufferedReader, I don't see the point of creating so many !
I see a mixture of old java code mixed with new version code. Looks like the programmer is trying to stuff in concepts as they come in his/her mind !
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sure. It was an interview question but I had already solved and submitted it . Later on I thought that although I got the code working successfully, let me also see whether I could have done it in a better way. The latter part is just for my learning.
 
salvin francis
Saloon Keeper
Posts: 2741
133
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:Sure. It was an interview question but I had already solved and submitted it .


Please do include these details in the post next time so that it would help us help you better.

Monica Shiralkar wrote:Later on I thought that although I got the code working successfully


Quite often, the interviewer is more interested in how you solve a problem rather than merely checking if the code works

 
salvin francis
Saloon Keeper
Posts: 2741
133
Google Web Toolkit Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I think the movie names itself can be discarded. It looks like we're only interested in the count. It would imply that every line contains a unique actor-movie name combination (A question I would ask the interviewer or clarify if possible). Based on what I understand, we're merely counting how many lines contain a given actor's name.
 
Saloon Keeper
Posts: 4168
160
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If you are on java >= 10. you can save quite some typing.
For instance: instead of

you can use

or

and using the method 'map.computeIfAbsent' saves quite some typing too:

And lastly: you can derive the top-N actors from the map<actor, List<movie>> in a more direct way. Here is a generic version:

(you need some static imports here, but your IDE is good in that).












 
salvin francis
Saloon Keeper
Posts: 2741
133
Google Web Toolkit Eclipse IDE Java
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Piet Souris wrote:
...


Wow, this is quite compact code ! I like that you have used the reversed method to self document the order of sorting. As a side note, I think the min is not required for the limit. Meaning, if the list has 2 actors and we need to limit the list to 3, we would see all 2 actors only.
 
Campbell Ritchie
Marshal
Posts: 71098
292
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nice stuff Can I challenge MS to explain how some of that recent code works.
 
Piet Souris
Saloon Keeper
Posts: 4168
160
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

salvin francis wrote: (...) I think the min is not required for the limit. Meaning, if the list has 2 actors and we need to limit the list to 3, we would see all 2 actors only.


Just tested it, you are absolutely right!
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:but the code would be clearer if you moved that line to before the first input.



Sorry. Which line before which line?
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:

Monica Shiralkar wrote:. . . But what may be self explanatory to one developer may not be self explanatory to the other. developer . . .

I would have thought the following would be self‑explanatory:-The following does no more than repeat what the code says:-The following is really confusingIf you really want to add to the confusion, you can write this:-There is only one way to make that worse: I shall leave guessing the circumstances where the following can occur as an “exercise for the reader”“exercise for MS”.You might rescue that code like this:-...but the code would be clearer if you moved that line to before the first input.



Thanks.Does that mean for lines such as listed above, it is better to have no comment at all?
 
Carey Brown
Bartender
Posts: 7437
66
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One thing to watch out for is "magic numers". In the examples the '3' is a magic number because you don't know what it stands for. In this case a comment would be helpful. Alternatively you could define a constant with a self explanatory name.
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carey Brown wrote:Your iterator loop should be an enhanced for-loop.




Is it because of this reason that if if all we are doing in an iterator is print something then we should not be needing iterator at all?
 
Marshal
Posts: 26135
77
Eclipse IDE Firefox Browser MySQL Database
  • Likes 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:Is it because of this reason that if if all we are doing in an iterator is print something then we should not be needing iterator at all?



It goes beyond that. Iterator is what we had to use 15 years ago before we had the enhanced for loop. Using Iterator for something which doesn't need specific Iterator features just says to readers of the code that maybe you haven't learned much in the last 15 years. Or that you wrote the code based on an ancient tutorial which you found on the web.

I was looking at some of my old code today (ugh, there was an NPE in it) and I found I was using an Iterator to go through a List and process its entries. Okay, it was old code. So did I need an Iterator now? Well, yeah, after it processes each entry it calls the Iterator's remove() method to delete the entry from the underlying List. But why does it do that? The List is created just for this loop, and it's going straight to GC after the loop ends, so what's the point? There's no point.

So now the code is List.stream().forEach(this::methodToProcessAnEntry). No more Iterator.
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote:

Monica Shiralkar wrote:Is it because of this reason that if if all we are doing in an iterator is print something then we should not be needing iterator at all?



It goes beyond that. Iterator is what we had to use 15 years ago before we had the enhanced for loop. Using Iterator for something which doesn't need specific Iterator features just says to readers of the code that maybe you haven't learned much in the last 15 years.


thanks
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote: No more Iterator.



You mean "no more iterator" only in this case?
 
Paul Clapham
Marshal
Posts: 26135
77
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:You mean "no more iterator" only in this case?



I was talking about one example, yes. There are still places where Iterators are useful, but nowadays they are specialized tools.
 
Saloon Keeper
Posts: 12499
269
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:Thanks.Does that mean for lines such as listed above, it is better to have no comment at all?


I think I already pointed this out to you once in another topic. Don't use comments in code to explain what you are doing, that's what the code itself is for. Use comments to describe why you are doing something, but only if you wrote a non-obvious solution because the obvious solution caused a bug.

Instead of comments, write class and method documentation. If your methods are no longer than about 5-10 lines and have clear method names, your code will become self documenting.
 
Campbell Ritchie
Marshal
Posts: 71098
292
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote:. . . There are still places where Iterators are useful . . .

There are lots of places where Iterators are used, but obscured in the background, for example the for‑each loop.
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Stephan van Hulst wrote:Use comments to describe why you are doing something, but only if you wrote a non-obvious solution because the obvious solution caused a bug.



Thanks. I think this line tells it all.

And more often, the code would be using the obvious solution and it would not be required
 
Paul Clapham
Marshal
Posts: 26135
77
Eclipse IDE Firefox Browser MySQL Database
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:And more often, the code would be using the obvious solution and it would not be required



That's right. If the code says "count++" there is no need for a comment which says "Increment the count", for example.
 
Campbell Ritchie
Marshal
Posts: 71098
292
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Monica Shiralkar wrote:. . . . Does that mean for lines such as listed above, it is better to have no comment at all?

I told you which of the comments I thought were good and which bad.
Did you work out how you might get the confusion with 3 and 4 in the same line?
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote:

Monica Shiralkar wrote:And more often, the code would be using the obvious solution and it would not be required



That's right. If the code says "count++" there is no need for a comment which says "Increment the count", for example.



Thanks. And for many developers there is not 1 but 2 problems here. First, that they give comments for such things and Second that their variable names are not simple and understandable . So they have to sort out both these problems at once.
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote: If the code says "count++" there is no need for a comment which says "Increment the count", for example.



Thats a good example.
 
Monica Shiralkar
Ranch Foreman
Posts: 2076
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:

Monica Shiralkar wrote:. . . But what may be self explanatory to one developer may not be self explanatory to the other. developer . . .

I would have thought the following would be self‑explanatory:-The following does no more than repeat what the code says:-The following is really confusingIf you really want to add to the confusion, you can write this:-There is only one way to make that worse: I shall leave guessing the circumstances where the following can occur as an “exercise for the reader”“exercise for MS”.You might rescue that code like this:-...but the code would be clearer if you moved that line to before the first input.



Was confusing for me.

I understood the first line which said " the following would be self‑explanatory:" and agree that . But the next lines were confusing for me.
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
reply
    Bookmark Topic Watch Topic
  • New Topic