DISCUSSION: Refactoring to reduce duplicate code

[pedrouid]

If you have taken a look at the source code, there is a lot of code that is duplicated which makes unit testing harder and debugging even worse. The major stress points are the modals and relevant reducers.

There is too much duplication in these areas which I'm working on cutting down as develop new features, but I would love to hear second opinions and suggestions on how to tackle this.

Let's debate this openly, ideally a more functional approach than object-oriented, but feel free to change my mind if you think it would improve the source code dramatically.

PS - Also want to aim to reduce dependencies rather than increasing them. Right now, react-scripts is a very well maintained packaged but it also exposes the app to many vulnerabilities.

[ricburton]

Can we refactor the title to discussion :P

Richard

Sent via Superhuman ( https://sprh.mn/?vip=richard@balance.io )

On Tue, May 22, 2018 at 11:29:05, Pedro Gomes < notifications@github.com > wrote:

If you have taken a look at the source code, there is a lot of code that is duplicated which makes unit testing harder and debugging even worse. The major stress points are the modals and relevant reducers.

There is too much duplication in these areas which I'm working on cutting down as develop new features, but I would love to hear second opinions and suggestions on how to tackle this.

Let's debate this openly, ideally a more functional approach than object-oriented, but feel free to change my mind if you think it would improve the source code dramatically

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub ( https://github.com/balance-io/balance-manager/issues/238 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAavkHaQFFSyPCEdVyO0OBLZVYNPe2s-ks5t1C7BgaJpZM4UI4hT ).

[samelliott89]

Some commentary after spending time in the codebase sprinkled in with some some good react hygiene suggestions. I've also taken into account that this is an OS front-end project which comes with a whole set of it's own challenges. This is also not a critique of the codebase:

• If a piece of code contains logic, it should be its own component. Some of the files are huge because of not enforcing this. You can see the run on effect here with the size of the PropType objects.
• Look at moving the style logic to its own respective file ./styles for each component i.e have a component/SendModal/index.js and component/SendModal/styles. Styles code doesn’t change as often as the business logic and it can take up a lot of room in component files.
• Let’s not be scared to break up some of the logic in said components into their own functions as to avoid ~100 line functions with a bunch of if/else statements. This will better on-ramp new contributors to the code base and make the code much easier to read and follow.
• PropTypes are a weak enforcement method, but it we’re going to use them, having them at the top of the file to initially describe the interface of the component is useful.
• Begin to steer away from string literals and just have some basic static objects to store this data.
• Having single functions in helpers/validators/someHelper.js than twenty functions (even small) in huge helpers/validators.js (I can see we’re moving to this)
• Something like https://sentry.io/ can be very useful for monitoring

I’ll update this comment if I think of more things but just put this together this morning. Again, excited to be helping this project.

[ricburton]

Thanks for your thoughts here ❤️

On Wed, 23 May 2018 at 12:34, Sam Elliott notifications@github.com wrote:

Some commentary after spending time in the codebase sprinkled in with some some good react hygiene suggestions. I've also taken into account that this is an OS front-end project which comes with a whole set of it's own challenges. This is also not a critique of the codebase:

• If a piece of code contains logic, it should be it’s own component. Some of the files are huge because of not enforcing this. You can see the run on effect here with the size of the PropType objects.
• Look at moving the style logic to its own respective file ./styles for each component i.e have a component/SendModal/index.js and component/SendModal/styles. Styles code doesn’t change as often as the business logic and it can take up a lot of room in component files.
• Let’s not be scared to break up some of the logic in said components into their own functions as to avoid ~100 line functions with a bunch of if/else statements. This will better on-ramp new contributors to the code base and make the code much easier to read and follow.
• PropTypes are a weak enforcement method, but it we’re going to use them, having them at the top of the file to initially describe the interface of the component is useful.
• Begin to steer away from string literals and just have some basic static objects to store this data.
• Having single functions in helpers/validators/someHelper.js than twenty functions (even small) in huge helpers/validators.js (I can see we’re moving to this)

I’ll update this comment if I think of more things but just put this together this morning. Again, excited to be helping this project.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/balance-io/balance-manager/issues/238#issuecomment-391414680, or mute the thread https://github.com/notifications/unsubscribe-auth/AAavkCgyMwkU2vrnNl6qOR8zCTqmXmpcks5t1Y-mgaJpZM4UI4hT .

-- Richard

[pedrouid]

@samelliott89 Totally onboard with these, I will start moving towards these approaches.

Can you just expand on the string literals? Is this related to the multiple state variables like AccountType, Network, etc?? I tend to use these to ensure state changes happen accordingly and avoid scenarios where objects transport the same reference and not trigger Redux state change. However I’m open to tackling this better.

Thanks again for the comprehensive code review!

_{Sent with GitHawk}

[mikedemarais]

@pedrouid @samelliott89 also wondering what ya mean in regards to string literal. thinking it could be the styled-components or perhaps he means using something like IndexDB instead of stringifying to localstorage https://github.com/balance-io/balance-manager/blob/81bc4f404f6f9f972c429d8bfb5bb63073b92b8b/src/handlers/localstorage.js#L8

[samelliott89]

@pedrouid More in reference to code like:

this.props.gasPriceOption === 'slow'

Could be like import { gasSpeed } from ./utils/... this.props.gasPriceOption === gasSpeed.slow

Just tidying up that sort of stuff, unless you think this can have unintended consequences with redux.

[pedrouid]

Sounds good, I really like this spec!

[samelliott89]

@pedrouid great. LMK if you want to say have a crack at doing this for one area of the code base and then I can lean in and roll out over other parts. Here to help.

[pedrouid]

@samelliott89 That would be great, if you find the time to have a crack on it, let me know if you bump into some quirks or weird parts of the codebase

[arku]

@pedrouid I would love to help with the refactor!

What do you think of breaking the refactoring task into a bunch of small focused issues on each of @samelliott89 's points?

[ricburton]

@pedrouid is focusing on WalletConnect.org.

@jinchung and I are here for this. We are currently laser focused on Shipping Exchange v1 with ShapeShift then we’d love to give this refactor the attention it deserves. On 9 Jun 2018, 23:58 -0400, Arun Kumar notifications@github.com, wrote:

@pedrouid I would love to help with the refactor! What do you think of breaking the refactoring task into a bunch of small focused issues on each of @samelliott89 's points? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.