Imagine that you perform code review in a very large company that develops software for the needs of the space industry. Requirements for reliability, speed, quality of the developed software are the highest.
Review the code presented on the printout, and in the table, the template of which is attached, indicate all the errors made in the code, inaccuracies, potential problems, etc. The more problems you find, the better. Each problem inherent in the code, the error has a certain "weight" in points. Your task is to score the maximum number of points in this test.
01 public class EmployeeList extends LinkedList implements List { 02 // 03 public static final Float _baseHourSalary = new Float("36.60"); 04 public static final Float _high_hour_salary = _maxHourSalary / 4; 05 public static final int _maxHourSalary = _high_hour_salary > 50 ? 110 : 55; 06 public Log log = LogFactory.getLog(); 07 private float[] koefs; 08 09 public EmployeeList() { 10 LoadKoefficients(); 11 } 12 13 @Override 14 public boolean add(Object o) { 15 for (Object obj : this) { 16 if (!(obj instanceof Employee)) continue; 17 if (o.equals(obj)) return false; // , 18 if (((Employee)o).getFio() == " " 19 || ((Employee)o).getFio() == " ") { 20 // 3244, 21 return false; 22 } 23 } 24 return add(o); 25 } 26 27 public void LoadKoefficients() { 28 float[] arr1 = KoefStorage.getKoefArray("ifns_2003_god"); 29 log.debug(" " + arr1.toString()); 30 float[] arr2 = KoefStorage.getKoefArray("ufns_2003_god"); 31 koefs = new Float[2000]; 32 for (int i = 0; i < arr1.length; i++) koefs[i] = arr1[i]; 33 for (int i = 0; i < arr2.length; i++) koefs[i + arr1.length] = arr2[i]; 34 recalculateKoeffs(); // 35 } 36 37 public void recalculateKoeffs() { 38 for (int i = 0; i < koefs.length; i++) 39 koefs[i] = koefs[i] < 0.88 ? koefs[i] / 4 : koefs[i] / 6; 40 } 41 42 public ArrayList getSalaries(LinkedList<Employee> employee) { 43 ArrayList result = new ArrayList(5); 44 for (int i = 0; i < employee.size(); i++) { 45 Employee e = employee.get(i); 46 if (!this.contains(e)) { 47 // . 48 return new ArrayList(0); 49 } 50 Employee found = (Employee) this.get(this.indexOf(e)); 51 result.add( 52 found.getFio() == " " // 53 ? _maxHourSalary : _baseHourSalary); 54 } 55 return result; 56 } 57 58 public LinkedList getSortedView() { 59 DataCollectionsHelper.sortBubble(this, new Comparator() { 60 public int compare(Object o1, Object o2) { 61 if (o1 instanceof Employee && o2 instanceof Employee) { 62 return ((Employee)o1).getTableNumber() - ((Employee)o2).getTableNumber(); 63 } else return -1; 64 } 65 }); 66 return this; 67 } 68 }
In one of the stories of Conan Doyle, the great detective ran into a case that became a real test of his deductive method. The key to the investigation was the solution of a special ciphered language of the “dancing little men” with which the criminals kept their correspondence. In the course of the case, Sherlock Holmes, using his method, was able to calculate how many dancing little men are included in one word, and also identified those who met most often. Solving the meaning of the first word - “How do messages begin? Of course, with greetings, dear Watson! This is elementary! ”- the detective easily calculated all the rest. After that, he was able to participate in their correspondence and expose the gang.
We suggest you to repeat his actions, but with the help of modern technology.
You have:
1) Computer, Java, IDE
2) An uncomplicated encrypted excerpt of an artistic text in Russian
3) The novel "War and Peace" L.N. Tolstoy (if you have a lot of time after decryption, so that you have something to do, you can read a little)
4) A set of 3 tips (â„–1, â„–2, â„–3) that open in turn. Opening each hint reduces the number of points you can get. Only 50 points, opening each hint reduces this amount by 10 points.
Error in the code | Field of knowledge | Points |
Class extends LinkedList already write implements List optional | Style | one |
Constants are not named by convention (HIGH_HOUR_SALARY should be something like this) | Style | one |
Cyclical dependency when declaring a constant | Surface | one |
Float type constants are unreasonable, float must be | Surface | one |
The constructor new float ("36.60") is terrible. | Surface | one |
Calculating _maxHourSalary / 4 will not give the expected result (due to integer division), you must explicitly cast the _maxHourSalary type to float | Hidden bug | 3 |
The name of the method LoadKoefficients is not by agreement (loadKoefficients must be) | Style | one |
If we inherit the list, then there should be generics (generics), without them an anachronism | Style | 2 |
The constructor uses a public method; this is dangerous in the case of inheritance. | Hidden bug | 3 |
The LoadKoefficients and recalculateKoeffs methods are meaningless to make public, as they are obviously only needed for the internal logic of the method. Their publication violates encapsulation. | OOP | 3 |
Judging by the add method, we have an error in the design of the class - this is not a list at all, but a lot (it is checked for re-entry). | Class design | five |
The class behaves incomprehensibly as when adding NOT Employee | Hidden bug | 3 |
Comparing lines for equality (must be equals) | Surface | one |
The o.getFio () == check is performed inside the loop (nothing prevents to check the outside) | Surface | one |
The practice of such a bug fix 3244 itself is flawed, thus never fixing bugs | Class design | 3 |
There are many ways to add an item to the list, but only add is redefined. So the logic given in the overdetermined method will not always work, which is even worse than the most erroneous logic. | Hidden bug | 3 |
Return add (o) causes endless recursion. Must be super.add (o) | Surface | one |
"Ifns_2003_god" - must be a constant | Style | 2 |
In the debug log, I wanted to transfer the entire array listing, and not an unnecessary pointer. Instead of arr1.toString (), it was necessary to use Arrays.deepToString | Logical error | 2 |
Listing the entire array is a long time, it is better to first check whether the debug mode is enabled at all | Performance | 3 |
The Float [] array does not give us anything other than permanent boxing / anboxing. Must be float [] | Surface | one |
Copying arrays needs to be done using Arrays.copy | Performance | 3 |
Why an array of precisely 2000 elements? And what if the loaded will be more or less? | Logical error | 2 |
A bunch of magic numbers in recalculateKoeffs | Style | 2 |
The logic in the data processing class is too tightly wired, these parts of logic usually change a lot, so you cannot write classes (logic is wired in recalculateKoeffs, .getFio () ==) | Class design | five |
What caused the love for LinkedList and inheritance from him? What are the prerequisites for this class? None! | Class design | five |
What is generally due to inheritance? Here it does not correspond to the class task at all (it was necessary to use composition, and not inheritance) | OOP | five |
There is no programming to the interface (there are a lot of links to specific implementations of ArrayList, LinkedList, which should have been avoided) | OOP | 2 |
new ArrayList (5) - and why exactly 5? magic number | Style | 2 |
Passing through the list with a for i = 0 loop and subsequent get is unproductive (especially for LinkedList). Must use for each | Performance | 3 |
Returning an empty ArrayList (0) list, correctly using the EMPTY_LIST constant or the emptyList method in Collections (for a bunch of reasons) | Hidden error, Performance | 3 |
In general, the logic of returning an empty list if one of the elements of the input list is not found inside is extremely dubious and clumsy. | Class design | 3 |
Undocumented public methods. How to guess what these methods do? | Style | 2 |
This getting an item from the list is terrible this.get (this.indexOf (e)) | Surface | one |
Why sortBubble and not Collections.sort? | Surface | one |
An anonymous comparator is created each time a method is called. One could write it in a class field, and even better in a static field. | Performance | 3 |
If the items being compared are not Employee, then less is given. This is an example of what class behavior is not clear (not to mention the breach of the comparator contract) | Logical error | 3 |
The difference comparator has a hidden error in it (for too large or small numbers) | Hidden bug | 3 |
Returning a certain external View to your list by sorting your list is an extremely clumsy class design (not to mention the fact that it breaks encapsulation) | Class design | four |
TOTAL | 95 |
Source: https://habr.com/ru/post/184996/
All Articles