How to refactor this code to remove duplication?

Question

There I have 2 methods(create and update) that push data to api. actually i need to optimize createUser and updateUser methods cuz is similar for 99%.

also maybe you have any idea how better to set directly nullable id property from router & check it to distinct edit from creating client, without using this.isAddMode

export class ClientFormComponent implements OnInit {
  registerForm: FormGroup = new FormGroup({});
  isAddMode = false;
  constructor(
    private formBuilder: FormBuilder,
    private snackBar: MatSnackBar,
    private route: ActivatedRoute,
    private router: Router,
    private clientService: ClientService
  ) {}

  ngOnInit(): void {
    console.log(this.route.snapshot.params["id"]);
    this.isAddMode = !this.route.snapshot.params["id"];
    console.log(this.isAddMode);

    //  [...]

    if (!this.isAddMode) {
      this.clientService
        .getById(this.route.snapshot.params["id"])
        .pipe(first())
        .subscribe((x) => this.registerForm.patchValue(x));
    }
  }

  onSubmit(): void {
    // stop here if form is invalid
    if (this.registerForm.invalid) {
      return this.openSnackBar("Formulaire invalide");
    }
    console.log(this.registerForm.value);
    if (this.isAddMode) {
      this.createUser();
    } else {
      this.updateUser();
    }
  }

  // [...]

  private createUser() {
    const data = {
      name: this.registerForm.value.name,
      organization: this.registerForm.value.organization,
      location: {
        address: this.registerForm.value.address,
        zipCode: this.registerForm.value.zipCode,
        city: this.registerForm.value.city,
      },
      contacts: {
        phone: "+33" + this.registerForm.value.phone,
        email: this.registerForm.value.email,
        site: this.registerForm.value.site,
      },
    };
    this.clientService
      .create(data)
      .pipe(first())
      .subscribe({
        next: () => {
          this.openSnackBar("Successfully added");
          this.router.navigate([""], {
            relativeTo: this.route,
          });
        },
        error: () => {
          this.openSnackBar("Formulaire invalide");
        },
      });
  }

  private updateUser() {
    const data = {
      name: this.registerForm.value.name,
      organization: this.registerForm.value.organization,
      location: {
        address: this.registerForm.value.address,
        zipCode: this.registerForm.value.zipCode,
        city: this.registerForm.value.city,
      },
      contacts: {
        phone: "+33" + this.registerForm.value.phone,
        email: this.registerForm.value.email,
        site: this.registerForm.value.site,
      },
    };
    this.clientService
      .update(this.route.snapshot.params["id"], data)
      .pipe(first())
      .subscribe({
        next: () => {
          this.openSnackBar("Successfully updated");
          this.router.navigate([""], {
            relativeTo: this.route,
          });
        },
        error: () => {
          this.openSnackBar("Formulaire invalide");
        },
      });
  }
}

by "optimizing" you actually mean "refactor" – Random Jul 04 '22 at 14:22 — Random, Jul 04 '22 at 14:22

score 1 · Answer 1 · answered Jul 04 '22 at 14:22

1

Yeah. You can optimize it. You can remove the addUser and updateUser methods on a separate service class and call that service function from here.

answered Jul 04 '22 at 14:22

Abdus Salam Azad

5,087
46
35

score 0 · Answer 2 · answered Jul 04 '22 at 14:39

You can refactor this way.
Gathering form info can be extracted, and the pipe/subscribe after the httpCall too:

  private createUser() {
    const data = this.getDataFromForm();
    const httpCall$ = this.clientService
                          .create(data)
    this.subscribeToCreateUpdate(httpCall$, true);
  }

  private updateUser() {
    const data = this.getDataFromForm();
    const httpCall$ = this.clientService
                          .update(this.route.snapshot.params["id"], data);
    this.subscribeToCreateUpdate(httpCall$, false);
  }
  
  private getDataFromForm() {
    return {
      name: this.registerForm.value.name,
      organization: this.registerForm.value.organization,
      location: {
        address: this.registerForm.value.address,
        zipCode: this.registerForm.value.zipCode,
        city: this.registerForm.value.city,
      },
      contacts: {
        phone: "+33" + this.registerForm.value.phone,
        email: this.registerForm.value.email,
        site: this.registerForm.value.site,
      },
    };
  }
  
  private subscribeToCreateUpdate(httpCall: Observable<any>, isCreate: boolean) {
    httpCall
      .pipe(first()) // unnecessary, http calls always happen only once
      .subscribe({
        next: () => {
          const msg = "Successfully" + (isCreate ? "updated" : "updated")
          this.openSnackBar(msg);
          this.router.navigate([""], {
            relativeTo: this.route,
          });
        },
        error: () => {
          this.openSnackBar("Formulaire invalide");
        },
      });
  }

score 0 · Answer 3 · answered Jul 04 '22 at 14:58

0

Your component should stay for UI only, so move the logic into service layer. It is pretty good and testable

answered Jul 04 '22 at 14:58

standbyoneself

135
6

How to refactor this code to remove duplication?

3 Answers3