As part of our “build in the open” strategy and to maintain an effort of making Ethora low-code “super app” engine better and more accessible for React Native and React.js developers, we publish our internal discussion where we take a look at the code review provided to us.
As part of this experiment, we have purchased a basic code review package provided by a specialized code review company and have looked in whether this kind of engagement can be used going forward to provide external feedback and suggestions on improving our project’s open source code base.
Detailed review and all comments can be found using this Github link: https://github.com/dappros/ethora/pull/242
This is a first high level look and just a beginning of this initiative, however we have already made certain conclusions:
- Pull Request confusion – the review team is probably used to be working with larger teams or processes where each code update happens via pull request and is then reviewed before it gets included. They have asked as for a pull request link. From our side we have just responded this is our repo, please check the latest code and please feel free to pick any part of the code to provide feedback on. We wanted to start somewhere but we didn’t expect the code review team to choose a pull request from November 2022 when we have the latest code release in July 2023 and we have updates to the code happening on daily / weekly basis. Next time we’ll be requesting a code review we will make sure to communicate clearly where to look.
- Generic feedback – part of the feedback is generic. This obviously is subjective and not every teach team will be happy to have external criticism in a format of blanket recommendations, but it feels like some of the feedback provided is generic and isn’t quite specific to our project. For example, when we’ve had some sections commented out it only happened in an experimental section and it’s not a typical problem with our code. At the same time, a bunch of general advice and blanket feedback is still very relevant and most of it is definitely useful. We would just appreciate if it was prioritized or focused around specific issues endemic to our project. For generic blanket approach we can always go for automated code analysis tools such as Codeclimate.
- Custom and specific feedback quoting code sections – this part we enjoyed the most as this offers relevant and actionable feedback to us. We would prefer in future engagements with code reviewers to have more of such specific feedback quoting our code and telling us what should be improved there.
- Tools and approaches recommendations – we have appreciated that the code reviewers have included recommendations of specific tools, CI/CD approaches etc which would be useful in our case. Some of them our team was aware of, some not, but it definitely helps to have somebody from outside to remind or suggest what’s available out there to make your work easier and your deliverables more professional.
Transcript for the call – taking a look at the Ethora React Native code review:
Okay so this is as I mentioned uh this is an external company they called aesthetic it’s for their product or a service that we have done we have taken the the the first package available there sort of a code review that they’re providing and they asked about like the pull requests on that they can analyze so they can look into the into our code and provide feedback on the architecture so what I told them is uh okay instead of like they have this package on their website like 150 dollars to do this analytics for the the standard initial package so I told them okay well instead of like doing calls and discussing this for a long time why don’t you just take our code like the latest code from our Repository and look into it and just provide us with like General initial feedback about like anything that you can suggest to improve and based on that we will see how or if we can you know interact better going forward uh what’s not clear for me is uh why they take like for example this pull request or merge from November last year because obviously we update our code quite frequently and we updated it just uh there was latest release just in July this year but maybe they specifically look into pull requests I’m not sure how that works actually but then we have this feedback from them provided last week so it looks like they have this product and probably they automate at least part of it and they see the they provide us with reports so it says like quality git failed not sure what that means this is like the merge done by beneath in November I guess some of this feedback would be still useful for us part of it looks like I obviously automated code review like we have done we have received from some other platforms like cold climate uh but definitely the some of the feedback is written we can see manually by a human and they so let’s look into that so they say included interview critical error review basic architecture review framework best practice basic maintainability subject matter expert review is also included welcome bonus so they talk about welcome bonus I’m not sure why they talk [Music] about that additional insights so they address specific pieces of the code here and yeah so this is this is pretty much that report so I suggest let’s just take a bit closer look into some of the things here uh so what an automated checks found in on this PR so do we know what they talk about Misha like do you know what automatic checks means is it like look what auto test like test coverage what that means I should get this related to here maybe some GitHub actions okay no no it’s mean pull request so they they propose like do CI continuous integration and build the project from source code on every pull request do some Linting called run code linters unit testing UI testing so is this so useful is it all generic is it like do you guys think it’s not relevant or is it like something that each of this direction we would benefit from looking into into this continuous integration it’s like for the deploying code up for example the Firebase yeah I understand but what do you mean specifically for our situation right now is it like something generic like somebody says okay it’s nice to have all that or is it is it like for us it’s useful like to look into one or each of these things because we talked about Unit testing before some of it is implemented we we know what CI means obviously and some of it is Linting I mean yeah that’s another thing UI testing is what we do so I’m not sure like is it like generic so is it like you can just copy paste this and say okay every product every project will benefit from this or is it something that’s that’s kind of relevant for us and it can be seen from our code for all projects okay and then proof of work uh what does it mean no clear context or proof of work is available so they say okay uh short demo video like Loom going through implemented changes short summary of what’s my newly tested so I think this actually like Misha is doing this so for example you’re you’re doing like screenshots of videos when you do like issues uh fixing so that’s super useful and we have improved there summaries and things like that uh but yeah I mean I don’t want to look into like too much into generic things I want to look uh I want to make sure if there is something like specific to us specific to our code that we should we should definitely study it closer um generic stuff is also useful if it’s relevant but we can always welcome to that as well so what is this reusable repository structure or similar tools do you guys do you know about this tool and rwl and X it’s like some abbreviation which is looks pretty complex no I don’t care about this minute so this is some way to organize the react native uh workspace git code I guess repository structure which is more structured and reusable modularity code sharing and product collaboration scalability it’s not clear if this is uh specific top project if we have a problem there or if it’s generic again because if if it’s like just they just copy paste generic stuff then it becomes obviously less useful for us well-defined players so this is uh again abstraction layered architecture they talk again this might be generic but I also was thinking about this can we do more by splitting our sort of logic and design layer and we talked about this for some of the projects when we need for example to change a user profile screen it would be useful if sometimes design layer is separate in the code and that means that we can update for example core engine when we make new version but clients who already customized their design UI representation and that file will be kind of untouched and it will be easier for projects to update so I guess some of this is relevant but they didn’t say okay this is like specifically for you guys and I mean I didn’t expect that for like initial like small package but uh uh this this kind of feedback requires more work obviously for somebody to to understand what’s useful what’s more important what’s relevant for us dependencies again I guess this is like more generic again well this this is probably potentially could be more useful I don’t know like because this talks about our specific code but again question is they done like November and maybe this call has changed already so not sure what’s if we still have this code here we can have it because it’s for laterals but it is it is called inside function I mean it’s very local and it’s very predictable so I don’t see here some some problem okay yeah I see they talk about mutation of classes and like wait it’s good to have like specific feedback about specific things so so they talk about called duplication component structure State Management so we we have a large number of State variables in this component we they probably say that we are not always correctly handling errors using async await I especially when we use like API calls they provide best practices okay they say exclude well okay I mean let’s yeah I’m not sure again how they decided okay let’s choose just this like pull request from November I mean is it I mean how is our repository structure why do you think they made the choice to like choose something from November do we not have like more like latest pull requests uh or you know how did they make this Choice do like what do you think mission Maybe this is their policy like to choose the quote from pull requests but from branches and this is like the latest pull request which we have in November uh let me check on July 19 I think yeah this year right sorry I can see this year right July 19th year it was opened by Buddhist yeah so that’s a bit not clear so I’ll I’ll reach out to them and you know try and find you know how they make that decision and how that works uh obviously again if this is like just you know looking into some of the pieces of the code like and most of it is like copy paste generic it’s it’s less useful but uh if they can offer us more if they can and you can also think from your side like for example if you think that some piece of the code or some some architecture would benefit from external experts like feedback then we can do another exercise like this with them we can say okay guys you have done looked into that whatever you have chosen now let’s let’s do another one and please look into specifically this thing so I’m happy to experiment a bit more that way because if we can get more of like external feedback like that which is focused on something useful then we can improve our product and we can create some plants around it so if you can think of any code that May benefit from that like react native react.js or if Boris for example would like to have feedback on the any of the server side called just think about it let me know and I will reach out to these guys and and do another we’ll do another experiment like this with them so some of these like things I mean I don’t think they’re super useful I I mean obviously if this like if we had lots of comments like left in a lot of code uh yes that would be potentially a problem but if it’s just in some experimental place I mean that’s not super useful I mean just some comments in here like in one paragraph it’s not a problem same here so they say console log with so we’re logging stuff into console and in production it’s better not to use this what do you think about this is there some easy way to automate this so in production the console login is uh switched off or do you think it’s not a problem at all I think that when we make production built it just removes all control lock from code but I’m not sure this is not effective elements but maybe they are right issue maybe even if you remove it so if I was doing this I mean I don’t know like the best practices I would probably just create some variable switch in somewhere in the like config file or somewhere saying okay this is a production version or this is a development version and in production version like they have in some uh like shell apps like you can like manage verbosity levels so you can switch off logging so production version will mean that there is no login into console so maybe that’s that’s something that yeah as you guys they say you say that we can we can use so this is probably useful this kind of feedback maybe it’s a lot of it maybe is obvious but uh it doesn’t mean it’s not useful because sometimes obvious things we just like we’re getting focused on building new features and we we don’t pay attention to things which still will be useful from the obvious kind of part of the things okay so this this is about audio uh so they they complain about lack of comments I guess audio recording process adding brief comment of function header would great to improve readability and maintainability well I guess this is fair enough so they and they provide how they would do it so they say uh and what do you think about this try uh section mission is it like the better way to I think they suggest you know do it it’s like error handling thing right try catch block yeah but I need to have a look into this audio recorder if it like throws an error because it uh it may just uh like it may just be a sink but not throwing an error okay and then they say use react to Tron in your rack native project I don’t know what react to throne is do you know yeah I looked into this uh previous week but you forgot one minute electron this is except for debugging too just another application for debugging right okay this is something we already pointed out before please do not include commented called into in the pr no point repeating same comments okay or whatever yeah that’s that’s clear uh this section render attachment has several responsibilities bundled together code could benefit from better and indentation and spacing uh Arrow functions how to call it Styles async await may be unnecessary in this context card called the text it’s a good practice to move such text strings to a separate localization file at least extract them as constants to make the code more maintainable uh well this is probably related to like Ledger discussion about separating design from the logic or like creating some template files I’m not sure how possible or how uh widespread this is in react native and react.js I think we’ll need to have a separate conversation about that um so this should be discussed as part of The Ledger architecture discussion so my my kind of summary without being like a react nature or xjs developer is that uh part of it and you know quite a big chunk of this is generic feedback that probably would apply to any project part of it is feedback that is kind of relevant but it’s not focusing on the most important or most value generated for our project however some of this feedback is useful and relevant for us for now as well and it’s also inspiring and triggering some discussions and some thinking which is useful for our improving architecture for reducing our uh technical depth for improving the code generally so I think this is useful this is not ideal but this is probably what uh what a good thing good result of like some initial package that can be achieved so that that’s actually that’s my feedback without understanding this from developers like perspective uh Misha Boris do you have like your thoughts about this because I would probably just say Okay think about it have another like discussion internally a little bit and then come back to them and say can you also look into this so try and experiment like ask them to look into another aspect for us so for example we can pick one component or one one code we can think about which parts of the engine that developers would normally interact with more when they do some customizations for example the the section where we have like the selections of chats like the chats menu or something like that so those kind of places which people would probably end up customizing more frequently and we could ask you know can you guys look into this specifically this section and then we can improve it or maybe there is other thing that we want to look into so what do you think Misha and Boris do you think this is useful or any you know better way to engage with uh uh something like this if they provide it like work more examples from our codes that you can fix that they think because we know that we have a large component or large chat screen uh and maybe it will be good to separate some logic from it yeah and I’ll I’m also wondering how to properly uh ensure that internally we have enough time to talk and plan these kind of things so maybe we should set up some recurring like on monthly basis or like something like this like do some architecture discussion for each of our client codes like Rec nature wreck.js code base and think about what architecture improvements and what’s refactoring activities that we should probably be doing ah Boris any feedback from you uh yeah I think they took uh they took this request because it has a lot of changes and they took only uh uh only uh PR requests I mean if we have called the review for example uh and we have several people which you write react native code yeah and one of those people have more experience than another than every changes in code should be made by PR requests when someone when less experienced people write to code they create a PR request first and then more experience people look to the spr request and agree or does not agree with code changes that’s why they took PR request and maybe this is only one PR request which have a lot of changes in the files because we don’t stick with such strategy because we are writing and we don’t reviewing code and uh maybe some yeah they have a lot of genetic things in this review but it depends on the cost I think there is also some tools which can generate such reports and yeah if as you say if we took some features and we’ll ask them to analyze it maybe it will be useful yeah and you’re right about automated things and we used one of them like called cold climate obviously but another problem with them as we already discussed internally is that uh it’s too generic when it’s doing like an automated way so for example the uh the cold climates uh it’s uh telling us okay we have like some problems with maintainability some issues but a lot of those issues for example our team doesn’t agree with so near the Misha or Venice for example agreed with most of these things because they are like generic so in our case for our project for our engine our team says it’s actually fine too because uh well just because of some architecture uh decisions so some of this generic is not applicable generic feedback is not applicable to us so I guess that’s why it’s useful to have this actually some human some external sort of human expert to look into our code 28:08:00 and in their case I think there is some degree of automation but also there is clear feedback from obviously human team or expert who is uh providing this in in a way that’s that’s actually relevant so hopefully they excluded a lot of like what’s not relevant but yeah I mean did you want to add anything else Boris or nothing from my side okay yeah I mean I guess uh that’s that’s so uh I think it’s actually useful uh not of all of it is like useful and but it’s it’s a it’s a great step so if we can set up some process with external reviewers which is can be repeated on regular basis and we can extract value from that process and make it efficient for all of us that that would be fantastic so let’s think about it we’ll discuss it a little bit further and I will come back to this.
