1
\$\begingroup\$

I have built a basic app which uses firebase for authentication.

The app is pretty simple.

  • Sign up the user to firebase using email and password.
  • Sign in the user
  • Sign out the user

Also, I have added some code to check if the user is already signed in. If he/she is already signed in, then proceed to the dashboard, otherwise navigate back to the login page.

I'd like to see where I am going wrong, and any room for improvement.

You can find the project here: https://github.com/98gzi/FirebaseProject

FirebaseProject/App.js

import { StyleSheet } from "react-native";
import React from "react";
import { NavigationContainer } from "@react-navigation/native";
import { createNativeStackNavigator } from "@react-navigation/native-stack";
import LoginScreen from "./src/screens/LoginScreen";
import { Provider } from "./src/context/DataContext";
import SignupScreen from "./src/screens/SignupScreen";
import IndexScreen from "./src/screens/IndexScreen";
import { setNavigator } from "./src/navigationService";
import InitialScreen from "./src/screens/InitialScreen";

const Stack = createNativeStackNavigator();

const App = () => {
  return (
    <Provider>
      <NavigationContainer
        ref={(navigator) => {
          setNavigator(navigator);
        }}
      >
        <Stack.Navigator>
          <Stack.Screen
            name="InitialScreen"
            component={InitialScreen}
            options={{ headerShown: false }}
          />
          <Stack.Screen
            name="Signin"
            component={LoginScreen}
            options={{ headerShown: false }}
          />
          <Stack.Screen
            name="Signup"
            component={SignupScreen}
            options={{ headerShown: false }}
          />
          <Stack.Screen
            name="Index"
            component={IndexScreen}
            options={{ headerShown: false }}
          />
        </Stack.Navigator>
      </NavigationContainer>
    </Provider>
  );
};

export default App;

const styles = StyleSheet.create({});

FirebaseProject/firebase/firebaseConfig.js

import { initializeApp } from "firebase/app";
import { getFirestore } from "firebase/firestore";
import { getAuth } from "firebase/auth";

const firebaseConfig = {
  // Removed code
};

// Initialize Firebase
const app = initializeApp(firebaseConfig);

export const auth = getAuth(app);
export const db = getFirestore();

FirebaseProject/src/components/AuthForm.js

import { StyleSheet, Text, View, TextInput, Button } from "react-native";
import React, { useEffect, useState } from "react";

const AuthForm = ({ headerText, buttonText, onSubmit, errorMessage }) => {
  const [email, setEmail] = useState("");
  const [password, setPassword] = useState("");

  return (
    <View style={styles.container}>
      <Text>{headerText}</Text>
      <TextInput
        style={styles.input}
        placeholder="Email"
        value={email}
        onChangeText={setEmail}
      />
      <TextInput
        style={styles.input}
        placeholder="Password"
        secureTextEntry={true}
        value={password}
        onChangeText={setPassword}
      />
      {errorMessage ? (
        <Text style={styles.errorMessage}>{errorMessage}</Text>
      ) : null}
      <Button
        title={buttonText}
        onPress={() => onSubmit({ email, password })}
      />
    </View>
  );
};

export default AuthForm;

const styles = StyleSheet.create({
  container: {
    margin: 10,
  },
  input: {
    height: 40,
    margin: 12,
    borderBottomWidth: 1,
    borderRadius: 5,
    padding: 10,
  },
  errorMessage: {
    margin: 12,
    color: "red",
    marginBottom: 15,
  },
});

FirebaseProject/src/context/createDataContext.js

import React, { useReducer } from "react";

export default (reducer, actions, initialState) => {
  const Context = React.createContext();

  const Provider = ({ children }) => {
    const [state, dispatch] = useReducer(reducer, initialState);

    const boundActions = {};
    for (let key in actions) {
      boundActions[key] = actions[key](dispatch);
    }

    return (
      <Context.Provider value={{ state, ...boundActions }}>
        {children}
      </Context.Provider>
    );
  };

  return { Context, Provider };
};

FirebaseProject/src/context/DataContext.js

import createDataContext from "./createDataContext";
import { auth, db } from "../../firebase/firebaseConfig";
import {
  createUserWithEmailAndPassword,
  onAuthStateChanged,
  signInWithEmailAndPassword,
  signOut,
} from "firebase/auth";
import { navigate } from "../navigationService";
import { addDoc, collection } from "firebase/firestore";

const dataReducer = (state, action) => {
  switch (action.type) {
    case "signin":
      return { errorMessage: "", user: action.payload };
    case "signup":
      return { errorMessage: "", user: action.payload };
    case "add_error":
      return { ...state, errorMessage: action.payload };
    case "clear_error_message":
      return { ...state, errorMessage: "" };
    default:
      return state;
  }
};

