Factory method done on reactjs component,











up vote
5
down vote

favorite












A Factory Pattern or Factory Method Pattern says that just define an interface or abstract class for creating an object but let the subclasses decide which class to instantiate. In other words, subclasses are responsible to create the instance of the class.



Advantage of Factory Design Pattern
Factory Method Pattern allows the sub-classes to choose the type of objects to create. It promotes the loose-coupling by eliminating the need to bind application-specific classes into the code. That means the code interacts solely with the resultant interface or abstract class, so that it will work with any classes that implement that interface or that extends that abstract class.



A factory method is a method of a factory that builds objects



With that said, I built a React component that uses this pattern, functionally it works perfect, but I would like to know if the pattern is properly implemented or not and what your feedback as an expert is on this scenario.



enter image description here



Relevant source code:



ListItemFactory.ts



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 {
// private _listItems: IListItem;
public getItems(requester: SPHttpClient, siteUrl: string, listName: string): Promise<any> {
switch(listName) {
case "GenericList":
let items: IListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Author/Title,Editor/Title&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IListItem }> => {
return response.json();
})
.then((json: { value: IListItem }) => {
console.log(JSON.stringify(json.value));
return items=json.value.map((v,i)=>(
{
// key: v.id,
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title
}
));
});
case "News":
let newsitems: INewsListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Created By,Modified By,newsheader,newsbody,expiryDate`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: INewsListItem }> => {
return response.json();
})
.then((json: { value: INewsListItem }) => {
return newsitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
newsheader: v.newsheader,
newsbody: v.newsbody,
expiryDate: v.expiryDate
}
));
});
case "Announcements":
let announcementitems: IAnnouncementListItem;
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Created,Author/Title,Modified,Editor/Title,announcementBody,expiryDate&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IAnnouncementListItem }> => {
return response.json();
})
.then((json: { value: IAnnouncementListItem }) => {
return announcementitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
announcementBody: v.announcementBody,
expiryDate: v.expiryDate
}
));
});
case "Directory":
let directoryitems: IDirectoryListItem;
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: IDirectoryListItem }> => {
return response.json();
})
.then((json: { value: IDirectoryListItem }) => {
return directoryitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
firstName: v.firstName,
lastName: v.lastName,
mobileNumber: v.mobileNumber,
internalNumber: v.internalNumber
}
));
});
default:
break;
}
}
}


Ifactorystate.ts



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 {
hasError: boolean;
status: string;
columns: IColumn;
DetailsListItemState: IDetailsListItemState;
DetailsNewsListItemState: IDetailsNewsListItemState;
DetailsDirectoryListItemState : IDetailsDirectoryListItemState;
DetailsAnnouncementsListItemState : IDetailsAnnouncementListItemState;
}

export interface IDetailsListItemState {
items: IListItem;
}

export interface IDetailsNewsListItemState {
items: INewsListItem;
}

export interface IDetailsDirectoryListItemState {
items: IDirectoryListItem;
}

export interface IDetailsAnnouncementListItemState {
items: IAnnouncementListItem;
}


and the component



//#region Imports
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,
buildColumns,
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";
import PropTypes from "prop-types";
//#endregion

export default class FactoryMethod extends React.Component<IFactoryMethodProps, IFactoryMethodState> {
constructor(props: IFactoryMethodProps, state: any) {
super(props);
this.setInitialState();
}


// lifecycle help here: https://staminaloops.github.io/undefinedisnotafunction/understanding-react/
//#region Mouting events lifecycle
// the data returned from render is neither a string nor a DOM node.
// it's a lightweight description of what the DOM should look like.
// inspects this.state and this.props and create the markup.
// when your data changes, the render method is called again.
// react diff the return value from the previous call to render with
// the new one, and generate a minimal set of changes to be applied to the DOM.
public render(): React.ReactElement<IFactoryMethodProps> {
if (this.state.hasError) {
// you can render any custom fallback UI
return <h1>Something went wrong.</h1>;
} else {
switch(this.props.listName) {
case "GenericList":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsListItemState.items} columns={this.state.columns} />;
case "News":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsNewsListItemState.items} columns={this.state.columns}/>;
case "Announcements":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsAnnouncementsListItemState.items} columns={this.state.columns}/>;
case "Directory":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsDirectoryListItemState.items} columns={this.state.columns}/>;
default:
return null;
}
}
}

public componentDidCatch(error: any, info: any): void {
// display fallback UI
this.setState({ hasError: true });
// you can also log the error to an error reporting service
console.log(error);
console.log(info);
}



// componentDidMount() is invoked immediately after a component is mounted. Initialization that requires DOM nodes should go here.
// if you need to load data from a remote endpoint, this is a good place to instantiate the network request.
// this method is a good place to set up any subscriptions. If you do that, don’t forget to unsubscribe in componentWillUnmount().
// calling setState() in this method will trigger an extra rendering, but it is guaranteed to flush during the same tick.
// this guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state.
// use this pattern with caution because it often causes performance issues. It can, however, be necessary for cases like modals and
// tooltips when you need to measure a DOM node before rendering something that depends on its size or position.
public componentDidMount(): void {
this._configureWebPart = this._configureWebPart.bind(this);
this.readItemsAndSetStatus(this.props.listName);
}

//#endregion
//#region Props changes lifecycle events (after a property changes from parent component)
// componentWillReceiveProps() is invoked before a mounted component receives new props.
// if you need to update the state in response to prop
// changes (for example, to reset it), you may compare this.props and nextProps and perform state transitions
// using this.setState() in this method.
// note that React may call this method even if the props have not changed, so make sure to compare the current
// and next values if you only want to handle changes.
// this may occur when the parent component causes your component to re-render.
// react doesn’t call componentWillReceiveProps() with initial props during mounting. It only calls this
// method if some of component’s props may update
// calling this.setState() generally doesn’t trigger componentWillReceiveProps()
public componentWillReceiveProps(nextProps: IFactoryMethodProps): void {
if(nextProps.listName !== this.props.listName) {
this.readItemsAndSetStatus(nextProps.listName);
}
}

//#endregion
//#region private methods
private _configureWebPart(): void {
this.props.configureStartCallback();
}

public setInitialState(): void {
this.state = {
hasError: false,
status: this.listNotConfigured(this.props)
? "Please configure list in Web Part properties"
: "Ready",
columns:,
DetailsListItemState:{
items:
},
DetailsNewsListItemState:{
items:
},
DetailsDirectoryListItemState:{
items:
},
DetailsAnnouncementsListItemState:{
items:
},
};
}

// reusable inline component
private ListMarqueeSelection = (itemState: {columns: IColumn, items: IListItem }) => (
<div>
<DetailsList
items={ itemState.items }
columns={ itemState.columns }
setKey="set"
layoutMode={ DetailsListLayoutMode.fixedColumns }
selectionPreservedOnEmptyClick={ true }
compact={ true }>
</DetailsList>
</div>
)

// read items using factory method pattern and sets state accordingly
private readItemsAndSetStatus(listName): void {
this.setState({
status: "Loading all items..."
});

const factory: ListItemFactory = new ListItemFactory();
factory.getItems(this.props.spHttpClient, this.props.siteUrl, listName || this.props.listName)
.then((items: any) => {

var myItems: any = null;
switch(this.props.listName) {
case "GenericList":
items = items as IListItem;
break;
case "News":
items = items as INewsListItem;
break;
case "Announcements":
items = items as IAnnouncementListItem;
break;
case "Directory":
items = items as IDirectoryListItem;
break;
}

const keyPart: string = this.props.listName === "GenericList" ? "" : this.props.listName;
// the explicit specification of the type argument `keyof {}` is bad and
// it should not be required.
this.setState<keyof {}>({
status: `Successfully loaded ${items.length} items`,
["Details" + keyPart + "ListItemState"] : {
items
},
columns: buildColumns(items)
});
});
}

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

//#endregion
}









share|improve this question














bumped to the homepage by Community 28 mins ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.






migrated from stackoverflow.com May 27 at 12:48


This question came from our site for professional and enthusiast programmers.















  • the getItems method in the factory, depending on the selected list (returns/instantiates) an array of the type needed, the caller code does not have to worry about this. Thats my thinking but I could be wrong of course
    – Luis Valencia
    May 24 at 9:47










  • This isn't factory method. ListItemFactory isn't subclassed. It makes a request, not creates an instance. That you assign request result to item isn't considered 'class instance'. This is just a method with a big pile of code and some smell. It should be splitted into several different methods (or possibly classes - if you can make use of them). You're doing switch(this.props.listName) any way, you could call respective methods there. I guess the examples are self-explanatory, en.wikipedia.org/wiki/Factory_method_pattern#PHP , they have nothing to do with your case.
    – estus
    May 24 at 10:01















up vote
5
down vote

favorite












A Factory Pattern or Factory Method Pattern says that just define an interface or abstract class for creating an object but let the subclasses decide which class to instantiate. In other words, subclasses are responsible to create the instance of the class.



Advantage of Factory Design Pattern
Factory Method Pattern allows the sub-classes to choose the type of objects to create. It promotes the loose-coupling by eliminating the need to bind application-specific classes into the code. That means the code interacts solely with the resultant interface or abstract class, so that it will work with any classes that implement that interface or that extends that abstract class.



A factory method is a method of a factory that builds objects



With that said, I built a React component that uses this pattern, functionally it works perfect, but I would like to know if the pattern is properly implemented or not and what your feedback as an expert is on this scenario.



enter image description here



Relevant source code:



ListItemFactory.ts



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 {
// private _listItems: IListItem;
public getItems(requester: SPHttpClient, siteUrl: string, listName: string): Promise<any> {
switch(listName) {
case "GenericList":
let items: IListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Author/Title,Editor/Title&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IListItem }> => {
return response.json();
})
.then((json: { value: IListItem }) => {
console.log(JSON.stringify(json.value));
return items=json.value.map((v,i)=>(
{
// key: v.id,
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title
}
));
});
case "News":
let newsitems: INewsListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Created By,Modified By,newsheader,newsbody,expiryDate`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: INewsListItem }> => {
return response.json();
})
.then((json: { value: INewsListItem }) => {
return newsitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
newsheader: v.newsheader,
newsbody: v.newsbody,
expiryDate: v.expiryDate
}
));
});
case "Announcements":
let announcementitems: IAnnouncementListItem;
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Created,Author/Title,Modified,Editor/Title,announcementBody,expiryDate&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IAnnouncementListItem }> => {
return response.json();
})
.then((json: { value: IAnnouncementListItem }) => {
return announcementitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
announcementBody: v.announcementBody,
expiryDate: v.expiryDate
}
));
});
case "Directory":
let directoryitems: IDirectoryListItem;
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: IDirectoryListItem }> => {
return response.json();
})
.then((json: { value: IDirectoryListItem }) => {
return directoryitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
firstName: v.firstName,
lastName: v.lastName,
mobileNumber: v.mobileNumber,
internalNumber: v.internalNumber
}
));
});
default:
break;
}
}
}


