Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have a colleague who has come up with a way of 'genericizing' information from a database so that all his web application's drop-down lists can share the same object in his MVC.NET C# code and Views, which can contain different data depending on what tables it is being used against.

We work for a government agency, and we have facilities divided up into areas called "Regions", which contain further subdivided areas called VISNs, which each contain "Sites" or "Facilities". My colleague has developed a very complex security scheme whereby people can be granted access to data based on their permission level which can be assigned by Region, VISN, Site, or any mixture of the three. It's a nice scheme and very clever. However, he has stored procedures that return lists of Regions, VISNs, and Sites based on a person's User Id, and he is returning "generic" values like TextFieldID, TextField, and TextParentID. My first problem with this, is that looking at this data coming out of the database, I would not know what the data is. I feel that fields coming from a query or stored procedure should be descriptive of the data they are delivering. What does everyone else think? The deeper issue for me however is that he is taking some of the data, concatenating it in his stored procedure like this

SELECT DISTINCT
    t.VisnID,
    NULL,
    t.StationID,
    'V' + CAST(t.VisnID as varchar) + ': ' + t.Station3N + ': ' + t.StationName,
    t.Inactive
FROM Stations t 

and sending it back in a "TextField" property, instead of sending back the discrete data separately (Station3N, StationName) and concatenating it in the View, which would allow for different concatenation depending on what device what accessing the application (perhaps mobile and desktop). His justification is that he can send all his various drop down data and capture them all, regardless of content, in the same C# object named "LookupValue."

public partial class LookupValue : IEquatable<LookupValue>
{
    public LookupValue(string textFieldId, string textField, bool inactive)
    {
        TextFieldID = textFieldId;
        TextField = textField;
        Inactive = inactive;
    }

    public LookupValue(string textParentId, string textFieldId, string textField, bool inactive)
    {
        TextParentID = textParentId;
        TextFieldID = textFieldId;
        TextField = textField;
        Inactive = inactive;
    }

    public LookupValue(string textParentId, string textSelfParentId, string textFieldId, string textField, bool inactive)
    {
        TextParentID = textParentId;
        TextSelfParentID = textSelfParentId;
        TextFieldID = textFieldId;
        TextField = textField;
        Inactive = inactive;
    }

    /// <summary>
    /// Returns a custom string identifier if the Inactive property is true.
    /// </summary>
    public string GetInactiveString()
    {
        string value = "";
        if (Inactive)
        {
            value = "[I]";
        }

        return value;
    }

    /// <summary>
    /// Returns a custom text label that concatenates the TextField property with the custom string identifier of the Inactive property.
    /// </summary>
    public string GetDisplayNameWithInactiveString
    {
        get { return TextField + " " + GetInactiveString(); }
    }

    public bool Equals(LookupValue other)
    {
        //Check whether the compared object is null.  
        if (Object.ReferenceEquals(other, null)) return false;

        //Check whether the compared object references the same data.  
        if (Object.ReferenceEquals(this, other)) return true;

        //Check whether the products' properties are equal.  
        return TextFieldID.Equals(other.TextFieldID) && TextField.Equals(other.TextField);
    }

    // If Equals() returns true for a pair of objects
    // then GetHashCode() must return the same value for these objects.
    public override int GetHashCode()
    {
        //Get hash code for the TextFieldID field if it is not null.  
        int hashTextFieldId = TextFieldID == null ? 0 : TextFieldID.GetHashCode();

        //Get hash code for the Code field.  
        int hashTextField = TextField.GetHashCode();

        //Calculate the hash code for the product.  
        return hashTextFieldId ^ hashTextField;
    }
}

He believes the re-usability of this object is worth the violation of Separation of Concerns and possible future difficulties in handling more than one display variation for a drop-down. I should point out that this object is contained in his Data project namespace (our projects are separated into Data, Web, Services, etc.) instead of the Web project namespace and he also returns this object to the Web layer via his Repository queries which call the stored procedures that I described earlier, which is gross violation of Separation of Concerns in my opinion.

I am just looking for some confirmation from other programmers that this is in fact a bad practice, and also looking for ideas I can present him on better ways to do what he is attempting. I have my own ideas, but again I'm just looking for other's ideas so I have more options to present to him.

--Edit -- I forgot to mention that we are using Entity Framework as an ORM, and his Repository classes in his DAL are dumping the data from the stored procs into this LookupValue object.

