3
\$\begingroup\$

Before I go too far, does this look right?

This is mostly to have a "central" place that hold a "hierarchical/structured" errors message.

public static class ValidationError
{
 static ValidationError()
 {
 Prefix = "VE";
 Notice = new _Notice();
 Exporter = new _Exporter();
 Importer = new _Importer();
 Shipping = new _Shipping();
 //few others one
 }
 public static string Prefix { get; private set; }
 public static _Notice Notice { get; private set; }
 public class _Notice
 {
 public _Notice()
 { 
 Prefix = ValidationError.Prefix + "NO";
 MustHaveOneTreatmentOption = Prefix + "001";
 }
 public string Prefix { get; private set; }
 public string MustHaveOneTreatmentOption { get; private set; }
 }
 public static _Exporter Exporter { get; private set; }
 public class _Exporter
 {
 public _Exporter()
 { 
 Prefix = ValidationError.Prefix + "EX";
 Mailing = new Address(Prefix, "MA");
 Physical = new Address(Prefix, "PH");
 }
 public string Prefix { get; private set; }
 public Address Mailing { get; private set; }
 public Address Physical { get; private set; }
 }
 public static _Importer Importer { get; private set; }
 public class _Importer
 {
 public _Importer()
 { 
 Prefix = ValidationError.Prefix + "IM";
 }
 public string Prefix { get; private set; }
 }
 public static _Shipping Shipping { get; private set; }
 public class _Shipping
 {
 public _Shipping()
 { 
 Prefix = ValidationError.Prefix + "SH";
 }
 public string Prefix { get; private set; }
 }
 public class Address
 {
 public Address(string _Prefix, string _Suffix)
 {
 Prefix = _Prefix + _Suffix + "AD";
 }
 public string Prefix { get; private set; }
 }
}
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Apr 1, 2014 at 12:10
\$\endgroup\$
3
  • 4
    \$\begingroup\$ It will help if you stated the objectives you are aiming to achieve with this code and give it some context so people can try and provide the most appropriate solution. In the meantime, you could check out FluentValidation which can help you structure your validation. \$\endgroup\$ Commented Apr 1, 2014 at 13:19
  • \$\begingroup\$ The code doesn't make much sense to me, what is it supposed to do? All I can see is a static class with a few properties that are effectively constant \$\endgroup\$ Commented Apr 1, 2014 at 13:26
  • \$\begingroup\$ To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. \$\endgroup\$ Commented Apr 1, 2014 at 19:38

2 Answers 2

7
\$\begingroup\$

Naming

Class names in C#, whether they're for a static, non-static, internal, public, private, abstract, nested or on-its-own class, should be PascalCase.

Find the intruder:

public class _Notice
public class _Exporter
public class _Importer
public class _Shipping
public class Address

Only Address is correct!

Keep the underscore prefix for private fields, it seems you're using prefixes only to avoid name clashes - this smells, because in naming what's important is consistency - using arbitrary prefixes is bad practice.

Also, it's possible the nested types are not warranted, but it's very hard to tell without seeing how you're using this ValidationError static class.

answered Apr 1, 2014 at 14:19
\$\endgroup\$
0
2
\$\begingroup\$

Non-static static classes
Your inner classes (_Notice, _Exporter, etc.) don't seem to have any use-case where their instances have independent meaning - their state is "hard-coded" on instantiation, and are read-only. Why don't you just declare them as static, just like their parent?

Instead of:

public static _Notice Notice { get; private set; }
public class _Notice
{
 public _Notice()
 { 
 Prefix = ValidationError.Prefix + "NO";
 MustHaveOneTreatmentOption = Prefix + "001";
 }
 public string Prefix { get; private set; }
 public string MustHaveOneTreatmentOption { get; private set; }
}

Simply declare:

public static class Notice
{
 static Notice()
 { 
 Prefix = ValidationError.Prefix + "NO";
 MustHaveOneTreatmentOption = Prefix + "001";
 }
 public static string Prefix { get; private set; }
 public static string MustHaveOneTreatmentOption { get; private set; }
}

This gives you a few advantages:

  • The awkward class name issue is solved
  • Usage is still the same
  • No need to worry about someone making instances of your classes (since you made their constructors public...)

Readonly properties
This is also recurring in your code - property getters for members which are initialized only upon instantiation. If you replace them with readonly members, you can make your code more succinct:

public static class Notice
{
 public static readonly string Prefix = ValidationError.Prefix + "NO";
 public static readonly string MustHaveOneTreatmentOption = Prefix + "001";
}
answered Apr 1, 2014 at 18:12
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Nice answer! ...but public static class exposing only public static readonly fields? At that point one might as well put them in a configuration file! \$\endgroup\$ Commented Apr 1, 2014 at 19:00
  • \$\begingroup\$ @MatsMug I suspect that is about what the OP had in mind when he wrote this... A C# style DSL... \$\endgroup\$ Commented Apr 1, 2014 at 19:49

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.