1

I'm new in the team (< 3 weeks) but I believe I am experience enough as a Xamarin dev to know what I'm doing and also experience enough on using Prism library to give some criticism on how things should be done on an existing code base.

On our next sprint, I am planning to give some dev talk and raise some concerns for the following:

  1. Image files are located on the project root folder (App/App.PlatformName/**.png_or_jpg) which I believe should be put in another folder, specifically App/App.UWP/Images/*.png_or_jpg
  2. Image names are hardcoded as string literal everywhere. I think they should be put in a single file/class which contains all the image names as constant string and this should be generated by a script that triggers every pre-build.
  3. Lazy loading a Command, more specifically, a DelegateCommand is over engineering. The type is not even resource intensive to be a candidate for lazy loading.
  4. Consider the code below:
IsBusy = true; // I believe this would show up some UI blockers
// some synchronous stuffs
IsBusy = false; // hide the blockers

I believe the code doesn't really do the intended purpose. If the synchronous stuff takes more time, say > 5 seconds, it would cause an ANR (application not responding), instead of showing some UI blockers. This would only work if the operation in the middle is an awaited Task, and if the process is CPU-bound operation. Wrapping it with IsBusy property does nothing.

  1. String literals are stored on a .cs files (noticed the S) instead of a single .resx which would make localization a nightmare task although there's really no task for it but I came from mobile development background and this is always put in mind during development.

There is more concerns from me but I'll cut it out here.

My questions are: Should I proceed on doing this? Would I appear cocky in front of the team? Would you do the same or let it go since the clients are not really concern about these things?

Thank you and appreciate your time on reading this.

3
  • Does this answer your question? How can I tactfully suggest improvements to others' badly designed code during review?
    – gnat
    Jan 13, 2021 at 19:55
  • @gnat thank you for linking me into that question. Although I get some insights from it, it still doesn't answer my questions. Also, this more from a new hire perspective and I haven't gathered the trust of my colleagues yet.
    – mr5
    Jan 13, 2021 at 20:09
  • You should start with questions, before "offering constructive criticism"
    – Fabio
    Jan 15, 2021 at 0:26

4 Answers 4

11

I have no experience with Prism library, but let me comment on the things you brought on the table:

  1. Image files are located on the project root folder (App/App.PlatformName/**.png_or_jpg) which I believe should be put in another folder, specifically App/App.UWP/Images/*.png_or_jpg

If you question this, be prepared to answer the question "why"? Can you explain a clear benefit like "there are so many images among other files in that folder that we start to loose oversight"? Or is your main argument something like "at my former employer, we always separated such files"? The latter is a pseudo-argument - if that is what you have in mind, better hold back with any critics.

Moreover, there will be some effort to change access to the current location in the code base, and the software will not only have to be changed, but also reviewed and tested after such a change. Be prepared to explain why you think the benefit will justify that effort, and be prepared to suggest a way which keeps the effort for this change low.

  1. Image names are hardcoded as string literal everywhere. I think they should be put in a single file/class which contains all the image names as constant string and this should be generated by a script that triggers every pre-build.

Same as #1. Moreover, putting all names into a single class file is not necessarily a good idea, since it creates a dependency to a single point. Whenever that file is recreated or touched, this may trigger a full rebuild of the application. So if you want to suggest this, can you explain to the team for sure why this drawback is really worth the hassle?

  1. Lazy loading a Command, more specifically, a DelegateCommand is over engineering. The type is not even resource intensive to be a candidate for lazy loading.

Maybe that's true, maybe you haven't fully understood the reasons behind this implementation. In case you have the task to work on a command's implementation, and the lazy-loading makes the change more complicated, then it is perfectly justified to ask the team if they agree if you remove the lazy loading to make things simpler. But you should approach this with a mindset of "I guess it is not needed, but I could be wrong, so I will test this change thoroughly" and not with a mindset of "this lazy-loading looks like crap, lets get rid of it and hope nothing will break".

  1. Consider the code below: ... ... I believe ...

This is clearly something you need to discuss with someone who knows the code base better than you. "Believing" is not a good basis for a change which can have non-local effects. If you come to the point where you can honestly say "I have made a detailed impact analysis on this ...", then you have a way better foundation for changing this code. Moreover, if the code is working well now, and it currently does not cause any issues, and you don't have a task which requires to change something around or related to it, you should consider just to make an entry in your teams issue tracker to analyse or change it later.

  1. String literals are stored on a .cs files (noticed the S) instead of a single .resx which would make localization a nightmare task although there's really no task for it

If there is no requirement for localization, and maybe there will never be for the specific application, you better hold your critics back here. The "Just in case" mindset is the most direct path into overengineering and wasting time for no apparent benefit. As I wrote above, "I was used to this from my former employer" is a strawmen, not a valid argument. Moreover, I guess it is debatable if localization would become "a nightmare", as you wrote, or just a little bit more work in case it will ever become a goal.

In short, keep in mind refactoring and restructuring code is not an end in itself, it is a means to an end, and never comes completely for free. Whenever you do this, make sure you know why you are doing it, how the cost-benefit relationship will be, and which risks a certain change will introduce.

2
  • ...make sure there are enough automatic tests -- This is UI (already difficult to automate tests for), and I would imagine testing whether or not a DelegateCommand lazy loads or not would be difficult, since it is a private implementation detail. Jan 14, 2021 at 14:18
  • @RobertHarvey: thanks for the reality check ;-) What I had in mind was not testing the "lazy loading", but whether the DelegateCommand still works properly after the "lazy load" was removed, which, in theory, should not affect its behaviour. But I removed that sentence, since I already mentioned testing in the sentence before.
    – Doc Brown
    Jan 14, 2021 at 15:23
7

No. It's too soon,

We all know that code is a product of compromises, you are assuming these mistakes are there because of lack of knowledge, but its probably a combination of factors that have lead to them.

Make sure you understand these factors before you criticise the team.

But also, where are the tests that fail due to these errors? MOAR TESTS REQUIRED!

6
  • I haven't explored the project that deals with unit testing yet. Yeah, it's kind of early to do these things but I want to hear opinions of other people in this regard. "We all know that code is a product of compromises, you are assuming these mistakes are there because of lack of knowledge" - couldn't it be that the compromise is a lack of knowledge on the subject matter? For the unit testing, I haven't explored that part yet but I assume #4 doesn't fail but how about the numbers 1,2,3,5?
    – mr5
    Jan 13, 2021 at 20:18
  • presumably your argument is that those would fail various performance, cross platform or localisation tests?
    – Ewan
    Jan 13, 2021 at 20:28
  • 1
    Yes, but I don't think those things can be covered through Unit Testing. It's more of a established coding practices.
    – mr5
    Jan 13, 2021 at 20:33
  • well, think about that carefully. If there is no failed test or requirement. How do you justify your need to correct these things? You'll just get push back such as "there's no requirement..." or "that takes too long" etc Are your objections simply coding style? or will they make a better product?
    – Ewan
    Jan 13, 2021 at 21:18
  • 1
    I think most of your stuff does have real effects on the product, but you should make end to end tests for performance, localisation etc first if people agree to the tests and the tests fail and you have a solution that makes them pass... youre good. if you just claim an effect and that the effect is important and the you can improve the effect and there are no negative side effects from the improvement, well.. not so good
    – Ewan
    Jan 15, 2021 at 8:54
2

I will say yes, but not in the "The code is bad and need to be fixed" way. The correct way would be to question the state of things but not all at once, more like when you encounter it or when a discussion or feature shift around the issue you identified.

Some examples:

  • Why is X being done like this ? I saw it done differently and would like to know the difference ?
  • I never saw Y being done like that, I was under the impression that it caused Z issue, was I wrong ?

The thing is, your colleagues and seniors probably now that those things are bad or not at the state of the art, but they probably don't have the time or security (unit tests, ...) to do it better.

Raising those issue without pointing fingers has many advantages:

  • It shows that you're curious about how things are done
  • It shows that you are skilled enough to question the way things have been done with good arguments, or enough humility to learn.
  • You may learn new ways to do things

Overall it builds your reputation in the team. Just be careful not to become that guy that is always criticizing the way things have been done/are done.

1

Ensure you fully understand the reasons for the difference before offering feedback or you risk damaging your credibility early in your new position.

Remember, you don't know what you don't know, so rule out every possibility.

Chances are you are right but if you make sure you avoid the risk of looking like a bull in a china shop and/or inadvertently I silting another dev.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.