const clearErrorMessage = (dispatch) => () => {
  dispatch({ type: "clear_error_message" });
};

const isSignedIn = (dispatch) => () => {
  onAuthStateChanged(auth, (userCred) => {
    if (userCred) {
      dispatch({ type: "signin", payload: userCred });
      navigate("Index");
    } else {
      navigate("Signin");
    }
  });
};

const signin =
  (dispatch) =>
  ({ email, password }) => {
    signInWithEmailAndPassword(auth, email, password)
      .then((userCredential) => {
        // Signed in
        const user = userCredential.user;
        dispatch({ type: "signin", payload: user });
        navigate("Index");
      })
      .catch((error) => {
        const errorCode = error.code;
        console.log(errorCode);
        if (errorCode === "auth/wrong-password") {
          dispatch({
            type: "add_error",
            payload: "Incorrect Password",
          });
        } else if (errorCode === "auth/invalid-email") {
          dispatch({
            type: "add_error",
            payload: "Invalid Email",
          });
        } else {
          dispatch({
            type: "add_error",
            payload: error.code,
          });
        }
      });
  };

const signup =
  (dispatch) =>
  ({ email, password }) => {
    createUserWithEmailAndPassword(auth, email, password)
      .then(async (userCredential) => {
        // Signed in
        const user = userCredential.user;

        await addDoc(collection(db, "users"), {
          worked: true,
        });

        dispatch({ type: "signup", payload: user });
      })
      .catch((error) => {
        const code = error.code;
        if (code === "auth/email-already-in-use") {
          dispatch({
            type: "add_error",
            payload: "Email already in use",
          });
        } else if (code === "auth/invalid-email") {
          dispatch({
            type: "add_error",
            payload: "Invalid email",
          });
        } else if (code === "auth/weak-password") {
          dispatch({
            type: "add_error",
            payload: "Weak password",
          });
        } else {
          dispatch({
            type: "add_error",
            payload: error.code,
          });
        }
      });
  };

const signout = (dispatch) => () => {
  signOut(auth)
    .then(() => {
      dispatch({ type: "signin" });
      navigate("Signin");
    })
    .catch((err) => {
      console.log(err);
      dispatch({
        type: "add_error",
        payload: err.code,
      });
    });
};

export const { Context, Provider } = createDataContext(
  dataReducer,
  { signup, signin, isSignedIn, signout, clearErrorMessage },
  { user: null, errorMessage: "" }
);

FirebaseProject/src/screens/DashboardScreen.js

import { StyleSheet, Text, View } from "react-native";
import React from "react";

const DashboardScreen = () => {
  return (
    <View>
      <Text>DashboardScreen</Text>
    </View>
  );
};

export default DashboardScreen;

const styles = StyleSheet.create({});

FirebaseProject/src/screens/IndexScreen.js

import { StyleSheet, Text, View } from "react-native";
import React, { useContext } from "react";
import {
  createDrawerNavigator,
  DrawerContentScrollView,
  DrawerItem,
  DrawerItemList,
} from "@react-navigation/drawer";
import DashboardScreen from "./DashboardScreen";
import Schedule from "./Schedule";
import { Context } from "../context/DataContext";

const Drawer = createDrawerNavigator();

const SignOutButton = (props) => {
  const { signout } = useContext(Context);
  return (
    <DrawerContentScrollView>
      <DrawerItemList {...props} />
      <DrawerItem label="Sign out" onPress={() => signout()} />
    </DrawerContentScrollView>
  );
};

const IndexScreen = () => {
  return (
    <Drawer.Navigator
      initialRouteName="Home"
      drawerContent={(props) => <SignOutButton {...props} />}
    >
      <Drawer.Screen name="Home" component={DashboardScreen} />
      <Drawer.Screen name="Schedule" component={Schedule} />
    </Drawer.Navigator>
  );
};

export default IndexScreen;

const styles = StyleSheet.create({});

FirebaseProject/src/screens/InitialScreen.js

import { StyleSheet, Text, View } from "react-native";
import React, { useContext, useEffect } from "react";
import { Context } from "../context/DataContext";
const InitialScreen = () => {
  const { isSignedIn } = useContext(Context);
  useEffect(() => {
    isSignedIn();
  }, []);
  return null;
};
export default InitialScreen;
const styles = StyleSheet.create({});

FirebaseProject/src/screens/LoginScreen.js

import {
  Button,
  StyleSheet,
  Text,
  TextInput,
  TouchableOpacity,
  View,
} from "react-native";
import React, { useContext, useEffect } from "react";
import AuthForm from "../components/AuthForm";
import { Context } from "../context/DataContext";
import { SafeAreaView } from "react-native-safe-area-context";