Ifactorystate.ts



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 {
hasError: boolean;
status: string;
columns: IColumn;
DetailsListItemState: IDetailsListItemState;
DetailsNewsListItemState: IDetailsNewsListItemState;
DetailsDirectoryListItemState : IDetailsDirectoryListItemState;
DetailsAnnouncementsListItemState : IDetailsAnnouncementListItemState;
}

export interface IDetailsListItemState {
items: IListItem;
}

export interface IDetailsNewsListItemState {
items: INewsListItem;
}

export interface IDetailsDirectoryListItemState {
items: IDirectoryListItem;
}

export interface IDetailsAnnouncementListItemState {
items: IAnnouncementListItem;
}


and the component



//#region Imports
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,
buildColumns,
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";
import PropTypes from "prop-types";
//#endregion

export default class FactoryMethod extends React.Component<IFactoryMethodProps, IFactoryMethodState> {
constructor(props: IFactoryMethodProps, state: any) {
super(props);
this.setInitialState();
}


// lifecycle help here: https://staminaloops.github.io/undefinedisnotafunction/understanding-react/
//#region Mouting events lifecycle
// the data returned from render is neither a string nor a DOM node.
// it's a lightweight description of what the DOM should look like.
// inspects this.state and this.props and create the markup.
// when your data changes, the render method is called again.
// react diff the return value from the previous call to render with
// the new one, and generate a minimal set of changes to be applied to the DOM.
public render(): React.ReactElement<IFactoryMethodProps> {
if (this.state.hasError) {
// you can render any custom fallback UI
return <h1>Something went wrong.</h1>;
} else {
switch(this.props.listName) {
case "GenericList":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsListItemState.items} columns={this.state.columns} />;
case "News":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsNewsListItemState.items} columns={this.state.columns}/>;
case "Announcements":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsAnnouncementsListItemState.items} columns={this.state.columns}/>;
case "Directory":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsDirectoryListItemState.items} columns={this.state.columns}/>;
default:
return null;
}
}
}

public componentDidCatch(error: any, info: any): void {
// display fallback UI
this.setState({ hasError: true });
// you can also log the error to an error reporting service
console.log(error);
console.log(info);
}



// componentDidMount() is invoked immediately after a component is mounted. Initialization that requires DOM nodes should go here.
// if you need to load data from a remote endpoint, this is a good place to instantiate the network request.
// this method is a good place to set up any subscriptions. If you do that, don’t forget to unsubscribe in componentWillUnmount().
// calling setState() in this method will trigger an extra rendering, but it is guaranteed to flush during the same tick.
// this guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state.
// use this pattern with caution because it often causes performance issues. It can, however, be necessary for cases like modals and
// tooltips when you need to measure a DOM node before rendering something that depends on its size or position.
public componentDidMount(): void {
this._configureWebPart = this._configureWebPart.bind(this);
this.readItemsAndSetStatus(this.props.listName);
}

//#endregion
//#region Props changes lifecycle events (after a property changes from parent component)
// componentWillReceiveProps() is invoked before a mounted component receives new props.
// if you need to update the state in response to prop
// changes (for example, to reset it), you may compare this.props and nextProps and perform state transitions
// using this.setState() in this method.
// note that React may call this method even if the props have not changed, so make sure to compare the current
// and next values if you only want to handle changes.
// this may occur when the parent component causes your component to re-render.
// react doesn’t call componentWillReceiveProps() with initial props during mounting. It only calls this
// method if some of component’s props may update
// calling this.setState() generally doesn’t trigger componentWillReceiveProps()
public componentWillReceiveProps(nextProps: IFactoryMethodProps): void {
if(nextProps.listName !== this.props.listName) {
this.readItemsAndSetStatus(nextProps.listName);
}
}

//#endregion
//#region private methods
private _configureWebPart(): void {
this.props.configureStartCallback();
}

