Review code or what's wrong with my test task?

Question

Java

Recently I was interviewing for the position of Middle Android Developer in one of the application development companies.
After talking with HR and a technical interview with a team leader, I was given a homework test. Completion time 5 hours.
Having successfully completed the task in 4.5 hours and sent it to HR, I began to look forward to the result.
The next day I received the answer "The level of performance of the test task leaves much to be desired."
The answer shocked me, to put it mildly... I wrote a letter asking me to describe my mistakes in more detail. In response, I received an ignore ...
I ask those who wish to comment on the execution of this test task. What is wrong with the performance?
Project on GitHub https://github.com/krawa/EstafetaTest
Thank you all!

Answer 1

Personally, I have a code of the form

setupList();
showListProgress(true);
getTaskList();

associated with procedural programming. Java is OOP as far as I know. And there are no patterns, nothing. I don't know how to cover such code with tests. Surely inside there is an appeal to environment parameters or global variables, which are simply a holiday to mock.
I don’t see the separation of logic into layers here, and inside the methods there is both data acquisition and display settings, however, in my opinion, this is not so much a programmer’s problem as a person who set the task of writing an entire application in 5 hours. In my opinion, it would be better to either give a day or give specific tasks for algorithms.
Since I myself work on Sharp, I can’t comment on the style of writing. Maybe some tricks are classics

Answer 2

An opinion on design, since I don’t have skills in java - the code, of course, is written for the machine, and it doesn’t matter how it is written - as long as it works, but read by a person and understand to maintain and expand the functionality - to a person.
adapter/TaskListAdapter.java

public boolean areItemsTheSame(Task item1, Task item2) {
                return !(item1 == null || item2 == null) && item1.getId() == item2.getId();
            }

If not (item1 or item2 are null) and the id of the items are the same...
As I understand it, if you pass item1 = null to the method, then you will have a nullPointerException or something like that?
Why not change the construction to item1 != null && item2 != null && item1.getId() == item2.getId(), I think it will be more readable. It may seem silly to use long variable names like originalTask ​​and taskToCompare, but they are clearly better than item1 and item2, and IDE autocomplete will help with typing
Ternary operator inside if is just as scary as if without opening parentheses
model/Task.java

if (ActualEndDate != null ? !ActualEndDate.equals(task.ActualEndDate) : task.ActualEndDate != null)
            return false;

Let me explain - a blurry eye, the end of the working day and the head does not cook at all. We urgently need to close the task - open the code and see the necessary section that we are going to comment out

...
if (длинное условие)
            doSomething();
doAnotherThing();
...

...
if (длинное условие)
//            doSomething();
doAnotherThing();
...

and now our doAnotherThink is not always executed, but only when the condition is met.
Here is an example of such an error in a real project ("Incorrectly commented out line")
Well,
Where 31 is used several times. For me it is more convenient to have a local variable with a name adequate to the value.
Read Perfect Code by McConnell - the ultimate book for developers

Answer 3

I'm not a programmer at all, not even a java junior.
But for example, a quick look at the comments says that you have never used javadocs, and for a mid-laner, this is most likely inexcusable.
Your variable names seem to have style, but they don't make sense. Looking at the code, it is not clear what exactly is stored in some variable, well, in general.

Answer 4

I myself am not a professional in androyd, rather an amateur, but I’ll insert my 5 kopecks:

private static Context mContext;

    @Override
    public void onCreate() {
        super.onCreate();
        mContext = this;
    }

Xs why so, but it is considered not kamilfo. I use it myself, and I didn’t catch a single crash dump, but this is not welcome in the android community (I hope someone will describe why in the comments). Accepted to pass the context.
In MainActivity, methods for working with the menu could be removed, because, apparently, they are not used anywhere (and apparently remained from the standard template).
In RestClient there is an assignment to a static variable.
I so understand you did singleton. Xs is this acceptable in androyd, but I would make it as an application component. Ideally, all this garbage should be created through DI (at least DI is used everywhere in backend development). For android, this is the Dagger 2 library (I haven’t used it myself yet, but it looks promising).
Plus there:

public static RestAPI get() {
        if(instance == null) instance = new RestClient();
        return instance.restAPI;
    }

This method should be marked as synchronized for good: if 2 threads go there at the same time, it may turn out that 2 RestClients will be created. Here, of course, on the side - this is unlikely to break something, but in large applications it will create some problems (again, I'm talking about the backend, because I'm more into it).
Keeping authentication parameters in this way is also not good, but this is a test task, so that's a side.
Now for the fragments:
You assign views to the properties of the fragment, in onDestroyView it would not be bad to reset them. Better yet, use butterknife.
Calling queries in a fragment is not fashionable these days. It would be nice to make some kind of thread wrapper service over retrofit (service not in the sense of an androyd service, but in the sense of a pattern), which you say download data to me, and then catch messages from it (otto from square will help you with this) . When you show a fragment, you request data and subscribe to events from the service, when you hide a fragment, you unsubscribe.
In response processing, you check if (getActivity() == null) return;. But with this you check the presence of the activity, but not the fragment, the fragment can be destroyed, or its view can be destroyed, or the activity can have another fragment altogether or something. In short, it will all lead to a crash.
onTaskClick: Well, the activity should change the fragments, the list fragment should just say "such and such a task is selected", and call the activity's callback. And already make a decision about what to do next. The official documentation describes how to do this (callback mechanism).
Really confused DetailsTaskFragment: to provide information about one task, you use RecyclerView. IMHO, this is fundamentally wrong. I understand your motivation why you did this (saving memory on views, etc.). But you rigidly set the structure and display type. The slightest change in the task display requirement, and all this code is to be deleted. Well, the tool is wrong: RecyclerView is for lists, for large lists, and the task is not a list, it's an entity. If you abandoned this idea, and used the usual layout + DataBinding, everything would be more convenient, changeable, and there would be 10 times less code.
In the TaskListAdapter, you have business logic mixed in, namely sorting. For good, you should do sorting in another place, for example, in the service that gives you data, or somewhere else, but definitely not in the UI, which is the adapter.
ItemViewHolder - there you have a handler. Xs is it right or not, but in Google's example, the handler is hung up in onBindViewHolder.
I also don’t quite understand (maybe I just didn’t read the task) the addAll method: why not just update the list? Plus, again, this is mostly business logic. And what's strange - I don't see notifyDataSetChanged().
In general, something like this.

Answer 5

All in a bunch. Business logic is mixed with display, the middle must have an idea how to separate it all. It was possible to template the same adapters and fragments. Maybe something else, I didn’t really look =)

Answer 6

Normal code. It's strange that he didn't like it.
GetConverter returns a factory, not a converter, and it would be better to add Factory to the name