Refactor Code Smells #1603
Refactor Code Smells #1603
Conversation
|
@elanonc thank you and welcome! However, I'm not sure about two other commits. Could you explain to me why you think it is better to have:
|
| type GoNextType = string | undefined; | ||
|
|
||
| const goNext = (genesisID: GoNextType) => { |
There was a problem hiding this comment.
It might be just
| type GoNextType = string | undefined; | |
| const goNext = (genesisID: GoNextType) => { | |
| const goNext = (genesisID?: HexString) => { |
What is the purpose of extracting it?
There was a problem hiding this comment.
I used a tool that my teacher recommended for detecting code smells. One of these smells was 'Missing Union Type Abstractions' which suggests using type aliases to avoid the repetition of union types.
The tool is called ReactSniffer and is still in development (it's part of a research project at my university). If you have any observations about this code smell, please feel free to share them in the comments, and I'll pass them on to the responsible parties.
Link: https://github.com/maykongsn/react-typescript-code-smells
There was a problem hiding this comment.
Yeah, in case these types will be used (or at least may be used or it is something tricky) — it is fine.
But most of the extracted union types look silly: they would not be used more than once or it is a simple union of primitives — I don't see any benefits.
For example, it is good to have:
type HexOrBytes = Uint8Array | HexString
which also may have a guard, some utils (e.g. toHex or toBytes), may be reused, and so on
But having NumberOrString instead of number | string in some specific function argument — looks silly.
And especially type WeirdName = string | undefined for an argument of the function that accepts optional string +)
I believe that no tool can determine what exactly you need in which case (at least without some well-trained AI model), so it is good to have automatic suggestions, but afterward, the coder should make a decision on what to replace, and what to keep...
Let's keep commits that fix Anys and refactoring of transactions components.
| @@ -1,4 +1,5 @@ | |||
| import React, { useState, useRef, useEffect } from 'react'; | |||
| // import styled from 'styled-components'; | |||
There was a problem hiding this comment.
I will make the corrections and resend. Thank you for the feedback.
|
I had an issue with my computer, so I couldn't make the commit changes before. But finally i did it. |
PR for college assignment. We refactored the Any Type smells, Enum Implicit Values, Missing Union Type, and Large Component.