share|improve this question
4  
It looks to me that he might very well be a smart guy who wants to create a semi-low-level architecture framework for that big government data you mentioned. People can come up with many ways to criticize it and from the way it is represented, it looks terrible. However, I myself would refrain from doing so. There might be lot of positive things that you don't know. At the end of the day, this guy essentially wrote his own customized database query framework. If he was able to make this work for the government huge data, he is most likely better than most of us here. –  InstructedA Jul 9 at 16:45
    
Does this ingenious solution solve a problem? –  Andrew Hoffman Jul 9 at 16:59
    
Having a stringly typed text field in the LookupValue class rather than making LookupValue a generic type parameter of the class seems like kind of a bad idea. Also, having serialization that prevents your data from containing ":" isn't ideal. –  psr Jul 9 at 17:02
1  
@user140149 I thought I was clear on that. Yes, I know it's a bad practice, but I am looking for some confirmation of it since I am not omniscient and this man is my boss and I also want some options for better solutions that I may not have thought of. –  JasonAlun Jul 9 at 17:24
1  
I confess I didn't follow exactly what was going on, but I'll leave a small piece of advice: if you can't sum up what you're doing in a sentence for purposes of a title and just have to put "this", it's probably not a good practice. –  corsiKa Jul 9 at 20:09

5 Answers 5

up vote 8 down vote accepted

I'm going to start flat out by saying after more than 30 years experience writing software and working in I.T in general, I have NEVER, EVER yet found a good reason for concatenating data coming out of a database.

You could be putting 90% of your application code in the DB infrastructure, and doing all sorts of magic things with it, and I still would not be persuaded that it's ever a good idea.

As has been hinted already, databases are designed for one specific task, and one specific task only, storing, searching, retrieving and general management of data.

The meta data, indexes, clusters and everything else a DBMS does well are there to ensure that it does it's job with the best efficiency possible, and if you try to circumvent that, your gonna land yourself in a whole heap of trouble.

Look at it this way, would you attempt to take your own C# code, and compile it by hand, to get better results than the compiler, could you even get better results, I doubt it, in the same token creating your own data scheme is reckless and irresponsible imho.

That said, maybe your colleague has his reasons, there may as others have said, be factors your not privy too.

What he needs to consider is not only the ramifications of his en-devour on current affairs, but what it holds for the future.