public setInitialState(): void {
this.state = {
hasError: false,
status: this.listNotConfigured(this.props)
? "Please configure list in Web Part properties"
: "Ready",
columns:,
DetailsListItemState:{
items:
},
DetailsNewsListItemState:{
items:
},
DetailsDirectoryListItemState:{
items:
},
DetailsAnnouncementsListItemState:{
items:
},
};
}

// reusable inline component
private ListMarqueeSelection = (itemState: {columns: IColumn, items: IListItem }) => (
<div>
<DetailsList
items={ itemState.items }
columns={ itemState.columns }
setKey="set"
layoutMode={ DetailsListLayoutMode.fixedColumns }
selectionPreservedOnEmptyClick={ true }
compact={ true }>
</DetailsList>
</div>
)

// read items using factory method pattern and sets state accordingly
private readItemsAndSetStatus(listName): void {
this.setState({
status: "Loading all items..."
});

const factory: ListItemFactory = new ListItemFactory();
factory.getItems(this.props.spHttpClient, this.props.siteUrl, listName || this.props.listName)
.then((items: any) => {

var myItems: any = null;
switch(this.props.listName) {
case "GenericList":
items = items as IListItem;
break;
case "News":
items = items as INewsListItem;
break;
case "Announcements":
items = items as IAnnouncementListItem;
break;
case "Directory":
items = items as IDirectoryListItem;
break;
}

const keyPart: string = this.props.listName === "GenericList" ? "" : this.props.listName;
// the explicit specification of the type argument `keyof {}` is bad and
// it should not be required.
this.setState<keyof {}>({
status: `Successfully loaded ${items.length} items`,
["Details" + keyPart + "ListItemState"] : {
items
},
columns: buildColumns(items)
});
});
}

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

//#endregion
}









share|improve this question














bumped to the homepage by Community 28 mins ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.






migrated from stackoverflow.com May 27 at 12:48


This question came from our site for professional and enthusiast programmers.















  • the getItems method in the factory, depending on the selected list (returns/instantiates) an array of the type needed, the caller code does not have to worry about this. Thats my thinking but I could be wrong of course
    – Luis Valencia
    May 24 at 9:47










  • This isn't factory method. ListItemFactory isn't subclassed. It makes a request, not creates an instance. That you assign request result to item isn't considered 'class instance'. This is just a method with a big pile of code and some smell. It should be splitted into several different methods (or possibly classes - if you can make use of them). You're doing switch(this.props.listName) any way, you could call respective methods there. I guess the examples are self-explanatory, en.wikipedia.org/wiki/Factory_method_pattern#PHP , they have nothing to do with your case.
    – estus
    May 24 at 10:01













up vote
5
down vote

favorite









up vote
5
down vote

favorite











A Factory Pattern or Factory Method Pattern says that just define an interface or abstract class for creating an object but let the subclasses decide which class to instantiate. In other words, subclasses are responsible to create the instance of the class.



Advantage of Factory Design Pattern
Factory Method Pattern allows the sub-classes to choose the type of objects to create. It promotes the loose-coupling by eliminating the need to bind application-specific classes into the code. That means the code interacts solely with the resultant interface or abstract class, so that it will work with any classes that implement that interface or that extends that abstract class.



A factory method is a method of a factory that builds objects



With that said, I built a React component that uses this pattern, functionally it works perfect, but I would like to know if the pattern is properly implemented or not and what your feedback as an expert is on this scenario.



enter image description here



Relevant source code:



ListItemFactory.ts



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 {
// private _listItems: IListItem;
public getItems(requester: SPHttpClient, siteUrl: string, listName: string): Promise<any> {
switch(listName) {
case "GenericList":
let items: IListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Author/Title,Editor/Title&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IListItem }> => {
return response.json();
})
.then((json: { value: IListItem }) => {
console.log(JSON.stringify(json.value));
return items=json.value.map((v,i)=>(
{
// key: v.id,
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title
}
));
});
case "News":
let newsitems: INewsListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Created By,Modified By,newsheader,newsbody,expiryDate`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: INewsListItem }> => {
return response.json();
})
.then((json: { value: INewsListItem }) => {
return newsitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
newsheader: v.newsheader,
newsbody: v.newsbody,
expiryDate: v.expiryDate
}
));
});
case "Announcements":
let announcementitems: IAnnouncementListItem;
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Created,Author/Title,Modified,Editor/Title,announcementBody,expiryDate&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IAnnouncementListItem }> => {
return response.json();
})
.then((json: { value: IAnnouncementListItem }) => {
return announcementitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
announcementBody: v.announcementBody,
expiryDate: v.expiryDate
}
));
});
case "Directory":
let directoryitems: IDirectoryListItem;
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: IDirectoryListItem }> => {
return response.json();
})
.then((json: { value: IDirectoryListItem }) => {
return directoryitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
firstName: v.firstName,
lastName: v.lastName,
mobileNumber: v.mobileNumber,
internalNumber: v.internalNumber
}
));
});
default:
break;
}
}
}


Ifactorystate.ts



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 {
hasError: boolean;
status: string;
columns: IColumn;
DetailsListItemState: IDetailsListItemState;
DetailsNewsListItemState: IDetailsNewsListItemState;
DetailsDirectoryListItemState : IDetailsDirectoryListItemState;
DetailsAnnouncementsListItemState : IDetailsAnnouncementListItemState;
}

export interface IDetailsListItemState {
items: IListItem;
}

export interface IDetailsNewsListItemState {
items: INewsListItem;
}

export interface IDetailsDirectoryListItemState {
items: IDirectoryListItem;
}

export interface IDetailsAnnouncementListItemState {
items: IAnnouncementListItem;
}


and the component



//#region Imports
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,
buildColumns,
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";
import PropTypes from "prop-types";
//#endregion

export default class FactoryMethod extends React.Component<IFactoryMethodProps, IFactoryMethodState> {
constructor(props: IFactoryMethodProps, state: any) {
super(props);
this.setInitialState();
}


// lifecycle help here: https://staminaloops.github.io/undefinedisnotafunction/understanding-react/
//#region Mouting events lifecycle
// the data returned from render is neither a string nor a DOM node.
// it's a lightweight description of what the DOM should look like.
// inspects this.state and this.props and create the markup.
// when your data changes, the render method is called again.
// react diff the return value from the previous call to render with
// the new one, and generate a minimal set of changes to be applied to the DOM.
public render(): React.ReactElement<IFactoryMethodProps> {
if (this.state.hasError) {
// you can render any custom fallback UI
return <h1>Something went wrong.</h1>;
} else {
switch(this.props.listName) {
case "GenericList":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsListItemState.items} columns={this.state.columns} />;
case "News":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsNewsListItemState.items} columns={this.state.columns}/>;
case "Announcements":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsAnnouncementsListItemState.items} columns={this.state.columns}/>;
case "Directory":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsDirectoryListItemState.items} columns={this.state.columns}/>;
default:
return null;
}
}
}

public componentDidCatch(error: any, info: any): void {
// display fallback UI
this.setState({ hasError: true });
// you can also log the error to an error reporting service
console.log(error);
console.log(info);
}



// componentDidMount() is invoked immediately after a component is mounted. Initialization that requires DOM nodes should go here.
// if you need to load data from a remote endpoint, this is a good place to instantiate the network request.
// this method is a good place to set up any subscriptions. If you do that, don’t forget to unsubscribe in componentWillUnmount().
// calling setState() in this method will trigger an extra rendering, but it is guaranteed to flush during the same tick.
// this guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state.
// use this pattern with caution because it often causes performance issues. It can, however, be necessary for cases like modals and
// tooltips when you need to measure a DOM node before rendering something that depends on its size or position.
public componentDidMount(): void {
this._configureWebPart = this._configureWebPart.bind(this);
this.readItemsAndSetStatus(this.props.listName);
}

//#endregion
//#region Props changes lifecycle events (after a property changes from parent component)
// componentWillReceiveProps() is invoked before a mounted component receives new props.
// if you need to update the state in response to prop
// changes (for example, to reset it), you may compare this.props and nextProps and perform state transitions
// using this.setState() in this method.
// note that React may call this method even if the props have not changed, so make sure to compare the current
// and next values if you only want to handle changes.
// this may occur when the parent component causes your component to re-render.
// react doesn’t call componentWillReceiveProps() with initial props during mounting. It only calls this
// method if some of component’s props may update
// calling this.setState() generally doesn’t trigger componentWillReceiveProps()
public componentWillReceiveProps(nextProps: IFactoryMethodProps): void {
if(nextProps.listName !== this.props.listName) {
this.readItemsAndSetStatus(nextProps.listName);
}
}

//#endregion
//#region private methods
private _configureWebPart(): void {
this.props.configureStartCallback();
}

public setInitialState(): void {
this.state = {
hasError: false,
status: this.listNotConfigured(this.props)
? "Please configure list in Web Part properties"
: "Ready",
columns:,
DetailsListItemState:{
items:
},
DetailsNewsListItemState:{
items:
},
DetailsDirectoryListItemState:{
items:
},
DetailsAnnouncementsListItemState:{
items:
},
};
}

// reusable inline component
private ListMarqueeSelection = (itemState: {columns: IColumn, items: IListItem }) => (
<div>
<DetailsList
items={ itemState.items }
columns={ itemState.columns }
setKey="set"
layoutMode={ DetailsListLayoutMode.fixedColumns }
selectionPreservedOnEmptyClick={ true }
compact={ true }>
</DetailsList>
</div>
)

// read items using factory method pattern and sets state accordingly
private readItemsAndSetStatus(listName): void {
this.setState({
status: "Loading all items..."
});

const factory: ListItemFactory = new ListItemFactory();
factory.getItems(this.props.spHttpClient, this.props.siteUrl, listName || this.props.listName)
.then((items: any) => {

var myItems: any = null;
switch(this.props.listName) {
case "GenericList":
items = items as IListItem;
break;
case "News":
items = items as INewsListItem;
break;
case "Announcements":
items = items as IAnnouncementListItem;
break;
case "Directory":
items = items as IDirectoryListItem;
break;
}

const keyPart: string = this.props.listName === "GenericList" ? "" : this.props.listName;
// the explicit specification of the type argument `keyof {}` is bad and
// it should not be required.
this.setState<keyof {}>({
status: `Successfully loaded ${items.length} items`,
["Details" + keyPart + "ListItemState"] : {
items
},
columns: buildColumns(items)
});
});
}

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

//#endregion
}









share|improve this question













A Factory Pattern or Factory Method Pattern says that just define an interface or abstract class for creating an object but let the subclasses decide which class to instantiate. In other words, subclasses are responsible to create the instance of the class.



Advantage of Factory Design Pattern
Factory Method Pattern allows the sub-classes to choose the type of objects to create. It promotes the loose-coupling by eliminating the need to bind application-specific classes into the code. That means the code interacts solely with the resultant interface or abstract class, so that it will work with any classes that implement that interface or that extends that abstract class.



A factory method is a method of a factory that builds objects



With that said, I built a React component that uses this pattern, functionally it works perfect, but I would like to know if the pattern is properly implemented or not and what your feedback as an expert is on this scenario.



enter image description here



Relevant source code:



ListItemFactory.ts



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 {
// private _listItems: IListItem;
public getItems(requester: SPHttpClient, siteUrl: string, listName: string): Promise<any> {
switch(listName) {
case "GenericList":
let items: IListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Author/Title,Editor/Title&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IListItem }> => {
return response.json();
})
.then((json: { value: IListItem }) => {
console.log(JSON.stringify(json.value));
return items=json.value.map((v,i)=>(
{
// key: v.id,
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title
}
));
});
case "News":
let newsitems: INewsListItem;
// tslint:disable-next-line:max-line-length
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Modified,Created,Created By,Modified By,newsheader,newsbody,expiryDate`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: INewsListItem }> => {
return response.json();
})
.then((json: { value: INewsListItem }) => {
return newsitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
newsheader: v.newsheader,
newsbody: v.newsbody,
expiryDate: v.expiryDate
}
));
});
case "Announcements":
let announcementitems: IAnnouncementListItem;
return requester.get(`${siteUrl}/_api/web/lists/getbytitle('${listName}')/items?$select=Title,Id,Created,Author/Title,Modified,Editor/Title,announcementBody,expiryDate&$expand=Author,Editor`,
SPHttpClient.configurations.v1,
{
headers: {
"Accept": "application/json;odata=nometadata",
"odata-version": ""
}
})
.then((response: SPHttpClientResponse): Promise<{ value: IAnnouncementListItem }> => {
return response.json();
})
.then((json: { value: IAnnouncementListItem }) => {
return announcementitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
announcementBody: v.announcementBody,
expiryDate: v.expiryDate
}
));
});
case "Directory":
let directoryitems: IDirectoryListItem;
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: IDirectoryListItem }> => {
return response.json();
})
.then((json: { value: IDirectoryListItem }) => {
return directoryitems=json.value.map((v,i)=>(
{
id: v.Id,
title: v.Title,
created: v.Created,
createdby: v.Author.Title,
modified: v.Modified,
modifiedby: v.Editor.Title,
firstName: v.firstName,
lastName: v.lastName,
mobileNumber: v.mobileNumber,
internalNumber: v.internalNumber
}
));
});
default:
break;
}
}
}


Ifactorystate.ts



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 {
hasError: boolean;
status: string;
columns: IColumn;
DetailsListItemState: IDetailsListItemState;
DetailsNewsListItemState: IDetailsNewsListItemState;
DetailsDirectoryListItemState : IDetailsDirectoryListItemState;
DetailsAnnouncementsListItemState : IDetailsAnnouncementListItemState;
}

export interface IDetailsListItemState {
items: IListItem;
}

export interface IDetailsNewsListItemState {
items: INewsListItem;
}

export interface IDetailsDirectoryListItemState {
items: IDirectoryListItem;
}

export interface IDetailsAnnouncementListItemState {
items: IAnnouncementListItem;
}


and the component



//#region Imports
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,
buildColumns,
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";
import PropTypes from "prop-types";
//#endregion

export default class FactoryMethod extends React.Component<IFactoryMethodProps, IFactoryMethodState> {
constructor(props: IFactoryMethodProps, state: any) {
super(props);
this.setInitialState();
}


// lifecycle help here: https://staminaloops.github.io/undefinedisnotafunction/understanding-react/
//#region Mouting events lifecycle
// the data returned from render is neither a string nor a DOM node.
// it's a lightweight description of what the DOM should look like.
// inspects this.state and this.props and create the markup.
// when your data changes, the render method is called again.
// react diff the return value from the previous call to render with
// the new one, and generate a minimal set of changes to be applied to the DOM.
public render(): React.ReactElement<IFactoryMethodProps> {
if (this.state.hasError) {
// you can render any custom fallback UI
return <h1>Something went wrong.</h1>;
} else {
switch(this.props.listName) {
case "GenericList":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsListItemState.items} columns={this.state.columns} />;
case "News":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsNewsListItemState.items} columns={this.state.columns}/>;
case "Announcements":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsAnnouncementsListItemState.items} columns={this.state.columns}/>;
case "Directory":
// tslint:disable-next-line:max-line-length
return <this.ListMarqueeSelection items={this.state.DetailsDirectoryListItemState.items} columns={this.state.columns}/>;
default:
return null;
}
}
}

