1
\$\begingroup\$

I have the following code, but first the explanation. I am using a REACTJS Office UI Component called details list: https://developer.microsoft.com/en-us/fabric#/components/detailslist

And I want my application to be able to render information from any type of Sharepoint List regardless of the columns the source has. For that I am trying to implement a factory method design pattern like this:

export  interface IListItem {
    [key: string]: any;
    id: string;
    title: string;
    modified: Date;
    created: Date;
    modifiedby: string;
    createdby: string;    
}

import {IListItem} from './IListItem';

export interface  IAnnouncementListItem extends IListItem {
    announcementBody: string;
    expiryDate: Date;  
}

import {IListItem} from './IListItem';

export interface IDirectoryListItem extends IListItem {
        firstName: string;
        lastName: string;
        mobileNumber: string;
        internalNumber: string;  
}


import {IListItem} from './IListItem';

export interface  INewsListItem extends IListItem {
    newsheader: string;
    newsbody: string;
    expiryDate: Date;
}

import { IListItem } from './models/IListItem';
import { SPHttpClient, SPHttpClientResponse } from '@microsoft/sp-http';
export  interface IFactory{
    getItems(requester: SPHttpClient, siteUrl: string, listName: string): IListItem[];
}

import { SPHttpClient, SPHttpClientResponse } from '@microsoft/sp-http';
import { IWebPartContext } from '@microsoft/sp-webpart-base';
import { IListItem} from './models/IListItem';
import { IFactory } from './IFactory';
import { INewsListItem } from './models/INewsListItem';
import { IDirectoryListItem } from './models/IDirectoryListItem';
import { IAnnouncementListItem } from './models/IAnnouncementListItem';

export class ListItemFactory implements IFactory{   
    getItems(requester: SPHttpClient, siteUrl: string, listName: string): IListItem[] {
        switch(listName) {
            case 'List':
                let items: IListItem[];
                requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id`,
                SPHttpClient.configurations.v1,
                {
                    headers: {
                    'Accept': 'application/json;odata=nometadata',
                    'odata-version': ''
                    }
                })
                .then((response: SPHttpClientResponse): Promise<{ value: IListItem[] }> => {
                    return response.json();
                })
                .then((response: { value: IListItem[] }): void => {
                    items= response.value;
                });
                return items;
            case 'News':
                let newsitems: INewsListItem[];
                requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id`,
                SPHttpClient.configurations.v1,
                {
                    headers: {
                    'Accept': 'application/json;odata=nometadata',
                    'odata-version': ''
                    }
                })
                .then((response: SPHttpClientResponse): Promise<{ value: INewsListItem[] }> => {
                    return response.json();
                })
                .then((response: { value: INewsListItem[] }): void => {
                    newsitems= response.value;
                });
                return newsitems;
            case 'Announcements':
                let announcementitems: IAnnouncementListItem[];
                requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id`,
                SPHttpClient.configurations.v1,
                {
                    headers: {
                    'Accept': 'application/json;odata=nometadata',
                    'odata-version': ''
                    }
                })
                .then((response: SPHttpClientResponse): Promise<{ value: IAnnouncementListItem[] }> => {
                    return response.json();
                })
                .then((response: { value: IAnnouncementListItem[] }): void => {
                    announcementitems= response.value;
                });
                return announcementitems;
            case 'Directory':
                let directoryitems: IDirectoryListItem[];
                requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id`,
                SPHttpClient.configurations.v1,
                {
                    headers: {
                    'Accept': 'application/json;odata=nometadata',
                    'odata-version': ''
                    }
                })
                .then((response: SPHttpClientResponse): Promise<{ value: IDirectoryListItem[] }> => {
                    return response.json();
                })
                .then((response: { value: IDirectoryListItem[] }): void => {
                    items= response.value;
                });
                return directoryitems;
            default:
                return null;
        }
      }
} 
    /* public getItems(requester: SPHttpClient, siteUrl: string, listName: string): IListItem[] {
        let items: IListItem[];
        requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id`,
        SPHttpClient.configurations.v1,
        {
            headers: {
            'Accept': 'application/json;odata=nometadata',
            'odata-version': ''
            }
        })
        .then((response: SPHttpClientResponse): Promise<{ value: IListItem[] }> => {
            return response.json();
        })
        .then((response: { value: IListItem[] }): void => {
            items= response.value;
        });
        return items;
    } */
}