things like:

  • 1) Future maintenance. Will he be still there in 10 years, or even 1 year for that matter? If not, who's going to maintain it.

  • 2) If he does leave, and he leaves under bad circumstances, will someone get trained in how his system works? Or will upper management just assume that everyone who worked with him understands this code? (Take it from me, I've been in this position quite a few times, it's not funny when you don't have the first clue what your looking at)

  • 3) What happens if the HTML spec changes in such a way that his concatenation breaks future revisions (Beacuse he concatenates using some protected character sequence for example), or what if future spec changes break his concatenation? (This scenario has already happened more times than you might realize)

  • 4) If your database server goes down in a ball of flames, which is easier to recover, a standards compliant database structure that everyone understands, or a hideousley obfuscated 3 headed mess of stored procs and custom data schemes. (I know which I'd pick)

I could sit here and give you a dozen more reasons easily, but I'm not going too.

Instead, I'm going to propose what just might be way out, that keeps you both happy.

He might be doing things this way because he doesn't know better.

You mention he's more of a DBA than a DEV, and that's fine, Iv'e worked with many guys who are amazing DBA's but put a compiler in front of them and they'll curl up a ball and start to cry, and will happily admit their lack of knowledge.

Likewise, Iv'e worked with DBA's who are so utterly stubborn and blind to their way of doing things for what ever reason, that they'll enforce that structure all the way down the line, often because they don't really understand whats happening beyond the DBA, but like many of us don't want to admit it (Human nature, is what it is...)

Whichever way you look at it, he's not going to budge, and your not going to be happy until your "do it correctly, inner dev" is pacified.

What you need is a compromise.

In my mind, you can easily achieve this compromise using views.

See if you can get him to put all his logic into a set of views, then the code that needs this approach can query those, just as easily as it queries existing tables and/or stored procs.

This way, the actual table definitions, remain in a pristine condition with separate columns, so you can write standard, compliant ORM based code against them.

Iv'e used this approach very successfully in the past for a not too dissimilar scenario, where I was tasked with creating an application to process some geographic data.

In my particular case, I needed to get some data back from a DB table, but this table had already been pre-processed in the back-end to return the data in a format suitable for another app. My solution here was to transform that data back to what was needed for my task, using a view.

Doing this, kept the original interface, and program code working, but it gave me the flexibility to change things for the better in the slice of the work I needed to do.

If you can't take this approach at the DB level, because for example he controls the DB and it's access, then you may have to resort to other tricks.

If your working in C# as you say you are, then it's not difficult to hook the code in different modules.

.NET code what ever language it's written in, is compiled down to simple MSIL, and there are dozens of ways of injecting your own code in there to replace anything that's already running.

Robert Harvey above mentioned using a product like Automapper.

This is a quite reasonable suggestion, but one you'll want to give some thought to.

I use Automapper all the time, and it's good at the job it's designed for. That job is passing and matching DTO's between layers in your N-Tier design.

Automapper is typically used to flatten and combine multiple DTO's into a single level view model, typically for use in a presentation layer driven pattern such as MVC or MVVM.

In this particular case however, I don't quite see how Automapper might help you. AM is conventions based. This means that if you have say an EF object on one side that looks like this:

public class EFobject
{
  public string name { get; set; }
  public string email { get; set; }
  public string dept { get; set; }
}

and a regular POCO on the other side that looks like this:

public class RegularPoco
{
  public string name { get; set; }
  public string email { get; set; }
}

Automapper will ensure that the name and email fields get copied across, but will ignore the dept field.

If you where concatenating the strings at the code level, then you could easily use Automapper to produce the same result your colleague is generating, by specifying mapping rules that took those three columns, and pushed them into one column with the ':' symbol between each, breaking them apart however, AM is the wrong tool for the job.

Another option you have, is to handle this in your business layer.

If your following good N-Tier architecture and design principles, then at an absolute minimum you should have 'Presentation', 'Business Rules' and 'Data' layers.

Myself personally, I usually separate my DTO's and View models out into distinct layers too, but that's just to aid the way I work in Visual Studio.

With a business rules layer, nothing makes it to the presentation layer without first going through that layer, and at that point you have the entire .NET eco-sphere at your disposal to break that string apart again.

However, as you point out, that's no help when you don't know what the original data was, so you have one other possible approach to dealing with this.

If you can't break him of his habit, and he resists change, then use "Praise based change".

Praise him on his approach, and appear to be helpful, suggest "Improving" on his way of doing things by adding in data you can digest at the Business rules layer for example, get him to change the output from something like:

1234:station1:fred

to

1234(stationid,int):station1(stationref,string):fred(stationname,string)

in his select driven obsession he can then simply pass things through a simple search and replace that removes anything in the brackets including said brackets, but... gives you all the information you need to de-serialize this thing back into a regular POCO.

Out of everything I've outlined here, I really wouldn't recommend going this route however, but sometimes you just have no choice but to go with the flow, and improvements for the greater good of your task are sometimes the only effective weapon you have.

In summary:

Standards and patterns & practices exist for a reason, and that reason extends way beyond initial development into the ALM Cycle until the product is eventually replaced.

Iv'e come across so many DBA's (and devs for that matter) over the years who just can't see this bigger picture beyond finishing the list of tickets they have to work on, that it scares the living daylights out of me.

Team members should never be looking for ways to do things bigger & better than everyone else, instead you should always be looking for common ground, and common sense.

As one of the other posters above put it: simplicity is always the best option , never forget what KISS stands for, because one fine day when the S**t hits the fan, It might just be KISS that saved you from getting well and truly roasted.

share|improve this answer
2  
I'm going to start flat out by saying after more than 30 years experience writing software and working in I.T in general, I have NEVER, EVER yet found a good reason for concatenating data coming out of a database. - Here's one: Denormalizing data while importing into an inverted index (such as what Solr uses) for faster querying –  Izkata Jul 10 at 1:53
1  
nope, sorry don't agree with that. Denormalize to a flat table structure via a view yes, but Denormalize by string concatenation, nope, absolutely not, it's bad practice. Now on the flip side if you have a DB like PostgreSQL for example that can use array based columns, then yes you can Denormalize several columns into one array bound column, or you can even Denormalize to a JSON based doc-store column in NoSql, but none of those are string concatenations even though they may look as though they are, the columns are specially designed types to manage them. –  shawty Jul 10 at 9:37
    
Thank you shawty, that's very helpful. He's the stubborn type and I've got a tough row to hoe to convince him, so your alternate approaches are welcome. I think I should have posted more of the C# code that he's using (mainly his Repository queries) to show the true ugliness of what this is causing in the Web app. That's what I really need ammo shooting down. He thinks what he's doing is "good" because it's "re-usable" (the LookupValue object and the genericized region,visn, and site stored procs), but they don't save any time in web dev and end up being more confusing than helpful. –  JasonAlun Jul 10 at 15:39
    
I hear ya there 140, Iv'e had to deal with so many types like this over the years. One book that helped me endlessly was : The Career "Programmer: Guerilla Tactics for an Imperfect World" - you can find it on Amazon here - amazon.co.uk/The-Career-Programmer-Guerilla-Imperfect/dp/… - the help and advice I got from this especially in my early years (The link is the second ed) was probably more help than any programming book I had. –  shawty Jul 10 at 19:18
    
Just a reminder to be courteous and respectful everybody. –  maple_shaft Jul 11 at 17:02

This sounds suspiciously like an inner platform.

Here are the potential problems:

  1. You'll be writing queries that query the database for what information to query (metaqueries) first, rather than queries that simply retrieve the needed information.

  2. You'll be subverting the role of the database, which already provides metadata capabilities such as rows, columns, and meaningful field names and data types.

  3. You'll be defeating the indexing and querying capabilities of the database (JOINs, etc).

  4. There's magic strings everywhere.

For an extreme example of where this can lead, read Can a system be 100% Data Driven?

There are better ways to accomplish what your fellow programmer desires, while still preserving an orthodox database and application design. Check out Automapper for an alternative.

share|improve this answer
    
I'm not sure I understand how #1 applies here. Maybe I'm just not following you. I'm not disagreeing, it's just much different that the problem I'm trying to describe -the separation of concerns issue and having essentially View information embedded in your database layer. Can you be more explicit with #4? Thanks for your reply! –  JasonAlun Jul 9 at 16:38
1  
If this is just amounts to an ORM for CRUD operations, then maybe it's not as bad as it seems. Read the article about VISION in the post I linked; if that cautionary tale does not apply to your situation, then more power to you. Note that many ORMs still allow you to deal in actual, strongly-typed fields, rather than key/value pairs and magic strings. –  Robert Harvey Jul 9 at 16:56
    
Yes, of course, but we I don't see how we will be "querying the database to see what information to query" - it's a stored procedure that returns site information, and perhaps he has others that return other information that would be used in dropdown of another type. Now, as I mentioned the procedures return fields that are not intuitive, and if that's what he's referring to I concur, but that isn't my biggest problem with this whole scheme. I'm looking at it more from a web developer's perspective. –  JasonAlun Jul 9 at 16:58
1  
@user140149 I would suggest to be mindful not to treat other people's code as some kind of holy text with hidden meaning. Back-end developers will sometimes overthink a problem and forget to KISS and YAGNI. –  Andrew Hoffman Jul 9 at 17:07
1  
"Inner Platform", "Vision article"? What comes to mind? PeopleSoft –  radarbob Jul 9 at 23:07

I've seen lots of patterns like this, and generally they're good - superficially they seem complex and awkward but they do provide a good way to avoid repeating very similar data access blocks.

I do agree that concatenating the data returned in the DB is a bad idea and the 3 fields should be returned and concatenated in the client (assuming it still needs to). One question you need to consider first though, is - is this concatenation format a standard for your forms? If so, then it does appear to make sense to do it in the DB (ie the data should already be in that format and is just modelled as 3 fields in the DB, but you should be thinking of the data as a single field, ie if the API to the DB should return that particular format of data, how it stores it internally is irrelevant)

share|improve this answer

Three words that should strike fear in the hearts of all programmers 'complex security scheme'. Whilst his scheme may be the best security implementation ever written, it's just as likely to be full of holes. I would always go for simple is better, even if it means writing a bit more code. Even the best code has bugs and if its to be used in government systems the code should be as robust as possible.

In a nutshell this is not good practice and likely to fail any serious code review.

share|improve this answer
2  
I agree, but could you be more specific about what you feel would fail a code review, maybe giving an example? This fellow is my supervisor and I need all the "ammo" I can muster to convince him he's heading down a bad path. In defense of his security scheme, in itself it really isn't that complex, it's just the number of permutations of sites and such that people can have access to is complex. But the mechanisms for validation of the access is pretty simple. I don't have any problem with his scheme per se, only how he's implementing it in the MVC app. –  JasonAlun Jul 9 at 18:57
    
I presume as its a government contract the code needs to pass quality tests using whichever standards your particular agency mandates. A home invented security scheme will probably not be on the list and will need to prove itself, or more likely due to time constraints and playing safe, be rejected. –  Stevetech Jul 9 at 19:06
    
The best example of simple getting complex quickly is the trust domain system used in the days of windows NT. The trusts worked fine with one or two domains but the number of trust relationships multiplied quickly as each domain was added. My issue with the solution is not the code which I have no opinion of, just the complexity when its not the only solution. As a rule complex solutions should openly be used as a last resort when other simpler answers have failed. –  Stevetech Jul 9 at 19:11
    
split over 2 comments due to the limitations of the stack exchange platform –  Stevetech Jul 9 at 19:11
    
I won't mention the agency I work for, as someone there may come across this someday and I don't need the grief, but suffice to say it has not distinguished itself in the security arena. Code reviews, if they even occur, are a joke and this will not get any more scrutiny than what my group places on it. This is partly why I am doing what I'm doing with his code. I want to make sure what we create with it is as good as possible. –  JasonAlun Jul 9 at 19:13

a way of 'genericizing' information from a database so that all his web application's drop-down lists can share the same object in his MVC.NET C# code and Views, which can contain different data depending on what tables it is being used against.

So, we take a DB schema that generally maps to the problem space at hand, and a user UI that generally maps to the problem space at hand, then create a data transport mechanism between the two that generally maps to nothing. Hmmmm.

His justification is that he can send all his various drop down data and capture them all, regardless of content, in the same C# object named "LookupValue."

Where's the design? We use a common class to fill dropdown lists - but it is a UI form/control with properties for customizing SQL, setting the list/collection itself, tying the list to the dropdown UI element, etc. Being a UI thing it gives common behavior as well.

LookupValue

  • The comments are banal, obvious, unnecessary
  • I see numerous violations of Open/CLosed principle in your future. My spidey sense says all those "this ID" and "that id" parameters, and that they are repetitive across constructors, is suspicious somehow. I wish I could be more specific.... I'd bet there are customized stored procedures for each constructor.
  • "Equateablitly" implementation is incomplete for the class. I'm not confident that the inherited, not over-ridden, Object.Equals() will return the same result as the IEquatable implementation. From MSDN: "If you implement IEquatable, you should also override the base class implementations of Object.Equals(Object)"
  • Is there a LookupValue collection? If not, why implement IEquateable? Again, from MSDN: "The IEquatable interface is used by generic collection objects ... when testing for equality. ... It should be implemented for any object that might be stored in a generic collection. "

I forgot to mention that we are using Entity Framework as an ORM, and his Repository classes in his DAL are dumping the data from the stored procs into this LookupValue object.

So he is willfully subverting the application architecture. Does he have approval for that? Are y'all purposefully getting away from the ORM? Hmmmm.

How badly does this blow your inherent data binding? How much extra code do you write for that?

The deeper issue for me however is that he is taking some of the data, concatenating it in his stored procedure like this ...

So much for arguing re-usability. You are right to call it "a deeper issue."

  • I recall having to write whole new stored procedures because of this. And these were virtually identical save for that special customization. One of these procedures "decided" that all blank/null numbers were zero - sounds reasonable, clearly a DB only issue. That did not survive the next requirements cycle!
  • How is that concatenation parsed on the receiving end? I bet it's not even a class. IMHO, any string that has imbedded coding/meaning should be a class unto itself so that the meaning is properly expressed and encapsulated.

My colleague has developed a very complex security scheme whereby people can be granted access to data based on their permission level which can be assigned by Region, VISN, Site, or any mixture of the three. It's a nice scheme and very clever. However, he has stored procedures that return lists of Regions, VISNs, and Sites based on a person's User Id

So he's willfully subverting the application architecture. (see? I re-used that sentence!). I mean, I assume y'all are taking advantage of the database's built-in security and access features. So now you have to keep these in synch!

In Conclusion

the re-usability of this object is worth the violation of Separation of Concerns and ...

Re-useability is not a principle - it is a consequence - of good object oriented design and coding.

He has woven a tapestry of un-maintainability and anti-design around a single class that is "re-usable." This is clearly not a balanced application of OO principles and guidelines.

Why would someone perpetrate such obscureata? Psycodeologists say they get a sense of superiority from creating something others do not understand. You may disprove this simply by showing me the documentation. What, no documentation? Hmmm.

share|improve this answer
    
why the negativity? –  radarbob Jul 10 at 14:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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