public componentDidCatch(error: any, info: any): void {
// display fallback UI
this.setState({ hasError: true });
// you can also log the error to an error reporting service
console.log(error);
console.log(info);
}



// componentDidMount() is invoked immediately after a component is mounted. Initialization that requires DOM nodes should go here.
// if you need to load data from a remote endpoint, this is a good place to instantiate the network request.
// this method is a good place to set up any subscriptions. If you do that, don’t forget to unsubscribe in componentWillUnmount().
// calling setState() in this method will trigger an extra rendering, but it is guaranteed to flush during the same tick.
// this guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state.
// use this pattern with caution because it often causes performance issues. It can, however, be necessary for cases like modals and
// tooltips when you need to measure a DOM node before rendering something that depends on its size or position.
public componentDidMount(): void {
this._configureWebPart = this._configureWebPart.bind(this);
this.readItemsAndSetStatus(this.props.listName);
}

//#endregion
//#region Props changes lifecycle events (after a property changes from parent component)
// componentWillReceiveProps() is invoked before a mounted component receives new props.
// if you need to update the state in response to prop
// changes (for example, to reset it), you may compare this.props and nextProps and perform state transitions
// using this.setState() in this method.
// note that React may call this method even if the props have not changed, so make sure to compare the current
// and next values if you only want to handle changes.
// this may occur when the parent component causes your component to re-render.
// react doesn’t call componentWillReceiveProps() with initial props during mounting. It only calls this
// method if some of component’s props may update
// calling this.setState() generally doesn’t trigger componentWillReceiveProps()
public componentWillReceiveProps(nextProps: IFactoryMethodProps): void {
if(nextProps.listName !== this.props.listName) {
this.readItemsAndSetStatus(nextProps.listName);
}
}

//#endregion
//#region private methods
private _configureWebPart(): void {
this.props.configureStartCallback();
}

public setInitialState(): void {
this.state = {
hasError: false,
status: this.listNotConfigured(this.props)
? "Please configure list in Web Part properties"
: "Ready",
columns:,
DetailsListItemState:{
items:
},
DetailsNewsListItemState:{
items:
},
DetailsDirectoryListItemState:{
items:
},
DetailsAnnouncementsListItemState:{
items:
},
};
}

// reusable inline component
private ListMarqueeSelection = (itemState: {columns: IColumn, items: IListItem }) => (
<div>
<DetailsList
items={ itemState.items }
columns={ itemState.columns }
setKey="set"
layoutMode={ DetailsListLayoutMode.fixedColumns }
selectionPreservedOnEmptyClick={ true }
compact={ true }>
</DetailsList>
</div>
)

// read items using factory method pattern and sets state accordingly
private readItemsAndSetStatus(listName): void {
this.setState({
status: "Loading all items..."
});

const factory: ListItemFactory = new ListItemFactory();
factory.getItems(this.props.spHttpClient, this.props.siteUrl, listName || this.props.listName)
.then((items: any) => {

var myItems: any = null;
switch(this.props.listName) {
case "GenericList":
items = items as IListItem;
break;
case "News":
items = items as INewsListItem;
break;
case "Announcements":
items = items as IAnnouncementListItem;
break;
case "Directory":
items = items as IDirectoryListItem;
break;
}

const keyPart: string = this.props.listName === "GenericList" ? "" : this.props.listName;
// the explicit specification of the type argument `keyof {}` is bad and
// it should not be required.
this.setState<keyof {}>({
status: `Successfully loaded ${items.length} items`,
["Details" + keyPart + "ListItemState"] : {
items
},
columns: buildColumns(items)
});
});
}

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

//#endregion
}






javascript react.js typescript design-patterns






share|improve this question













share|improve this question











share|improve this question




share|improve this question










asked May 24 at 8:41









Luis Valencia

115112




115112





bumped to the homepage by Community 28 mins ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.







bumped to the homepage by Community 28 mins ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.






migrated from stackoverflow.com May 27 at 12:48


This question came from our site for professional and enthusiast programmers.






migrated from stackoverflow.com May 27 at 12:48


This question came from our site for professional and enthusiast programmers.














  • the getItems method in the factory, depending on the selected list (returns/instantiates) an array of the type needed, the caller code does not have to worry about this. Thats my thinking but I could be wrong of course
    – Luis Valencia
    May 24 at 9:47










  • This isn't factory method. ListItemFactory isn't subclassed. It makes a request, not creates an instance. That you assign request result to item isn't considered 'class instance'. This is just a method with a big pile of code and some smell. It should be splitted into several different methods (or possibly classes - if you can make use of them). You're doing switch(this.props.listName) any way, you could call respective methods there. I guess the examples are self-explanatory, en.wikipedia.org/wiki/Factory_method_pattern#PHP , they have nothing to do with your case.
    – estus
    May 24 at 10:01


















  • the getItems method in the factory, depending on the selected list (returns/instantiates) an array of the type needed, the caller code does not have to worry about this. Thats my thinking but I could be wrong of course
    – Luis Valencia
    May 24 at 9:47










  • This isn't factory method. ListItemFactory isn't subclassed. It makes a request, not creates an instance. That you assign request result to item isn't considered 'class instance'. This is just a method with a big pile of code and some smell. It should be splitted into several different methods (or possibly classes - if you can make use of them). You're doing switch(this.props.listName) any way, you could call respective methods there. I guess the examples are self-explanatory, en.wikipedia.org/wiki/Factory_method_pattern#PHP , they have nothing to do with your case.
    – estus
    May 24 at 10:01
















the getItems method in the factory, depending on the selected list (returns/instantiates) an array of the type needed, the caller code does not have to worry about this. Thats my thinking but I could be wrong of course
– Luis Valencia
May 24 at 9:47




the getItems method in the factory, depending on the selected list (returns/instantiates) an array of the type needed, the caller code does not have to worry about this. Thats my thinking but I could be wrong of course
– Luis Valencia
May 24 at 9:47












This isn't factory method. ListItemFactory isn't subclassed. It makes a request, not creates an instance. That you assign request result to item isn't considered 'class instance'. This is just a method with a big pile of code and some smell. It should be splitted into several different methods (or possibly classes - if you can make use of them). You're doing switch(this.props.listName) any way, you could call respective methods there. I guess the examples are self-explanatory, en.wikipedia.org/wiki/Factory_method_pattern#PHP , they have nothing to do with your case.
– estus
May 24 at 10:01




This isn't factory method. ListItemFactory isn't subclassed. It makes a request, not creates an instance. That you assign request result to item isn't considered 'class instance'. This is just a method with a big pile of code and some smell. It should be splitted into several different methods (or possibly classes - if you can make use of them). You're doing switch(this.props.listName) any way, you could call respective methods there. I guess the examples are self-explanatory, en.wikipedia.org/wiki/Factory_method_pattern#PHP , they have nothing to do with your case.
– estus
May 24 at 10:01










1 Answer
1






active

oldest

votes

















up vote
0
down vote













