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:
The switch statement, is there a way to rewrite it and make it shorter probably without the SWITCH?
The state implementation, I am not quite convinced yet about it.
-
\$\begingroup\$ Far too much code for anyone to even bother looking at this to try and understand it. \$\endgroup\$Martin Dawson– Martin Dawson2017年10月17日 20:53:11 +00:00Commented Oct 17, 2017 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– 200_success2017年10月19日 23:06:58 +00:00Commented Oct 19, 2017 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– Martin Dawson2017年10月20日 07:36:49 +00:00Commented Oct 20, 2017 at 7:36
1 Answer 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; }) }
Promises should be used for async operations. It makes no sense for
getItems
to returnIListItem[]
if it has to make a network request to get the items.Consider defining
IDetailsListItemState
as a generic interface. By doing this you can avoid defining aListItemState
for every type ofListItem
.export interface IListItemState<Item extends IListItem> { columns: IColumn[]; items: Item[]; }
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 acceptsitems
andcolumns
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> ) }
Explore related questions
See similar questions with these tags.