Hi!
Here’s my React app demo: https://dariuszb94.github.io/Lux-cars/#/ and code: https://github.com/Dariuszb94/Lux-cars
I have used React, Redux, Styled-Components. Can you tell me if there are any errors or mistakes you can see?
Just taking a quick look through the code, just being my nitpicky self, if I were doing a code review at work…
Store.js should be store.js. We typically reserve Pascal case for classes and components and the files that export them.
Looking at carsReducer.js, wow, that’s a lot of data. I am assuming that is supposed to be dynamically loaded. If it’s supposed to be dynamically loaded, then perhaps it’s best not to default it all? In any case, I would move that data into another file, like carsDefaultData.json or something like that. you can import it and apply it to the initial state if you still want to. I would have a better action name than CARS. I should be able to read that and know what it is doing. Maybe SAVE_CARS or STORE_CARS or LOAD_CARS? I think actions (like most functions) should start with or feature prominently an action verb. This logic should be applied across all your reducers. Your action creators should start with an action verb.
In general, I find some of your formatting irregular. I might get used to using a linter and something like prettier. The bigger the project and the more people that are working on a project, the more important it becomes.
“choosen” isn’t a word. You mean “chosen”.
I’m not sure what you are doing with your action creators - I’ve never seen it done this way. Really, they aren’t action creators, I don’t think, not in the traditional sense. Take choosenPowerMax. An action creator should just return an action object. But you have this.
export const choosenPowerMax = (e) => dispatch => {
dispatch({
type:CHOOSEN_POWER_MAX ,
payload:e
});
};
So, it is a function that takes in a payload (strangely labelled “e” and feeds it to a function that accepts dispatch, something that an action creator should not know or care about. I think it should be:
export const choosePowerMax = payload => ({
type:CHOOSEN_POWER_MAX,
payload,
});
That is it, a function that returns an action object. Then in you component, like say MainSearch.js, when it is fed into connect to created the redux HOC, it will know to wrap it in dispatch. Speaking of which, in MainSearch.js, instead of listing all your dispatching functions in connect:
export default connect(mapStateToProps,{chooseModel, updateMaker, choosenPowerMax, choosenPowerMin, choosenYearMin , choosenYearMax, choosenPriceMin , choosenPriceMax })(MainSearch);
You can pull them out and make it more readable:
const mapDispatchToProps = {
chooseModel,
updateMaker,
choosenPowerMax,
choosenPowerMin,
choosenYearMin,
choosenYearMax,
choosenPriceMin,
choosenPriceMax,
};
export default connect(mapStateToProps, mapDispatchToProps)(MainSearch);
I think that’s a lot easier to read. mapDispatchToProps can take a function, but here we are using the object shorthand.
I think your components should be organized better, into folders. I like mimicking the tree structure of the app, with common components used in several places in a folder src/common. But there are other approaches. But you need to do something - apps can have hundreds of even thousands of components.
This:
<AnyAttr onClick={() => this.makerShow()}>
is the same as:
<AnyAttr onClick={ this.makerShow }>
You are just wrapping it in a middleman function that is calling it. Sometimes you need to - like if you need to pass in parameters that aren’t the default passed for that callback usage, but sometimes you don’t.
This:
<AnyAttrText>{ (this.props.maker.maker === undefined) ? "Any Maker" : this.props.maker.maker}</AnyAttrText>
would probably be the same as:
<AnyAttrText>{ this.props.maker.maker && "Any Maker" }</AnyAttrText>
depending on how your data works.
This:
minYearsShow(){
this.setState({ minYearsShow: !this.state.minYearsShow});
this.setState({ makerShow: false});
this.setState({ modelShow: false});
this.setState({ minPowerShow: false});
this.setState({ maxPowerShow: false});
this.setState({ maxYearsShow: false});
this.setState({ maxPricesShow: false});
this.setState({ minPricesShow: false});
}
would be the same as:
minPricesShow(){
this.setState({
minPricesShow: !this.state.minPricesShow,
makerShow: false,
modelShow: false,
minPowerShow: false,
minYearsShow: false,
maxYearsShow: false,
maxPricesShow: false,
});
}
You don’t need a separate call for each property. But even easier, since you have to set them to false a lot anyway, I would have handled it like this:
const initialState = {
makerShow: false,
modelShow: false,
minPowerShow: false,
maxPowerShow: false,
minYearsShow: false,
maxYearsShow: false,
minPricesShow: false,
maxPricesShow: false,
};
class MainSearch extends Component {
constructor(props) {
super(props);
this.state = initialState;
// ...
minPricesShow(){
this.setState({
...initialState,
minPricesShow: !this.state.minPricesShow,
});
}
And really, since new state is based on old state, you should be using the functional setState:
minPricesShow(){
this.setState(oldState => ({
...initialState,
minPricesShow: oldState.minPricesShow,
}));
}
I haven’t done much work with styled components, but I would find a way to get them out of this file. I don’t like a file over 100 lines. 200 lines gives be the willies. This is over 500. I actually wanted to figure out why you’re using all those setState calls (it could be easier to just set a value that tells you what prop to show if you only want one shown, if I understand) but I don’t want to wade through all that code.
I’m just skipping around now, but I notice in Footer.js:
<GetInTouch></GetInTouch>
This doesn’t make sense. it looks like you are trying to use it as a containment component, but this component but you are not using it as such. You should be using it as:
<GetInTouch />
The other approach would work if you were passing subcomponents, like:
<GetInTouch>
<PhoneNumber />
</GetInTouch>
or something like that. But then the GetInTouch component would have to be rewritten.
Jumping around some more, in SimilarVehicles.js:
componentDidMount() {
this.setState({ cars: this.props.cars});
this.filterCars();
}
Instead of a setState here, I think I’d just do that in the constructor. I feel like the work of filterList could probably be handled there too, but I’m not sure.
This function:
updateList(filteredArray){
let prevState = JSON.stringify([...this.state.cars]);
let newState = JSON.stringify([...filteredArray]);
if(prevState===newState){
}
if(prevState!==newState){
this.setState({ cars: filteredArray});
}
}
This troubles me a lot. OK, first of all, const would be better. And why spread the arrays into an array? Why not just JSON.stringify(this.state.cars)?
And that is kind of a clumsy way to compare two arrays. I think it would be really easy to write a helper function to do that more cleanly by indexing through.
And what does this do:
if(prevState===newState){
}
And if all you’re doing is updating if it’s changed, I would just forget the logic and setState. This is coming off redux state and if you’re using redux properly, the reference to that array should only change if the data changes, because you’re not supposed to mutate state.
This here:
filterModel(car, filteredArray){
if(this.props.cars[this.props.id.choosenId].brand===car.brand || ((this.props.cars[this.props.id.choosenId].price-40000)<=car.price) || ((this.props.cars[this.props.id.choosenId].price+40000)>=car.price)) {
if(filteredArray.length<4)
filteredArray.push(car);
}
}
I would have put all that logic into a helper function to make this more readable. And it concerns me that you are mutating filteredArray.
But like I said, I’m being nitpicky. You put a lot of good work in this. This is hard stuff.
And just to reemphasize, set up eslint and prettier - your code will be cleaner and easier to read. It will make your code look more professional and you will pick up some good habits. They’ll drive you crazy at first, but they’ll pay off in the long run.
1 Like
Can’t really add a lot to that, but I’d suggest using redux toolkit, which should drastically drop the amount of Redux boilerplate code, and comes with thunk/logger/immer/etc built in.
Also re styled components: imo I would pull as many as possible out as simple common components, and put them in their own files under their own folder. Then you use those to build the app, like Lego blocks (I group them in files like Buttons.ts and Inputs.ts but whatever works for you).
Personally, I would keep styled components that are only used with one specific non-common component in the same file – this is personal preference, but it’s quicker to scan if everything relating to one specific piece of logic is in the same place. But ideally the vast majority of them should not really be specific to a single component, you just build up your actual app components with apo logic using a combination of generic styled components (I’ve done a lot of React Native stuff recently where this pattern becomes necessary for sanity, it works well for styled components as well).
1 Like
That’s a huge help, thank you very much. This will help me reduce my bad habits, a lot of things you wrote made me realize that I can do things differently. Thanks 