When I feel a design is overcomplicated (read: "big objects/functions"), I take a step back and think about the concerns. You have multiple objects and files, so which one should you start with? I always try to start from the top – the largest thing, the thing highest in the call stack, and/or the thing closest to the user – this approach reduces (or at least highlights) complexity faster than starting from the bottom up. This would be your component.



Here's all the things your component does:




  • Use the factory to get data

  • Use additional switch statements based on the type of things returned from ListItemFactory to do not much

  • Render a list given a set of (ideally) interchangeable items.


The multiple switch statements with the same cases are a smell, and in this case it looks like you've built your component to use concrete types, when it should be using some shared interface (abstract type). This is the whole reason for using factories and duck-typed objects (aka Liskov substitution principle, the L in SOLID). You shouldn't have to worry about the concrete type, only that everything shares the same behavior – in this case these things must all return state that's compatible with ListMarqueeSelection's items prop: IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem. Use TypeScript to enforce that if you want, but omit mentions of the concrete classes in your component.



To explain more fully, if you're using a factory, you shouldn't see any references to the underlying object types (e.g. IListItem) after calling the factory; otherwise, you're going to have to update the component anytime you want to add a new object type, and this is a violation of the Open-closed principle (the O in SOLID). You should be able to add new types of lists without modifying the component – only the factory. In your case, it's probably as simple as just removing the switch statement, and storing items with setState(). Here I feel like TypeScript is working against you, and if you omit the type casting, your code gets cleaner.



I would write your component to look more like this:






import listFactory from 'wherever';

const MarqueeThing = ({ items }) => (
<ul>
{items.map(item => (<li>{item.text}</li>))}
</ul>
);

class List extends Component {
constructor(props) {
super(props);
this.state = { items: }
}

componentDidMount() {
const listFetcher = listFactory(this.props.listType)
listFetcher.fetch().then(items => this.setState({ items });
}

render() {
if (!this.state.items.length) {
return 'Loading...';
}

return (<MarqueeThing items={this.state.items} />);
}
}





On to the factory – here's all the things your ListItemFactory does:




  • Select what kind of thing to return (the switch-statement)

  • Fetch data