The state class:

import { IListItem } from './models/IListItem';
import { INewsListItem } from './models/INewsListItem';
import { IDirectoryListItem } from './models/IDirectoryListItem';
import { IAnnouncementListItem } from './models/IAnnouncementListItem';
import {
  IColumn
} from 'office-ui-fabric-react/lib/DetailsList';

export interface IFactoryMethodState{
  type: string;
  status: string;
  DetailsListItemState: IDetailsListItemState;
  DetailsNewsListItemState: IDetailsNewsListItemState;
  DetailsDirectoryListItemState : IDetailsDirectoryListItemState;
  DetailsAnnouncementListItemState : IDetailsAnnouncementListItemState;
}

export interface IDetailsListItemState {
  columns: IColumn[];
  items: IListItem[];
}

export interface IDetailsNewsListItemState {
  columns: IColumn[];
  items: INewsListItem[];
}

export interface IDetailsDirectoryListItemState {
  columns: IColumn[];
  items: IDirectoryListItem[];
}

export interface IDetailsAnnouncementListItemState {
  columns: IColumn[];
  items: IAnnouncementListItem[];
}

as you can see, I have different types of results, so I encapsulated that info IfactoryMethodState

Now in the component I use it like this:

import * as React from 'react';
import styles from './FactoryMethod.module.scss';
import { IFactoryMethodProps } from './IFactoryMethodProps';
import { 
  IDetailsListItemState,  
  IDetailsNewsListItemState,
  IDetailsDirectoryListItemState,
  IDetailsAnnouncementListItemState,
  IFactoryMethodState
} from './IFactoryMethodState';
import { IListItem } from './models/IListItem';
import { IAnnouncementListItem } from './models/IAnnouncementListItem';
import { INewsListItem } from './models/INewsListItem';
import { IDirectoryListItem } from './models/IDirectoryListItem';

import { escape } from '@microsoft/sp-lodash-subset';
import { SPHttpClient, SPHttpClientResponse } from '@microsoft/sp-http';
import { ListItemFactory} from './ListItemFactory';
import { TextField } from 'office-ui-fabric-react/lib/TextField';
import {
  DetailsList,
  DetailsListLayoutMode,
  Selection,
  IColumn
} from 'office-ui-fabric-react/lib/DetailsList';
import { MarqueeSelection } from 'office-ui-fabric-react/lib/MarqueeSelection';
import { autobind } from 'office-ui-fabric-react/lib/Utilities';

let _columns = [
  {
    key: 'column1',
    name: 'Name',
    fieldName: 'name',
    minWidth: 100,
    maxWidth: 200,
    isResizable: true
  },
  {
    key: 'column2',
    name: 'Value',
    fieldName: 'value',
    minWidth: 100,
    maxWidth: 200,
    isResizable: true
  },
];

export default class FactoryMethod extends React.Component<any, IFactoryMethodState> {
  private listItemEntityTypeName: string = undefined;
  private _selection: Selection;

  constructor(props: IFactoryMethodProps, state: any) {
    super(props);

    //Initialize state
    this.state = {
      type: 'ListItem',
      status: this.listNotConfigured(this.props) ? 'Please configure list in Web Part properties' : 'Ready',
      DetailsListItemState:{
        columns:[],
        items:[]
      },
      DetailsNewsListItemState:{
        columns:[],
        items:[]
      },
      DetailsDirectoryListItemState:{
        columns:[],
        items:[]
      },
      DetailsAnnouncementListItemState:{
        columns:[],
        items:[]
      },
    };
  }

