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.

mr5
  • 121

4 Answers4

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.

Doc Brown
  • 218,378
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!

Ewan
  • 83,178
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.

JayZ
  • 827
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.

0b1
  • 17