Hello! I would like to know your opinion about how I write application code. I ask you to judge strictly, to point the finger at everything for which you can scold at work, as a team, well, in general, to the fullest. I don’t know if I can do that here, but I’d risk it. The github code is> https://github.com/ATumbler/metropolis/tree/master/server/so . I posted only what interests me

Thank!

Closed due to the fact that the issue is too general for the participants Alexey Shimansky , Kromster , αλεχολυτ , Denis Bubnov , Mikhail Vaysman 22 Apr '17 at 20:44 .

Please correct the question so that it describes the specific problem with sufficient detail to determine the appropriate answer. Do not ask a few questions at once. See “How to ask a good question?” For clarification. If the question can be reformulated according to the rules set out in the certificate , edit it .

  • Oooh ... I also write. Comments just not enough. - Alexander Semikashev
  • The java code convention is not honored at all. A curly brace with no new line needs to be written, camelCase to use, declare a class to be a variable of just the desired interface (List - ArraytList). Use IDE and auto format. - Victor Khovanskiy
  • @Victor Khovanskiy, oh, my IDE is AIDE, I write code in a smartphone, auto-formatting is there by default, I know that brackets are not ice. - Flippy
  • And about the code itself? :) - Flippy
  • Why do you write such a description to the readme.me file? Such a description is better to add to the commit comment, and the result of the work done is better to mark not the dates, but the version number. - Danil

2 answers 2

If you go down the code, then:

1) Mass indiscriminate imports (do you really need it all?).

2) Do not use access modifiers.

3) Do not follow the rules for naming variables.

4) The Context in the Activity placed in the class field for some reason (is it not better to use this in the case of Activity?).

5) Initialization of List<UserModel> users moved to a separate method, when it could be done immediately in the class field where this variable is declared.

6) Initialization of the UserAdapter adapter after UserAdapter adapter 4 would seem to be done in the class field where this variable is declared, but if you think well, it is taken out separately, only to update this adapter once. RecyclerView will provide you with an adapter on demand .getAdapter() .

7) Some View rendered in the class fields, but are used once for all the code, and some are used at all at the moment of their initialization. For example, the variable fab_add initialized in onCreate in one method, and is used once in the same onCreate , but in another method. In this case, with the experience of the preceding paragraphs, the removal of the code into the methods initViews() , setupViews() , initAdapter() should be discarded, then the variable fab_add in the class field will not have to endure.

8) It is customary to make string resources in a separate string file located in project resources.

9) Permanent creation of a new object new Handler() , when each View contains a Handler . Take any of your .postDelayed rendered in the class field and tell it .post or .postDelayed .

10) Permanent creation of objects throughout the entire code .. new Response.Listener() , new Runnable() , new Response.ErrorListener() .., you will someday be cursed by the garbage collector. For example, different variations of new Runnable() , which, if you look, are likely to be often called. This is where it would be worthwhile to put these Runnable objects into the class field by initializing once and then calling on them.

11) This transfer of MB brackets is used in languages ​​such as C++ , but in Java more common to open a curly bracket without a new line. If you add extra spaces to this and take into account the previous points, then in the UserActivity class the number of lines of code can be reduced from 400 to 200-250 (offhand).

I only looked at one UserActivity class. The rest look lazy :)

  • Wow, thank you so much! Regarding paragraph 9 - you advise instead of creating a new object to reinitialize the old one? - Flippy
  • Pro 9 point - just why create a new Handler object when it already exists in the nearest standard component :) And why are you always doing a second delay? So the designers conceived? - Artilirium
  • No, just network requests are fast and I have not found another way to keep the execution status. Window Dergano appeared / disappeared - Flippy

Before criticism: not bad in general, I would even say very well. Solid 4.

Now criticism:

  1. Violation of the naming style - the name of the interface TrigListener is very confusing - was it really difficult to call TriggerListener ?
  2. In general, what prevented TriggerListener making it a public internal class TriggerActivity ? The integrity of the code would not be violated.
  3. He is very keen on anonymous classes - the “steps” of the code seem to illustrate the problem of readability of the code. It would be possible to dilute lambda here and there with expressions
  4. There are some stylistic problems with curly brackets - they are always from a new line, they are on the old line
  5. Unary if/else framing braces
  6. At least in bins you should use private
  7. Almost do not use annotations (except for the self-evident @Override ) - I recommend you look at ButterKnife
  8. Construction

public class TriggerActivity extends AppCompatActivity implements OnClickListener, TrigListener

I would write like this:

public class TriggerActivity extends AppCompatActivity implements OnClickListener, TriggerListener

  • All problems are due to my IDE ns smartphone, annotations cannot be used, code self-correction makes brackets so, about if-else — I read somewhere that, on the contrary, it’s bad to frame single lines, I honestly forget about access modifiers ... oh, I'll be long get used to AS. Thank! - Flippy
  • Can I criticize? :) On the fifth point - in java code conventions it says just if statements always use braces {} . - post_zeew