  public componentWillReceiveProps(nextProps: IFactoryMethodProps): void {
    this.listItemEntityTypeName = undefined;
    //Initialize state
    this.state = {
      type: 'ListItem',
      status: this.listNotConfigured(this.props) ? 'Please configure list in Web Part properties' : 'Ready',
      DetailsListItemState:{
        columns:[],
        items:[]
      },
      DetailsNewsListItemState:{
        columns:[],
        items:[]
      },
      DetailsDirectoryListItemState:{
        columns:[],
        items:[]
      },
      DetailsAnnouncementListItemState:{
        columns:[],
        items:[]
      },
    };
  }

  public render(): React.ReactElement<IFactoryMethodProps> {
    let { type, 
      status, 
      DetailsListItemState, 
      DetailsNewsListItemState, 
      DetailsDirectoryListItemState, 
      DetailsAnnouncementListItemState } = this.state;

        switch(this.props.listName)
        {
          case "List":
            return (
              <div>
                <MarqueeSelection selection={ this._selection }>
                  <DetailsList
                    items={ DetailsListItemState.items }
                    columns={ DetailsListItemState.columns }
                    setKey='set'
                    layoutMode={ DetailsListLayoutMode.fixedColumns }
                    selection={ this._selection }
                    selectionPreservedOnEmptyClick={ true }                
                    compact={ true }
                  />
                </MarqueeSelection>
              </div>
            );         
          case "Announcements":
            return (
              <div>
                <MarqueeSelection selection={ this._selection }>
                  <DetailsList
                    items={ DetailsAnnouncementListItemState.items }
                    columns={ DetailsAnnouncementListItemState.columns }
                    setKey='set'
                    layoutMode={ DetailsListLayoutMode.fixedColumns }
                    selection={ this._selection }
                    selectionPreservedOnEmptyClick={ true }                
                    compact={ true }
                  />
                </MarqueeSelection>
              </div>
            );
        case "News":
          return (
            <div>
              <MarqueeSelection selection={ this._selection }>
                <DetailsList
                  items={ DetailsNewsListItemState.items }
                  columns={ DetailsNewsListItemState.columns }
                  setKey='set'
                  layoutMode={ DetailsListLayoutMode.fixedColumns }
                  selection={ this._selection }
                  selectionPreservedOnEmptyClick={ true }                
                  compact={ true }
                />
              </MarqueeSelection>
            </div>
          );
        case "Directory":
          return (
            <div>
              <MarqueeSelection selection={ this._selection }>
                <DetailsList
                  items={ DetailsDirectoryListItemState.items }
                  columns={ DetailsDirectoryListItemState.columns }
                  setKey='set'
                  layoutMode={ DetailsListLayoutMode.fixedColumns }
                  selection={ this._selection }
                  selectionPreservedOnEmptyClick={ true }                
                  compact={ true }
                />
              </MarqueeSelection>
            </div>
          );
        default : 
          break;
      }
  }

  private readItems(): void {
    this.setState({
      status: 'Loading all items...'
    });
    let factory = new  ListItemFactory();

    //Here its where we actually use the pattern to make our coding easier.
    switch(this.props.listName)
    {
      case "List":
        let listItems  = factory.getItems(this.props.spHttpClient, this.props.siteUrl, this.props.listName);  
        this.setState({
          status: `Successfully loaded ${listItems.length} items`,
          DetailsListItemState : {
            items: listItems,
            columns: [
            ]
          }
        });      
        break;
      case "Announcements":
        let announcementlistItems  = factory.getItems(this.props.spHttpClient, this.props.siteUrl, this.props.listName) as IAnnouncementListItem[];  
        this.setState({
          status: `Successfully loaded ${listItems.length} items`,
          DetailsAnnouncementListItemState : {
            items: announcementlistItems,
            columns: []
          }
        });          
        break;
      case "News":
        let newsListItems  = factory.getItems(this.props.spHttpClient, this.props.siteUrl, this.props.listName) as INewsListItem[];  
        this.setState({
          status: `Successfully loaded ${listItems.length} items`,
          DetailsNewsListItemState : {
            items: newsListItems,
            columns: []
          }
        });      
        break;
      case "Directory":
        let directoryListItems  = factory.getItems(this.props.spHttpClient, this.props.siteUrl, this.props.listName) as IDirectoryListItem[];  
        this.setState({
          status: `Successfully loaded ${listItems.length} items`,
          DetailsDirectoryListItemState : {
            items: directoryListItems,
            columns: []
          }
        });   
        break;
      default : 
        break;
    }    
  }

