3
\$\begingroup\$

I have to create a form, which enables users to move stock of items to different statuses. The form will show the current stock(quantity) in a given status and then 5 input fields corresponding to 5 statuses. The user can input into one or more fields as to the quantity that they want to be moved into the new status. Once the user submits the form, based on whether a field has value, I have to put an entry corresponding to that entry into a table which will store some common properties as well as the intended status and the quantity of each.

Consider the following scenario, if total stock in status Purchased is 100.

  • Production - 30
  • Finished - 20
  • Finished (Internal Only)
  • Wastage
  • Base Stock - 50

I will need to create 3 entries corresponding to Production, Finished and Base Stock, respectively from a single object returned from the front end form.

The application is written in AngularJS and C# Web API. The following is what I have written so far. WStockStatus is a single object in which I capture the input values from front end as separate fields.

Does this follow best practices? Is there a better way to go about it?

public bool moveStock(WStockStatus fromStockStatus)
    {
    const int STATUS_BASE = 1;
    const int STATUS_FIN = 2;
    const int STATUS_FINI = 3;
    const int STATUS_WAST = 4;
    const int STATUS_PRGR = 5;
    StockStatus stockStatus = new StockStatus();
    stockStatus.date = fromStockStatus.date;
    stockStatus.location = fromStockStatus.location;
    if(fromStockStatus.qtyBase > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyBase;
        stockStatus.status = STATUS_BASE;
        createStockStatus(stockStatus);  //another function that creates individual entry for stock status
    }
    if(fromStockStatus.qtyFin > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyFin;
        stockStatus.status = STATUS_FIN;
        createStockStatus(stockStatus);
    }
    if(fromStockStatus.qtyFini > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyFini;
        stockStatus.status = STATUS_FINI;
        createStockStatus(stockStatus); 
    }
    if(fromStockStatus.qtyWast > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyWast;
        stockStatus.status = STATUS_WAST;
        createStockStatus(stockStatus);
    }
    if(fromStockStatus.qtyPrgr > 0)
    {
        stockStatus.quantityUOM = fromStockStatus.qtyPrgr;
        stockStatus.status = STATUS_PRGR;
        createStockStatus(stockStatus); 
    }
    return true;
}


    private void createStockStatus(StockStatus newObj)
    {
        if (newObj.isValid())
        {
            newObj.deleted = false;
            newObj.submittedId = null;
            UserPersonal user = context.UserPersonal.FirstOrDefault(u => u.email == currentUserEmail);
            newObj.setCreatedTimeStamp(user); //sets fields CreatedBy, ModifiedBy with value of user
            context.StockStatus.Add(newObj);
            context.SaveChanges();
        }
        return null;
    }
\$\endgroup\$
  • \$\begingroup\$ Those are used to indicate other assignment statements which are totally not related to the problem at hand. Do I still need to put the code in place? \$\endgroup\$ – roshan Sep 11 '18 at 5:05
  • \$\begingroup\$ Is it possible that e.g both qtyWast > 0 and qtyPrgr > 0 ? \$\endgroup\$ – Heslacher Sep 11 '18 at 5:29
  • 1
    \$\begingroup\$ Yes, any of those 5 fields can have some value in them. With the example that I gave, the if conditions corresponding to qtyBase, qtyPrgr and qtyFin will be true. \$\endgroup\$ – roshan Sep 11 '18 at 5:38
2
\$\begingroup\$

This

const int STATUS_BASE = 1;
const int STATUS_FIN = 2;
const int STATUS_FINI = 3;
const int STATUS_WAST = 4;
const int STATUS_PRGR = 5;  

screams for an enum like

public enum StockState
{
    Base = 1, Fin, Fini, Wast, Prgr 
}  

but you should rename the enumvalues in a way that you or Sam the maintainer doesn't has to guess about the meaning. If you or Sam have to fix a bug in 6 months it will be easier if your code can be graps at first glance. Just don't use abbreviations for naming things.


bool moveStock()

