• Post Reply Bookmark Topic Watch Topic
  • New Topic

Advice on Code  RSS feed

 
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hey all!

So I was asked to produce some code for a job application which took a list of matches, create a league table from them and then return a sorted list by points, goal difference, goal scored and then team name. I wrote the code in an hour or so, had it code reviewed by a senior developer where I work (didn't have much wrong to say about it). I submitted the code and was basically told it was awful.

In the spirit of always improving oneself, I have asked for feedback but I haven't heard anything thus far. I'm a little perplexed about what's wrong with it so could I ask someone to take a look at it and provide constructive criticism? I'm thick skinned so I can take it!



Any feedback is welcome!

Cheers!
 
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One thing that jumps out right away as strange is this code and the other methods similar to it.

What do you think you're achieving by doing this?
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So by this I was preventing any modification to the internal variables by defensively copying those variables to any calling client. I read a lot of "Effective Java" by Joshua Bloch and this is the following from his book:

You must program defensively, with the assumption that clients of your class will do their best to destroy its invariants.


I like to make classes, which are meant to be final, immutable after instantiation.

What would your take on that be?
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Were there any other requirements you're not telling us about? Another thing I see that might be questioned is that the LeagueTable.getTableEntries() method will always sort the current LeagueTable object's list of entries. This may or may not have been a desired behavior but for me, it is certainly a surprising one either way.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Oli Jones wrote:So by this I was preventing any modification to the internal variables by defensively copying those variables to any calling client. I read a lot of "Effective Java" by Joshua Bloch and this is the following from his book:

You must program defensively, with the assumption that clients of your class will do their best to destroy its invariants.


I like to make classes, which are meant to be final, immutable after instantiation.

What would your take on that be?


Was there a requirement to do that? Also, primitives are immutable. String is also immutable. So what you're doing with those getters is misguided at best.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Your Comparator implementation is also not what I would do. I would implement Comparator so that it did normal, ascending order, then use Collections.reverseOrder(Comparator) to get the one for sorting in descending order.
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Of course, so that code is redundant. But other than that, is there anything else you can spot? Thanks for the feedback so far!
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Okay, so not something you'd personally do but is there anything wrong with that implementation?
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I do think the comparator class does sort it in ascending order, however.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Oli Jones wrote:Okay, so not something you'd personally do but is there anything wrong with that implementation?

It puts into question your knowledge of the Java Collections API and your decision making as a programmer/designer.
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Okay, so in the context of a LeagueTable, what would you mean by it in ascending order? I'm a little confused here. Ascending order would be from smallest to largest, but descending order would be from largest to smallest? The top 5 in the PL... is the following in ascending or descending order?

# Team Pl GD Pts
1
Leicester
33 26 72
2
Tottenham
33 35 65
3
Arsenal
32 22 59
4
Man City
32 25 57
5
Man Utd
32 9 53
 
Marshal
Posts: 56600
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Welcome to the Ranch

I would avoid those multiple return statements in the Comparator.
I don't like what you are doing with the Streams in lines 13 and 17. I wouldn't use forEach; you can partition the home and away teams into their own Lists. Only I can't think how to do it just at the moment.
Shouldn't the match outcome be determined by the Match object.
I don't like the if (x) return y; else return z; style, which makes the code hard to read. Use ?: instead.
As well as defensive copies of ints, why are you using new Integer? And how did you write line 231?
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Your Match.getMatchOutcome() method doesn't make sense to me either. It only checks if the team provided is the homeTeam and assumes it's the awayTeam otherwise. Is that assumption safe to make? What if the provided team is neither the homeTeam nor the awayTeam? Your code will say that it was a DRAW instead of throwing an exception or something.

And this:

is better written as this:

And I would even question the need to have that method since there is no difference in clarity between the method call and the actual body of the method. That is,

My implementation would be something like this:

If there were specific requirements around the team specified being neither the home or away team, I would adjust that guard clause accordingly.
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:Welcome to the Ranch

I would avoid those multiple return statements in the Comparator.
I don't like what you are doing with the Streams in lines 13 and 17. I wouldn't use forEach; you can partition the home and away teams into their own Lists. Only I can't think how to do it just at the moment.
Shouldn't the match outcome be determined by the Match object.
I don't like the if (x) return y; else return z; style, which makes the code hard to read. Use ?: instead.
As well as defensive copies of ints, why are you using new Integer? And how did you write line 231?


Thanks for the welcome

Comparator - I'm not sure how else I could create it when you need to compare multiple variables of a class? How would you write it?
Match outcome is determined by the match outcome (see lines 211 - 228) but updating the LeagueEntry with the match outcome I'd think would be the job of the LeagueEntry?
Is there any reason you wouldn't use forEach?
Yep, defensive copies I went a little overboard with and they are pointless; lesson learnt
Line 231? You mean the ternary operator?
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Your Match.getMatchOutcome() method doesn't make sense to me either. It only checks if the team provided is the homeTeam and assumes it's the awayTeam otherwise. Is that assumption safe to make? What if the provided team is neither the homeTeam nor the awayTeam? Your code will say that it was a DRAW instead of throwing an exception or something.

And this:

is better written as this:

And I would even question the need to have that method since there is no difference in clarity between the method call and the actual body of the method. That is,

My implementation would be something like this:

If there were specific requirements around the team specified being neither the home or away team, I would adjust that guard clause accordingly.


This is all great, thank you very much.

For your statement about the getMatchOutcome, yep, you're right. I should have probably spent a bit longer on this! I appreciate your honesty!

As to the assumption, within the LeagueTable class I do make sure that the match which is being acted on is by the team we are dealing with. There is a filter applied within the stream But yep, I will make sure in future that I provide additional checks like you've suggested.

 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Oli Jones wrote:Okay, so in the context of a LeagueTable, what would you mean by it in ascending order? I'm a little confused here. Ascending order would be from smallest to largest, but descending order would be from largest to smallest? The top 5 in the PL... is the following in ascending or descending order?

I guess my original suggestion wouldn't have worked. Here's an article that shows how to create a chained comparator: http://www.codejava.net/java-core/collections/sorting-a-list-by-multiple-attributes-example

Following that pattern, I would implement a Comparator for each field involved, then use the reverse() method where the field needs to be sorted in descending order before adding it to the chain.

I have to admit though, I would need to look up a lot of this stuff while doing it. I don't know if you were allowed to do that but if I were conducting the test, I would allow it. I don't think it's fair to expect anybody to be walking around with this stuff around in their heads ready to be pulled at a moments notice. For me, a coding test is as much about seeing how resourceful as candidate is in finding the information he needs as it is seeing if he/she can come up with a good solution.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm curious: were those three classes, LeagueTable, LeagueTableEntry, and Match given to you to fill out or were those what you came up with from a more general set of requirement statements?
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Could also be written as:
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I was given the three classes as a template and I had to fill them out! I would have designed it in a different way if I had the option of doing so. Thank you for the resource on chained comparators, I'll read up on that today!
 
Oli Jones
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The method to get a list of sorted table entries was one of the requirements and the method signature was given to me and I had to fill it out in response to an earlier post I must have missed.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Oli Jones wrote:The method to get a list of sorted table entries was one of the requirements and the method signature was given to me and I had to fill it out in response to an earlier post I must have missed.

Fair enough. This is how I would have done it though:

Notice that there should be code in your constructor or field declaration that ensures that the entries member is not null. And I'm using the name entries instead of leagueTableList. It reads better to me:

"A LeagueTable has a List of LeagueTableEntry objects called entries" vs "A LeagueTable has a List of LeagueTableEntry objects called leagueTableList”.

Alternatively, I would read my version out loud (it helps to do that so you actually hear any funny things you write) as: ”A LeagueTable has a List of entries (whose elements are LeagueTableEntry objects).” I find that makes more sense than ”A LeagueTable has a List of leagueTableList (whose elements are LeagueTableEntry objects).”
 
Campbell Ritchie
Marshal
Posts: 56600
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:. . . What if the provided team is neither the homeTeam nor the awayTeam? Your code will say that it was a DRAW instead of throwing an exception or something. . . .
Good point. The only way a team can be neither home nor away is if the team never played in that match at all.

It is possible to play matches at distant venues where neither team is the home team, e.g. the Cup Final.
 
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Oli Jones wrote:I was given the three classes as a template and I had to fill them out! I would have designed it in a different way if I had the option of doing so.

Me too, so I sympathise.

The names your were given (assuming you were) suggest to me that whoever thought this test up wasn't much of a designer themselves, because the names LeagueTable and LeagueTableEntry suggest implementations, not actual entities. Match is the only class name I would have used myself.

A few other things I notice in general:
1. You're missing a Team class, and arguably also a League - a LeagueTable, after all, only applies to one season of a League.

2. Most of your classes seem unduly verbose (far too many methods); but it's difficult to know how much of that was forced on you.
Specifically, you don't need methods to update each individual variable in LeagueTableEntry. I would just have defined a single method:
  public void add(Match m) { ...

3. LeagueEntry.gamesPlayed is redundant, since you already have won, drawn and lost.

4. You could have used equals() and hashCode() to make your structures more robust. For example, as specified, a LeagueTableEntry cannot exist for more than one team, therefore I would have made entries for the same Team "equal", and LeagueTable.leagueTableList a Set.

5. I like the MatchOutcome enum, but I think I would have called it Outcome, since it's already a member of Match.

6. getMatchOutcome() seems unduly complicated. My effort:
HIH

Winston
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:
6. getMatchOutcome() seems unduly complicated. My effort:

Line 4 is very clever. I'm a little ambivalent about it though because my eyes get stuck on it for a few seconds while my brain catches up and makes sure everything is covered. The Boolean == Boolean comparison is just not something I'd normally do. It does work though, and very succinctly, which is why I'm ambivalent. If it's covered by some unit tests, I'd probably wave it through

OP should note that here, as in the other implementation offered, the check for a draw must be done first.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Line 4 is very clever...

??? I've never been accused of that before.

It just seemed obvious to me. based on Oli's original code; but I notice I forgot what you remembered, which is that you also have to eliminate the case where the team doesn't match either of the participants.

That's why good programming is usually the result of teamwork.

Winston
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:
Oli Jones wrote:I was given the three classes as a template and I had to fill them out! I would have designed it in a different way if I had the option of doing so.
Me too, so I sympathise.

Going back to my very first thoughts when I read this thread, I have to admit that if I was doing this, I wouldn't have had LeagueTable/LeagueTableEntry at all - at least, not the way they were "templated".

I'd have had Team, Match, League, Table and Record classes, with League being basically a Set<Team>, and Record being the equivalent of a table entry, but without the Team component.

Then:
1. Table becomes a wrapper to (or extension of) a Map<Team, Record>, which is what it "feels" like it ought to be.
2. A Record's natural order (ie, implement Comparable for it) can be exactly as you would expect it to appear in a table.
It should then be dead easy to write a Comparator<Map.Entry<Team, Record>> to sort Table entries in "record" order.

You could then get a sorted list of entries at ANY point in a season with the following Table method:
Hope I'm not overloading you here, Oli.

Winston
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!