  private _getSelectionDetails(): string {
    let selectionCount = this._selection.getSelectedCount();

    switch (selectionCount) {
      case 0:
        return 'No items selected';
      case 1:
        return '1 item selected: ' + (this._selection.getSelection()[0] as any).name;
      default:
        return `${selectionCount} items selected`;
    }
  }

  private listNotConfigured(props: IFactoryMethodProps): boolean {
    return props.listName === undefined ||
      props.listName === null ||
      props.listName.length === 0;
  }


}

What I dont like about this code:

  1. The switch statement, is there a way to rewrite it and make it shorter probably without the SWITCH?

  2. The state implementation, I am not quite convinced yet about it.

\$\endgroup\$
  • \$\begingroup\$ Far too much code for anyone to even bother looking at this to try and understand it. \$\endgroup\$ – Martin Dawson Oct 17 '17 at 20:53
  • \$\begingroup\$ @MartinDawson You're welcome to ignore questions that don't interest you. On the other hand, users are welcome to post as much code as they feel is necessary, up to the 64 kiB limit. \$\endgroup\$ – 200_success Oct 19 '17 at 23:06
  • \$\begingroup\$ @200_success Or I could tell OP instead that the vast majority of users will ignore his question because their is too much code or ignore it like you are doing and he will get no answers probably. You are more than welcome to ignore my comment. \$\endgroup\$ – Martin Dawson Oct 20 '17 at 7:36
1
\$\begingroup\$
  1. Avoid code duplication. This is the single largest problem with your proposed code.

    Take the ListItemFactory::getItems method for an example. The only difference between your case statements is the types which you are asserting to return. This method could be expressed much more simply with the following:

    getItems(requester: SPHttpClient, siteUrl: string, listName: string): Promise<IListItem[]> {
        if (!['List', 'News', 'Announcements', 'Directory'].includes(listName)) {
            // I personally avoid returning null whenever possible.
            // an empty array is safer.
            return Promise.resolve([]);
        }
    
        return requester.get(
            `${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id`,
            SPHttpClient.configurations.v1,
            {
                headers: {
                    'Accept': 'application/json;odata=nometadata',
                    'odata-version': ''
                }
            }
        ).then((response: SPHttpClientResponse): Promise<{ value: IListItem[] }> => {
            return response.json();
        }).then((response: { value: IListItem[] }) => {
            return response.value;
        })
    }
    
  2. Promises should be used for async operations. It makes no sense for getItems to return IListItem[] if it has to make a network request to get the items.

  3. Consider defining IDetailsListItemState as a generic interface. By doing this you can avoid defining a ListItemState for every type of ListItem.

    export interface IListItemState<Item extends IListItem> {
      columns: IColumn[];
      items: Item[];
    }
    
  4. Again, avoid unnecessary duplication. I strongly recommend refactoring your React class not handle multiple behavior types depending on the value of this.props.listName. By doing this you make the code incredibly hard to read. I recommend using a pure component which accepts items and columns properties and rendering that instead. This could look something like the following (untested):

    function MarqueeHelper<T>(props: { selection: Selection, items: IListItem, columns: IListItemState<T> }): React.ReactElement<IFactoryMethodProps> {
        let { selection, items, columns } = props
        return (
            <MarqueeSelection selection={selection}>
                <DetailsList
                    items={items}
                    columns={columns}
                    setKey='set'
                    layoutMode={DetailsListLayoutMode.fixedColumns}
                    selection={selection}
                    selectionPreservedOnEmptyClick={true}
                    compact={true}
                />
            </MarqueeSelection>
        )
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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