Based on the .NET Naming Guidlines methods should be named using PascalCase casing hence moveStock() should read MoveStock(). While we are at naming things I would name the method parameter sourceStockState which is IMO more clear.

stockStatus.date again properties should be named using PascalCase casing.


Because moveStock can't return false it should be void.


Let's get the meat. If you would change the createStockStatus() method in a way that it takes 2 additional parameters your moveStock() would get more readable because it would get shorter.

Assume the createStockStatus() would look like so

private void createStockStatus(StockStatus newObj, int quantityUOM, StockState state)
{
    if (!newObj.isValid()) { return; }

    newObj.quantityUOM = quantityUOM;
    newObj.state = (int)state;
    newObj.deleted = false;
    newObj.submittedId = null;
    UserPersonal user = context.UserPersonal.FirstOrDefault(u => u.email == currentUserEmail);
    newObj.setCreatedTimeStamp(user); //sets fields CreatedBy, ModifiedBy with value of user
    context.StockStatus.Add(newObj);
    context.SaveChanges();
} 

then the former moveStock() could look like so

public void moveStock(WStockStatus fromStockStatus)
{
    StockStatus stockStatus = new StockStatus();
    stockStatus.date = fromStockStatus.date;
    stockStatus.location = fromStockStatus.location;

    if(fromStockStatus.qtyBase > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyBase, StockState.Base); 
    }
    if(fromStockStatus.qtyFin > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyFin, StockState.Fin); 
    }
    if(fromStockStatus.qtyFini > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyFini, StockState.Fini); 
    }
    if(fromStockStatus.qtyWast > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyWast, StockState.Wast); 
    }
    if(fromStockStatus.qtyPrgr > 0)
    {
        createStockStatus(stockStatus, fromStockStatus.qtyPrgr, StockState.Prgr); 
    }
}

Edit

But my main intention was to see if there is a better way by which I can avoid using these multiple if statements, like using a different data structure or use a different format of capturing the data.

By moving the if condition into createStockStatus() like so

private void createStockStatus(StockStatus newObj, int quantityUOM, StockState state)
{
    if (quantityUOM <= 0 || !newObj.isValid()) { return; }

    newObj.quantityUOM = quantityUOM;
    newObj.state = (int)state;
    newObj.deleted = false;
    newObj.submittedId = null;
    UserPersonal user = context.UserPersonal.FirstOrDefault(u => u.email == currentUserEmail);
    newObj.setCreatedTimeStamp(user); //sets fields CreatedBy, ModifiedBy with value of user
    context.StockStatus.Add(newObj);
    context.SaveChanges();
}  

the moveStock() could look like so

public void moveStock(WStockStatus fromStockStatus)
{
    StockStatus stockStatus = new StockStatus();
    stockStatus.date = fromStockStatus.date;
    stockStatus.location = fromStockStatus.location;

    createStockStatus(stockStatus, fromStockStatus.qtyBase, StockState.Base); 
    createStockStatus(stockStatus, fromStockStatus.qtyFin, StockState.Fin); 
    createStockStatus(stockStatus, fromStockStatus.qtyFini, StockState.Fini); 
    createStockStatus(stockStatus, fromStockStatus.qtyWast, StockState.Wast); 
    createStockStatus(stockStatus, fromStockStatus.qtyPrgr, StockState.Prgr); 

}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you. Looks a lot better and meaningful already. But my main intention was to see if there is a better way by which I can avoid using these multiple if statements, like using a different data structure or use a different format of capturing the data. \$\endgroup\$ – roshan Sep 11 '18 at 6:20
  • \$\begingroup\$ I cannot change the front end layout which means all the 5 input fields for 5 statuses has to be there. So I am capturing all these inputs in separate fields (qtyBase, etc) of a single object of WStockStatus. Is there something that I can change here which will make the code better? \$\endgroup\$ – roshan Sep 11 '18 at 6:24
  • \$\begingroup\$ @BCdotWEB thanks for that. Answering in two tabs lead to this :-( \$\endgroup\$ – Heslacher Sep 11 '18 at 14:09

Your Answer

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

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