const LoginScreen = ({ navigation }) => {
  const { state, signin, clearErrorMessage } = useContext(Context);

  useEffect(() => {
    const unsubscribe = navigation.addListener("focus", () => {
      clearErrorMessage();
    });
    return unsubscribe;
  }, [navigation]);

  return (
    <SafeAreaView>
      <View>
        <AuthForm
          headerText="Login into Combat Coach"
          buttonText="Login"
          onSubmit={signin}
          errorMessage={state.errorMessage}
        />

        <TouchableOpacity onPress={() => navigation.navigate("Signup")}>
          <Text style={styles.navLink}>Don't have an account? Sign up</Text>
        </TouchableOpacity>
      </View>
    </SafeAreaView>
  );
};

export default LoginScreen;

const styles = StyleSheet.create({
  navLink: {
    color: "#0000EE",
    textAlign: "center",
  },
});

FirebaseProject/src/screens/Schedule.js

import { StyleSheet, Text, View } from "react-native";
import React from "react";

const Schedule = () => {
  return (
    <View>
      <Text>Schedule</Text>
    </View>
  );
};

export default Schedule;

const styles = StyleSheet.create({});

FirebaseProject/src/screens/SignupScreen.js

import { StyleSheet, Text, View, TouchableOpacity } from "react-native";
import React, { useContext, useEffect } from "react";
import AuthForm from "../components/AuthForm";
import { Context } from "../context/DataContext";
import { SafeAreaView } from "react-native-safe-area-context";
const SignupScreen = ({ navigation }) => {
  const { state, signup, clearErrorMessage } = useContext(Context);
  useEffect(() => {
    const unsubscribe = navigation.addListener("focus", () => {
      clearErrorMessage();
    });
    return unsubscribe;
  }, [navigation]);
  return (
    <SafeAreaView>
      <View>
        <AuthForm
          headerText="Sign up to Combat Coach"
          buttonText="Signup"
          onSubmit={signup}
          errorMessage={state.errorMessage}
        />
        <TouchableOpacity onPress={() => navigation.navigate("Signin")}>
          <Text style={styles.navLink}>Already have an account? Sign in</Text>
        </TouchableOpacity>
      </View>
    </SafeAreaView>
  );
};
export default SignupScreen;
const styles = StyleSheet.create({
  navLink: {
    color: "#0000EE",
    textAlign: "center",
  },
});

FirebaseProject/src/navigationService.js

import { CommonActions } from "@react-navigation/native";
let navigator;

export const setNavigator = (nav) => {
  navigator = nav;
};

export const navigate = (routeName, params) => {
  navigator.dispatch(
    CommonActions.navigate({
      name: routeName,
      params,
    })
  );
};
\$\endgroup\$
0

1 Answer 1

1
+400
\$\begingroup\$

Review

Overall the code looks decent. It seems the code is separated into components well, and indentation is consistent. It makes good usage of hooks, and const for values that are never re-assigned. There is at least one place where let is used and const could be used instead - that is in src/context/createDataContext.js the for loop within the Provider definition.

Most of the functions are succinct though there are a couple that are a bit on the long side - e.g. in src/context/DataContext.js the function signin occupies 30 lines, signup is 38 lines, etc. Some of those have repeated code - e.g. in the catch callbacks so abstracting common code to a separate function would help make those shorter and also eliminate the number of places code needs to be updated if something happens to change (e.g. in a dependent library).

Suggestions

Documentation

Documenting the code

While most of the code is self-documenting, it might help anyone reading the code (including your future self) to have comments above each function - e.g. documenting the parameters, return values, any possible exceptions that may be thrown, etc.

A common convention is to include a Readme file in markdown format in the root of the project. This can include information about installation/setup, testing, etc. While initializeApp is documented on the firebase Docs it would be helpful to know which configuration options are required, optional, etc. For example - I saw the following in /firebase/firebaseConfig.js:

const firebaseConfig = {
  // Removed code
};

and I had to figure out what was needed instead of Removed code.

Use a linter

Unless you are already using a linter, I suggesting finding one like ESLint. I have it enabled in my IDE (i.e. PHPStorm) and it can typically be added as an extension in many other popular IDEs. There is also a Demo available on the website where code can be pasted to see what types of suggestions are offered. Some of the suggestions below came from ESLint messages.

Unused variables

Perhaps this is leftover boiler-plate code in many cases, but some variables are assigned but never used after that. For example:

const styles = StyleSheet.create({});

This appears five times within the code pasted above. There is no need to assign those variables if they aren't used, and perhaps there may be no need to create an empty stylehseet object.

Eliminating single use variables

In src/screens/LoginScreen.js there is a variable unsubscribe declared within the callback to useEffect within LoginScren, which is returned immediately after it is assigned.