  • Transform data


Factories typically just do the first thing – they may or may not actually instantiate classes (often they just return the class type), and they certainly don't have intimate details about the construction of the things they return.



I would start by moving the code in each switch statement to a separate object (you already have these – IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem). After doing so, the factory can just return the object type, and the switch statement can simply be replaced with a dictionary. The component that uses the factory's returned object type can then instantiate/invoke.



I would rewrite your factory to look more like this:






import newsItemFetcher from 'wherever';
import directoryItemsFetcher from 'wherever';

export default (listType) => {
const lookup = {
'News': newsItemsFetcher,
'Directory': directoryItemsFetcher,
// ...
}
return lookup[listType];
}





Then, you just need to make sure each of the items returned from the factory has a .fetch() that returns a Promise.



Finally, on to the concrete objects returned from the factory. At this point, these are data providers masquerading as models. They're functions and transformations, and they return data in the same format, but it's the transformation that's important, not the data. This is a common problem people have with OOP - they focus too much on the object's properties, and not enough on the messages (function calls / behavior). Alan Kay, the guy that coined the term OOP regrets the name for this reason.



With that in mind, I would update all of your I*Item things to simply be a function that returns a promise, and this is the code that's in the factory's switch statement. Then, you could remove the .fetch() in the component, and simply do:






const fetch = listFetcherFactory(this.props.listType);
fetch().then(items => this.setState({ items });





Please excuse my rough code - if you need clarification, please ask.






share|improve this answer





















    Your Answer





    StackExchange.ifUsing("editor", function () {
    return StackExchange.using("mathjaxEditing", function () {
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    });
    });
    }, "mathjax-editing");

    StackExchange.ifUsing("editor", function () {
    StackExchange.using("externalEditor", function () {
    StackExchange.using("snippets", function () {
    StackExchange.snippets.init();
    });
    });
    }, "code-snippets");

    StackExchange.ready(function() {
    var channelOptions = {
    tags: "".split(" "),
    id: "196"
    };
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function() {
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled) {
    StackExchange.using("snippets", function() {
    createEditor();
    });
    }
    else {
    createEditor();
    }
    });

    function createEditor() {
    StackExchange.prepareEditor({
    heartbeatType: 'answer',
    convertImagesToLinks: false,
    noModals: true,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    imageUploader: {
    brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
    contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
    allowUrls: true
    },
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    });


    }
    });














    draft saved

    draft discarded


















    StackExchange.ready(
    function () {
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f195263%2ffactory-method-done-on-reactjs-component%23new-answer', 'question_page');
    }
    );

    Post as a guest















    Required, but never shown

























    1 Answer
    1






    active

    oldest

    votes








    1 Answer
    1






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes








    up vote
    0
    down vote













    When I feel a design is overcomplicated (read: "big objects/functions"), I take a step back and think about the concerns. You have multiple objects and files, so which one should you start with? I always try to start from the top – the largest thing, the thing highest in the call stack, and/or the thing closest to the user – this approach reduces (or at least highlights) complexity faster than starting from the bottom up. This would be your component.



    Here's all the things your component does:




    • Use the factory to get data

    • Use additional switch statements based on the type of things returned from ListItemFactory to do not much

    • Render a list given a set of (ideally) interchangeable items.


    The multiple switch statements with the same cases are a smell, and in this case it looks like you've built your component to use concrete types, when it should be using some shared interface (abstract type). This is the whole reason for using factories and duck-typed objects (aka Liskov substitution principle, the L in SOLID). You shouldn't have to worry about the concrete type, only that everything shares the same behavior – in this case these things must all return state that's compatible with ListMarqueeSelection's items prop: IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem. Use TypeScript to enforce that if you want, but omit mentions of the concrete classes in your component.



    To explain more fully, if you're using a factory, you shouldn't see any references to the underlying object types (e.g. IListItem) after calling the factory; otherwise, you're going to have to update the component anytime you want to add a new object type, and this is a violation of the Open-closed principle (the O in SOLID). You should be able to add new types of lists without modifying the component – only the factory. In your case, it's probably as simple as just removing the switch statement, and storing items with setState(). Here I feel like TypeScript is working against you, and if you omit the type casting, your code gets cleaner.



    I would write your component to look more like this:






    import listFactory from 'wherever';

    const MarqueeThing = ({ items }) => (
    <ul>
    {items.map(item => (<li>{item.text}</li>))}
    </ul>
    );

    class List extends Component {
    constructor(props) {
    super(props);
    this.state = { items: }
    }

    componentDidMount() {
    const listFetcher = listFactory(this.props.listType)
    listFetcher.fetch().then(items => this.setState({ items });
    }

    render() {
    if (!this.state.items.length) {
    return 'Loading...';
    }

    return (<MarqueeThing items={this.state.items} />);
    }
    }





    On to the factory – here's all the things your ListItemFactory does:




    • Select what kind of thing to return (the switch-statement)

    • Fetch data

    • Transform data


    Factories typically just do the first thing – they may or may not actually instantiate classes (often they just return the class type), and they certainly don't have intimate details about the construction of the things they return.



    I would start by moving the code in each switch statement to a separate object (you already have these – IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem). After doing so, the factory can just return the object type, and the switch statement can simply be replaced with a dictionary. The component that uses the factory's returned object type can then instantiate/invoke.



    I would rewrite your factory to look more like this:






    import newsItemFetcher from 'wherever';
    import directoryItemsFetcher from 'wherever';

    export default (listType) => {
    const lookup = {
    'News': newsItemsFetcher,
    'Directory': directoryItemsFetcher,
    // ...
    }
    return lookup[listType];
    }





    Then, you just need to make sure each of the items returned from the factory has a .fetch() that returns a Promise.



    Finally, on to the concrete objects returned from the factory. At this point, these are data providers masquerading as models. They're functions and transformations, and they return data in the same format, but it's the transformation that's important, not the data. This is a common problem people have with OOP - they focus too much on the object's properties, and not enough on the messages (function calls / behavior). Alan Kay, the guy that coined the term OOP regrets the name for this reason.



    With that in mind, I would update all of your I*Item things to simply be a function that returns a promise, and this is the code that's in the factory's switch statement. Then, you could remove the .fetch() in the component, and simply do:






    const fetch = listFetcherFactory(this.props.listType);
    fetch().then(items => this.setState({ items });





    Please excuse my rough code - if you need clarification, please ask.






    share|improve this answer

























      up vote
      0
      down vote













      When I feel a design is overcomplicated (read: "big objects/functions"), I take a step back and think about the concerns. You have multiple objects and files, so which one should you start with? I always try to start from the top – the largest thing, the thing highest in the call stack, and/or the thing closest to the user – this approach reduces (or at least highlights) complexity faster than starting from the bottom up. This would be your component.



      Here's all the things your component does:




      • Use the factory to get data

      • Use additional switch statements based on the type of things returned from ListItemFactory to do not much

      • Render a list given a set of (ideally) interchangeable items.


      The multiple switch statements with the same cases are a smell, and in this case it looks like you've built your component to use concrete types, when it should be using some shared interface (abstract type). This is the whole reason for using factories and duck-typed objects (aka Liskov substitution principle, the L in SOLID). You shouldn't have to worry about the concrete type, only that everything shares the same behavior – in this case these things must all return state that's compatible with ListMarqueeSelection's items prop: IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem. Use TypeScript to enforce that if you want, but omit mentions of the concrete classes in your component.



      To explain more fully, if you're using a factory, you shouldn't see any references to the underlying object types (e.g. IListItem) after calling the factory; otherwise, you're going to have to update the component anytime you want to add a new object type, and this is a violation of the Open-closed principle (the O in SOLID). You should be able to add new types of lists without modifying the component – only the factory. In your case, it's probably as simple as just removing the switch statement, and storing items with setState(). Here I feel like TypeScript is working against you, and if you omit the type casting, your code gets cleaner.



      I would write your component to look more like this:






      import listFactory from 'wherever';

      const MarqueeThing = ({ items }) => (
      <ul>
      {items.map(item => (<li>{item.text}</li>))}
      </ul>
      );

      class List extends Component {
      constructor(props) {
      super(props);
      this.state = { items: }
      }

      componentDidMount() {
      const listFetcher = listFactory(this.props.listType)
      listFetcher.fetch().then(items => this.setState({ items });
      }

      render() {
      if (!this.state.items.length) {
      return 'Loading...';
      }

      return (<MarqueeThing items={this.state.items} />);
      }
      }





      On to the factory – here's all the things your ListItemFactory does:




      • Select what kind of thing to return (the switch-statement)

      • Fetch data

      • Transform data


      Factories typically just do the first thing – they may or may not actually instantiate classes (often they just return the class type), and they certainly don't have intimate details about the construction of the things they return.



      I would start by moving the code in each switch statement to a separate object (you already have these – IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem). After doing so, the factory can just return the object type, and the switch statement can simply be replaced with a dictionary. The component that uses the factory's returned object type can then instantiate/invoke.



      I would rewrite your factory to look more like this:






      import newsItemFetcher from 'wherever';
      import directoryItemsFetcher from 'wherever';

      export default (listType) => {
      const lookup = {
      'News': newsItemsFetcher,
      'Directory': directoryItemsFetcher,
      // ...
      }
      return lookup[listType];
      }





      Then, you just need to make sure each of the items returned from the factory has a .fetch() that returns a Promise.



      Finally, on to the concrete objects returned from the factory. At this point, these are data providers masquerading as models. They're functions and transformations, and they return data in the same format, but it's the transformation that's important, not the data. This is a common problem people have with OOP - they focus too much on the object's properties, and not enough on the messages (function calls / behavior). Alan Kay, the guy that coined the term OOP regrets the name for this reason.



      With that in mind, I would update all of your I*Item things to simply be a function that returns a promise, and this is the code that's in the factory's switch statement. Then, you could remove the .fetch() in the component, and simply do:






      const fetch = listFetcherFactory(this.props.listType);
      fetch().then(items => this.setState({ items });





      Please excuse my rough code - if you need clarification, please ask.






      share|improve this answer























        up vote
        0
        down vote










        up vote
        0
        down vote









        When I feel a design is overcomplicated (read: "big objects/functions"), I take a step back and think about the concerns. You have multiple objects and files, so which one should you start with? I always try to start from the top – the largest thing, the thing highest in the call stack, and/or the thing closest to the user – this approach reduces (or at least highlights) complexity faster than starting from the bottom up. This would be your component.



        Here's all the things your component does:




        • Use the factory to get data

        • Use additional switch statements based on the type of things returned from ListItemFactory to do not much

        • Render a list given a set of (ideally) interchangeable items.


        The multiple switch statements with the same cases are a smell, and in this case it looks like you've built your component to use concrete types, when it should be using some shared interface (abstract type). This is the whole reason for using factories and duck-typed objects (aka Liskov substitution principle, the L in SOLID). You shouldn't have to worry about the concrete type, only that everything shares the same behavior – in this case these things must all return state that's compatible with ListMarqueeSelection's items prop: IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem. Use TypeScript to enforce that if you want, but omit mentions of the concrete classes in your component.



        To explain more fully, if you're using a factory, you shouldn't see any references to the underlying object types (e.g. IListItem) after calling the factory; otherwise, you're going to have to update the component anytime you want to add a new object type, and this is a violation of the Open-closed principle (the O in SOLID). You should be able to add new types of lists without modifying the component – only the factory. In your case, it's probably as simple as just removing the switch statement, and storing items with setState(). Here I feel like TypeScript is working against you, and if you omit the type casting, your code gets cleaner.



        I would write your component to look more like this:






        import listFactory from 'wherever';

        const MarqueeThing = ({ items }) => (
        <ul>
        {items.map(item => (<li>{item.text}</li>))}
        </ul>
        );

        class List extends Component {
        constructor(props) {
        super(props);
        this.state = { items: }
        }

        componentDidMount() {
        const listFetcher = listFactory(this.props.listType)
        listFetcher.fetch().then(items => this.setState({ items });
        }

        render() {
        if (!this.state.items.length) {
        return 'Loading...';
        }

        return (<MarqueeThing items={this.state.items} />);
        }
        }





        On to the factory – here's all the things your ListItemFactory does:




        • Select what kind of thing to return (the switch-statement)

        • Fetch data

        • Transform data


        Factories typically just do the first thing – they may or may not actually instantiate classes (often they just return the class type), and they certainly don't have intimate details about the construction of the things they return.



        I would start by moving the code in each switch statement to a separate object (you already have these – IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem). After doing so, the factory can just return the object type, and the switch statement can simply be replaced with a dictionary. The component that uses the factory's returned object type can then instantiate/invoke.



        I would rewrite your factory to look more like this:






        import newsItemFetcher from 'wherever';
        import directoryItemsFetcher from 'wherever';

        export default (listType) => {
        const lookup = {
        'News': newsItemsFetcher,
        'Directory': directoryItemsFetcher,
        // ...
        }
        return lookup[listType];
        }





        Then, you just need to make sure each of the items returned from the factory has a .fetch() that returns a Promise.



        Finally, on to the concrete objects returned from the factory. At this point, these are data providers masquerading as models. They're functions and transformations, and they return data in the same format, but it's the transformation that's important, not the data. This is a common problem people have with OOP - they focus too much on the object's properties, and not enough on the messages (function calls / behavior). Alan Kay, the guy that coined the term OOP regrets the name for this reason.



        With that in mind, I would update all of your I*Item things to simply be a function that returns a promise, and this is the code that's in the factory's switch statement. Then, you could remove the .fetch() in the component, and simply do:






        const fetch = listFetcherFactory(this.props.listType);
        fetch().then(items => this.setState({ items });





        Please excuse my rough code - if you need clarification, please ask.






        share|improve this answer












        When I feel a design is overcomplicated (read: "big objects/functions"), I take a step back and think about the concerns. You have multiple objects and files, so which one should you start with? I always try to start from the top – the largest thing, the thing highest in the call stack, and/or the thing closest to the user – this approach reduces (or at least highlights) complexity faster than starting from the bottom up. This would be your component.



        Here's all the things your component does:




        • Use the factory to get data

        • Use additional switch statements based on the type of things returned from ListItemFactory to do not much

        • Render a list given a set of (ideally) interchangeable items.


        The multiple switch statements with the same cases are a smell, and in this case it looks like you've built your component to use concrete types, when it should be using some shared interface (abstract type). This is the whole reason for using factories and duck-typed objects (aka Liskov substitution principle, the L in SOLID). You shouldn't have to worry about the concrete type, only that everything shares the same behavior – in this case these things must all return state that's compatible with ListMarqueeSelection's items prop: IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem. Use TypeScript to enforce that if you want, but omit mentions of the concrete classes in your component.



        To explain more fully, if you're using a factory, you shouldn't see any references to the underlying object types (e.g. IListItem) after calling the factory; otherwise, you're going to have to update the component anytime you want to add a new object type, and this is a violation of the Open-closed principle (the O in SOLID). You should be able to add new types of lists without modifying the component – only the factory. In your case, it's probably as simple as just removing the switch statement, and storing items with setState(). Here I feel like TypeScript is working against you, and if you omit the type casting, your code gets cleaner.



        I would write your component to look more like this:






        import listFactory from 'wherever';

        const MarqueeThing = ({ items }) => (
        <ul>
        {items.map(item => (<li>{item.text}</li>))}
        </ul>
        );

        class List extends Component {
        constructor(props) {
        super(props);
        this.state = { items: }
        }

        componentDidMount() {
        const listFetcher = listFactory(this.props.listType)
        listFetcher.fetch().then(items => this.setState({ items });
        }

        render() {
        if (!this.state.items.length) {
        return 'Loading...';
        }

        return (<MarqueeThing items={this.state.items} />);
        }
        }





        On to the factory – here's all the things your ListItemFactory does:




        • Select what kind of thing to return (the switch-statement)

        • Fetch data

        • Transform data


        Factories typically just do the first thing – they may or may not actually instantiate classes (often they just return the class type), and they certainly don't have intimate details about the construction of the things they return.



        I would start by moving the code in each switch statement to a separate object (you already have these – IListItem, INewsListItem, IAnnouncementListItem, IDirectoryListItem). After doing so, the factory can just return the object type, and the switch statement can simply be replaced with a dictionary. The component that uses the factory's returned object type can then instantiate/invoke.



        I would rewrite your factory to look more like this:






        import newsItemFetcher from 'wherever';
        import directoryItemsFetcher from 'wherever';

        export default (listType) => {
        const lookup = {
        'News': newsItemsFetcher,
        'Directory': directoryItemsFetcher,
        // ...
        }
        return lookup[listType];
        }





        Then, you just need to make sure each of the items returned from the factory has a .fetch() that returns a Promise.



        Finally, on to the concrete objects returned from the factory. At this point, these are data providers masquerading as models. They're functions and transformations, and they return data in the same format, but it's the transformation that's important, not the data. This is a common problem people have with OOP - they focus too much on the object's properties, and not enough on the messages (function calls / behavior). Alan Kay, the guy that coined the term OOP regrets the name for this reason.



        With that in mind, I would update all of your I*Item things to simply be a function that returns a promise, and this is the code that's in the factory's switch statement. Then, you could remove the .fetch() in the component, and simply do:






        const fetch = listFetcherFactory(this.props.listType);
        fetch().then(items => this.setState({ items });





        Please excuse my rough code - if you need clarification, please ask.






        import listFactory from 'wherever';

        const MarqueeThing = ({ items }) => (
        <ul>
        {items.map(item => (<li>{item.text}</li>))}
        </ul>
        );

        class List extends Component {
        constructor(props) {
        super(props);
        this.state = { items: }
        }

        componentDidMount() {
        const listFetcher = listFactory(this.props.listType)
        listFetcher.fetch().then(items => this.setState({ items });
        }

        render() {
        if (!this.state.items.length) {
        return 'Loading...';
        }

        return (<MarqueeThing items={this.state.items} />);
        }
        }





        import listFactory from 'wherever';

        const MarqueeThing = ({ items }) => (
        <ul>
        {items.map(item => (<li>{item.text}</li>))}
        </ul>
        );

        class List extends Component {
        constructor(props) {
        super(props);
        this.state = { items: }
        }

        componentDidMount() {
        const listFetcher = listFactory(this.props.listType)
        listFetcher.fetch().then(items => this.setState({ items });
        }

        render() {
        if (!this.state.items.length) {
        return 'Loading...';
        }

        return (<MarqueeThing items={this.state.items} />);
        }
        }





        import newsItemFetcher from 'wherever';
        import directoryItemsFetcher from 'wherever';

        export default (listType) => {
        const lookup = {
        'News': newsItemsFetcher,
        'Directory': directoryItemsFetcher,
        // ...
        }
        return lookup[listType];
        }





        import newsItemFetcher from 'wherever';
        import directoryItemsFetcher from 'wherever';

        export default (listType) => {
        const lookup = {
        'News': newsItemsFetcher,
        'Directory': directoryItemsFetcher,
        // ...
        }
        return lookup[listType];
        }





        const fetch = listFetcherFactory(this.props.listType);
        fetch().then(items => this.setState({ items });





        const fetch = listFetcherFactory(this.props.listType);
        fetch().then(items => this.setState({ items });






        share|improve this answer












        share|improve this answer



        share|improve this answer










        answered Nov 5 at 5:22









        Johntron

        1,005625




        1,005625






























            draft saved

            draft discarded




















































            Thanks for contributing an answer to Code Review Stack Exchange!


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            Use MathJax to format equations. MathJax reference.


            To learn more, see our tips on writing great answers.





            Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


            Please pay close attention to the following guidance:


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            To learn more, see our tips on writing great answers.




            draft saved


            draft discarded














            StackExchange.ready(
            function () {
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f195263%2ffactory-method-done-on-reactjs-component%23new-answer', 'question_page');
            }
            );

            Post as a guest















            Required, but never shown





















































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown

































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown







            Popular posts from this blog

            404 Error Contact Form 7 ajax form submitting

            How to know if a Active Directory user can login interactively

            TypeError: fit_transform() missing 1 required positional argument: 'X'