-
Notifications
You must be signed in to change notification settings - Fork 0
Comp 1 #3
base: master
Are you sure you want to change the base?
Comp 1 #3
Conversation
rakeshM-Webonise
commented
Sep 3, 2019
- Added NetworkManager
- Added SQLITE Wrapper
import { Platform, Alert } from "react-native"; | ||
import { STRING_CONSTANTS } from "./string-constant"; | ||
|
||
export default class Utility { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this class for?Lets not name any class or path as plain utility.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class for common methods like date,time operations,alerts and null checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets not club them all together.The class will inflate and SRP(Single Responsibility Principle) will be broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for eg - break this class down into a another class like
export default class Alerter {
static showAlertWithMessage(title,message,navigation,onOkPressed,onCancelPressed){
Alert.alert(
title,
message,
[
{
text: STRING_CONSTANTS.OK_MSG,
onPress: () => onOkPressed()
}
],
{ cancelable: false }
);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utility should act as factory @ameyawebonise what you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then lets name such classes as xxxFactory
.It should return a singleton instance of the class being created though
import { MonoText } from "../components/StyledText"; | ||
import NetworkManager from "../src/network/network-manager"; | ||
|
||
export default class HomeScreen extends React.Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functional component
@@ -1,32 +1,35 @@ | |||
import React from 'react'; | |||
import React from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file extension should be jsx
for all components
__DEV__ | ||
? require('../assets/images/robot-dev.png') | ||
: require('../assets/images/robot-prod.png') | ||
? require("../assets/images/robot-dev.png") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract this outside component as this will hit performance of component on update
<TouchableOpacity onPress={this._handleHelpPress} style={styles.helpLink}> | ||
<Text style={styles.helpLinkText}>Help, it didn’t automatically reload!</Text> | ||
<TouchableOpacity | ||
onPress={this._handleHelpPress} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove _
} | ||
} | ||
|
||
_handleLearnMorePress = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove _
from all methods naming, it's not as per naming conventions
} | ||
} | ||
|
||
//Reference - https://www.npmjs.com/package/expo-sqlite-orm/v/1.4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this
Accept: 'application/json', | ||
'Content-Type': 'application/json', | ||
}; | ||
No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no newline end of file