useEffect(() => {
    const unsubscribe = navigation.addListener("focus", () => {
      clearErrorMessage();
    });
    return unsubscribe;
  }, [navigation]);

There is little point in assigning this variable. It can be simplified to simply returning the return value from the call to addListener.

useEffect(() => {
  return navigation.addListener("focus", () => {
    clearErrorMessage();
  });
}, [navigation]);

The same is true in src/screens/SignupScreen.js.

In src/context/DataContext.js object destructuring could be used to eliminate single-use variables - e.g. in the first promise callback following signInWithEmailAndPassword()

.then((userCredential) => {
       // Signed in
       const user = userCredential.user;
       dispatch({ type: "signin", payload: user });
       navigate("Index");
     })

The parameter userCredential could be destructured like this to eliminate const user = userCredential.user;:

.then(({ user }) => {
    // Signed in
    dispatch({ type: "signin", payload: user });
    navigate("Index");
  })

Similarly in the catch callback

.catch((error) => {
       const errorCode = error.code;
       console.log(errorCode);
       if (errorCode === "auth/wrong-password") {
         dispatch({
           type: "add_error",
           payload: "Incorrect Password",
         });
       } else if (errorCode === "auth/invalid-email") {
         dispatch({
           type: "add_error",
           payload: "Invalid Email",
         });
       } else {
         dispatch({
           type: "add_error",
           payload: error.code,
         });
       }

The object returned to the call to dispatch() could be pulled out since the only thing that changes is the payload:

.catch(({ code. }) => {
  let payload = code;
  if (code === "auth/wrong-password") {
    payload = "Incorrect Password";
  } else if (code === "auth/invalid-email") {
    payload = "Invalid email";
  }
  dispatch({
    type: "add_error",
    payload,
  });
}

Simplifying promises

Also in In src/context/DataContext.js the signup function uses traditional promise format, yet the callback to createUserWithEmailAndPassword() uses async / await. While it isn't really the case that it is an example of callbackhell not use async / await to simplify the code? For example, the signup method could be simplified from:

const signup =
  (dispatch) =>
  ({ email, password }) => {
    createUserWithEmailAndPassword(auth, email, password)
      .then(async (userCredential) => {
        // Signed in
        const user = userCredential.user;

        await addDoc(collection(db, "users"), {
          worked: true,
        });

        dispatch({ type: "signup", payload: user });
      })
      .catch((error) => {
        const code = error.code;
        if (code === "auth/email-already-in-use") {
          dispatch({
            type: "add_error",
            payload: "Email already in use",
          });
        } else if (code === "auth/invalid-email") {
          dispatch({
            type: "add_error",
            payload: "Invalid email",
          });
        } else if (code === "auth/weak-password") {
          dispatch({
            type: "add_error",
            payload: "Weak password",
          });
        } else {
          dispatch({
            type: "add_error",
            payload: error.code,
          });
        }
      });
  };

To the following, using try .. catch (as well as simplifying the catch callback with suggestions from above:

const signup =
  (dispatch) =>
  async ({ email, password }) => {
    console.log('signing up ', email);
    try {
      const userCredential = await createUserWithEmailAndPassword(auth, email, password)

      // Signed in
      const user = userCredential.user;

      await addDoc(collection(db, "users"), {
        worked: true,
      });

      dispatch({ type: "signup", payload: user });
    }
    catch({ code }) {
      let payload = code;
      if (code === "auth/email-already-in-use") {
        payload = "Email already in use";
      } else if (code === "auth/invalid-email") {
        payload = "Invalid email";
      } else if (code === "auth/weak-password") {
        payload = "Weak password";
      }
      dispatch({
        type: "add_error",
        payload,
      });
    }
  };

Looping with for...in

In src/context/createDataContext.js the Provider definition has a for...in loop.

const boundActions = {};
for (let key in actions) {
  boundActions[key] = actions[key](dispatch);
}

While it works, MDN considers for...in deprecated and recommends using for...of instead. The method Object.entries() could be used in conjunction with a for...of loop:

const boundActions = {};
for (const [key, action] of Object.entries(actions)) {
  boundActions[key] = action(dispatch);
}

Avoid excess wrapper functions

There are a couple places where a listener is bound to the focus event, and a callback simply calls clearErrorMessage().

const unsubscribe = navigation.addListener("focus", () => {
      clearErrorMessage();
    });

As long as the callback isn't passed arguments that the function is not expecting, then the extra lambda/anonymous function can be removed:

const unsubscribe = navigation.addListener("focus", clearErrorMessage);
\$\endgroup\$
0

Not the answer you're looking for? Browse other questions tagged or ask your own question.