6
\$\begingroup\$

I was rejected after a tech screening for the following code. I mainly struggled to make the code 'production ready'. I was running out of time so I did not create a class for testing. How could I have improved this code?

/* Problem Imagine you are building a friend locator app. Given your current position identified on a bidimensional plane as (0,0) and a list of friend locations, each identified by a (x,y) pair, return the k closest friends to your current location. For simplicity, you can compute the distance between you and your friends using the Euclidean distance formula ( d = sqrt(x^2 +y^2) ). Given a list of friend locations [[1,3], [-2,2]], K = 1 Output: [[-2,2]] */ import java.io.*; import java.util.*; /* * To execute Java, please define "static void main" on a class * named Solution. * * If you need more classes, simply define them inline. */ class Solution { static class Pair{ int x, y; Pair(int x, int y){ this.x = x; this.y = y; } } /* d -> cords time - O(n) space - O(n) */ public static List<Pair> closestFriends(List<Pair> list, int k){ SortedMap<Double, Pair> map = new TreeMap<>(); List<Pair> results = new ArrayList<Pair>(); int i =0; for(Pair pair : list){ double d = distance(pair); map.put(d, pair); } for(Map.Entry<Double, Pair> entry : map.entrySet()){ if(i < k){ results.add(entry.getValue()); i++; } } return results; } public static double distance(Pair p){ return Math.sqrt(p.x*p.x + p.y*p.y); } public static void main(String[] args) { ArrayList<Pair> cords = new ArrayList<>(); cords.add(new Pair(1,3)); cords.add(new Pair(-2,2)); cords.add(new Pair(0,10)); cords.add(new Pair(-2,2)); cords.add(new Pair(1,3)); cords.add(new Pair(-2,2)); int k =1; /* empty list of cords negative k list list of cords test cases for distance - correct calculation */ List<Pair> res = closestFriends(cords, k); for(int i =0; i < res.size(); i++){ System.out.println(res.get(i).x + "," +res.get(i).y); } } }
\$\endgroup\$
2
  • 1
    \$\begingroup\$ class Solution { is something I'd expect to see in a programming-challenge, not in production code. Was this provided by the company? \$\endgroup\$ Commented Jul 12, 2021 at 23:18
  • 2
    \$\begingroup\$ Why not use streams? It's a one-liner with streams... return cords.stream().sorted(Comparator.comparingDouble(p -> Math.hypot(p.x, p.y))).limit(k).collect(Collectors.toList()). \$\endgroup\$ Commented Jul 13, 2021 at 9:20

1 Answer 1

Reset to default
10
\$\begingroup\$

major issue: misunderstanding of exercise

you should find the amount k of friends - not the friends within an distanz k

major issue: bug

for(Pair pair : list){ double d = distance(pair); map.put(d, pair); }

if two "pair"s have the same distance you'll lose one result

major issue: naming

sounds obvious but you should create a Position not a Pair (also named in your exercise as Location - but never a Pair)

minor issue: Map instead of a List (mostly assumption on this)

your exercise wants you to return a the first k items of a sorted list. I had expected you to use a List and create a Comparator, sort the list and then return the k first entries. you use a map to sort the element. and it's a buggy (see above) algorithm to sort.

minor issue: Java coding conventions, not considering encapsulation

as example I say that int x, y; should be:

int x; int y;

and formatting, use the spaces properly

minor issue: naming

  • when they give you the amount k you could pick up and rename that variable into a proper name
  • when they give you a list of friends that you should filter rename that list properly

public static List<Pair> closestFriends(List<Pair> friends, int amount)...

another one here: I'm missing a proper verb in your method name: getClosestFriends() or maybe findClosestLocations()

\$\endgroup\$
6
  • 3
    \$\begingroup\$ Another major issue is naming number 3: List<Pair> list - I'd already kick the OP out of my office for that. Yes, we see from the data type that the variable is a list, there's no need to repeat this information in the variable name. But what does it contain, what does it signify? No indication. \$\endgroup\$ Commented Jul 13, 2021 at 5:52
  • 1
    \$\begingroup\$ yes, yes, very obvious, my bad i didnt see it! List<Pair> list should be named properly: List<Pair> candidates or fiends or similar (still Pair is improper) \$\endgroup\$ Commented Jul 13, 2021 at 5:57
  • 6
    \$\begingroup\$ I don't get why you're saying the first major issue is that he misunderstood the exercise. Best I can tell, he did in fact find the K closest friends. \$\endgroup\$ Commented Jul 13, 2021 at 9:58
  • 1
    \$\begingroup\$ @MartinFrank tbh, in logner code the Data type in the declaration can be far away. My lists are named candidatesList (or similar), just to be on the very same side. Also makes it obvious at a glance whether that one line is referencing candidateS oder a candidate. \$\endgroup\$ Commented Jul 13, 2021 at 10:50
  • 1
    \$\begingroup\$ @Hobbamok whyen you use the Type as a variablen Name, this is called HungarianNotation. Things get bad when you change the type: then you have to rename the variablen name as well! (see en.wikipedia.org/wiki/Hungarian_notation) and why this is always a point of discussion: softwareengineering.stackexchange.com/questions/102689/…) \$\endgroup\$ Commented Jul 13, 2021 at 11:18

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.