0
\$\begingroup\$

I will not go into depth as for how Czech Birth number should exactly look like. It is kind of similar to insuarance number, example:

Let's say a valid Birth number may be e.g.:

6854161578

, where:

  • 68 signifies year 1968
  • 54 signifies month 04 (April) and +50 in it means it belongs to a woman
  • 16 signifies day 16
  • 1578 should for one ensure limited amount of the numbers generated by day
  • 1578 is arbitrary number ensuring, among other things, the Birth number is unique and divisible by 11, it is fixed to 4 digits

There are a few rules to it, but the whole number for simplicity must be divisible by 11. And there can't obviously be 2 people with the same Birth number :)

I am in process of re-writing such application in C# with Visual Studio and .NET 4.0 for checking its validity.

An excerpt follows:

First some error exceptions:

public class InputEmpty : Exception
{
}


public class InputNotInteger : Exception
{
}

Then just a beginning of my new class, should be self-explanatory:

public class BirthNumber
{


    // this holds the original ARawBirthNumberString given to the constructor
    private string FRawBirthNumberString;


    // this holds the original ARawBirthNumberString given to the constructor without delimiters
    private string FSanitizedBirthNumberString;


    // private read-only property for FBirthNumberString
    private string RawBirthNumberString
    {
        get
        {
            return FRawBirthNumberString;
        }
    }


    // private read-only property for FBirthNumberString
    public string SanitizedBirthNumberString
    {
        get
        {
            return FSanitizedBirthNumberString;
        }
    }


    // contructor
    public BirthNumber(string ARawBirthNumberString)
    {


        // store exactly what we received as input from the constructor
        FRawBirthNumberString = ARawBirthNumberString;


        // ensure there are no starting and/or trailing spaces around the string
        FSanitizedBirthNumberString = FRawBirthNumberString.Trim();


        // remove some common characters from the string: \, ,/
        FSanitizedBirthNumberString = RemoveCharFromString(FSanitizedBirthNumberString, ' ');
        FSanitizedBirthNumberString = RemoveCharFromString(FSanitizedBirthNumberString, '/');
        FSanitizedBirthNumberString = RemoveCharFromString(FSanitizedBirthNumberString, '\\');


        // check if the string is empty, and in that case, return with error
        if (string.IsNullOrEmpty(FSanitizedBirthNumberString))
            throw new InputEmpty();


        // declare local variable BirthNumber for temporary use
        long BirthNumber;


        // check if the string contains an integer, and if not, return with error
        if (!Int64.TryParse(FSanitizedBirthNumberString, out BirthNumber))
            throw new InputNotInteger();


    }

    private int RawYear
    {
        get
        {
            return Convert.ToInt32(RawBirthNumberString.Substring(0, 2));
        }
    }

    private int RawMonth
    {
        get
        {
            return Convert.ToInt32(RawBirthNumberString.Substring(2, 2));
        }
    }

    private int RawDay
    {
        get
        {
            return Convert.ToInt32(RawBirthNumberString.Substring(4, 2));
        }
    }

    public int Year
    {
        get
        {


            // define local variable to work with, and in the end use it as return value
            int Year;


            if (RawBirthNumberString.Length >= 9)
            {


                switch (RawBirthNumberString.Length)
                {

                    // in case the the birth number is exactly 10 numbers long
                    case 10:
                        {


                            if (RawYear <= 53)
                            {

                                // if the RawYear is less or equal 53,
                                // we consider that a 20th century

                                Year = 2000 + RawYear;

                                DateTime Now = DateTime.Now;

                                bool YearValid = (Year <= Now.Year);

                                if (!YearValid)
                                    throw new YearDoesNotExistYet();

                                return Year;

                            }

                            else
                            {

                                // if the RawYear is greater than 53,
                                // we consider that a 19th century

                                Year = 1900 + RawYear;

                                return Year;

                            }

                        }


                    // in case the the birth number is exactly 9 numbers long
                    case 9:
                        {


                            Year = 1900 + RawYear;

                            bool YearValid = (Year <= 53);

                            if (!YearValid)
                                throw new NineNumbersLongExistUpToYear1953();

                            return Year;


                        }


                    // in case of other lengths
                    default:
                        {

                            return 0;

                        }


                }

            }
            else return 0;


        }

    }

}

The usage of this short form would follow like that:

    try
    {

        // create an instance of an object BirthNumber
        BirthNumber MyBirthNumber = new BirthNumber("6854161578");

        // for this example I coded only a Year extraction, so...
        int MyYear = MyBirthNumber.Year;

        EBirthNumber.Text = MyBirthNumber.SanitizedBirthNumberString;

        EBirthNumber.Background = Brushes.Lime;

    }
    catch (InputEmpty)
    {

        SetStatus("Input field is empty.", Brushes.Red);

    }
    catch (InputNotInteger)
    {

        SetStatus("Birth number consists only of numbers (and a possible slash or spaces)", Brushes.Red);

    }

I shortened the code as much as I could only for you to get an idea of my approach. Some missing functions like SetStatus should also be self-explanatory in favor of you not having to dig through all the code. i hope it is sufficient and if not, feel free to notify me.


I am new to Visual Studio, C# and .NET. I hope you can give some guidance.

Any recommendations at this point?

\$\endgroup\$

closed as off-topic by t3chb0t, paparazzo, Graipher, Jamal Jul 30 '17 at 18:01

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I don't like all that spacing. Don't understand "1578 should for one ensure limited amount of the numbers generated by day". \$\endgroup\$ – paparazzo Jul 30 '17 at 14:22
  • 2
    \$\begingroup\$ Yes, spaces drive me crazy too... \$\endgroup\$ – Maxim Jul 30 '17 at 14:41
  • 2
    \$\begingroup\$ Down on the spaces! Well, maybe... It's okay to have separate spaces between the declaration block and each action block (calculation, search, etc.); however, it's a little silly to have spacing between each and every declaring variable, each and every action, and every curly brace. \$\endgroup\$ – Sometowngeek Jul 30 '17 at 17:10
  • 1
    \$\begingroup\$ I shortened the code as much as I could - this usually ends very badly and leads to answers that are not helpful or have a little value because you'll probably get suggestions to improve your code in ways that you already have done. My advice is to post your real code, not any shortened one. In addition to that I vote-to-close this question as off-topic because reviewing a shortened version is pointless. \$\endgroup\$ – t3chb0t Jul 30 '17 at 17:15
  • 1
    \$\begingroup\$ Vlastimil, check out this secret Gist It shows an example of how the code is usually laid out. \$\endgroup\$ – Sometowngeek Jul 30 '17 at 17:19
3
\$\begingroup\$

In general: do not insert an empty line after each line of code. It is really annoying :)
Also, so not comment lines that are self-explanatory. When you define a local variable, a comment that says you are defining a local variable is a waste of effort and space.

1) Make read-only fields read-only. Since they are set only in your constructor they are immutable:

 private readonly string FRawBirthNumberString;
 private readonly string FSanitizedBirthNumberString;

2) No need for private properties on private read-only fields. Just reference the fields.

3) Usually we use lower case characters to start parameters:

public BirthNumber(string aRawBirthNumberString)

4) Use built-in functions and don't waste code lines:

// trim and remove some common characters from the string: \, ,/
FSanitizedBirthNumberString = FRawBirthNumberString.Trim()
    .Replace(" ", string.Empty)
    .Replace("/", string.Empty)
    .Replace("\\", string.Empty);

5) Use expression bodies when they improve readability:

private int RawYear => Convert.ToInt32(FRawBirthNumberString.Substring(0, 2));
private int RawMonth => Convert.ToInt32(FRawBirthNumberString.Substring(2, 2));
private int RawDay => Convert.ToInt32(FRawBirthNumberString.Substring(4, 2));

6) Use var, it's easier. Also, don't declare extra variables like Now when you do not need them:

var YearValid = (Year <= DateTime.Now.Year);

Actually, you don't even need that variable.

In your Year getter you define a variable Year. Local variables should have small letter, but also, you mix up the local Year with your property Year - not a good idea!

7) Move a small else clause to the top to improve readability. Actually, that would lead to

if (FSanitizedBirthNumberString.Length < 9)
    return 0;

But that would be redundant because you return 0 anyway if the length is not 9 or 10.

Your getter could be this:

get
{
    switch (FSanitizedBirthNumberString.Length)
    {
        case 10:
            if (RawYear > 53)
                return 1900 + RawYear;

            int year = 2000 + RawYear;
            if (year > DateTime.Now.Year)
                throw new YearDoesNotExistYet();
            return year;

        case 9:
            if (RawYear > 53)
                throw new NineNumbersLongExistUpToYear1953();
            return 1900 + RawYear;

        default:
            return 0;
    }
}

}

Mark that I used FSanitizedBirthNumberString instead of the Raw version, which has a different length. Also, in the last ValidYear-check, I used RawYear instead of your original Year, because 1900 + something is never <= 53!

You could also move the logic form the getter to a separate method.

Alternatively, you can get rid of the switch completely, as @Paparazzi suggested in their comments. I guess it's a matter of taste which form you prefer:

get
{
    if (FRawBirthNumberString.Length == 9)
    {
        if (RawYear > 53)
            throw new NineNumbersLongExistUpToYear1953();
        return 1900 + RawYear;
    }

    if (FRawBirthNumberString.Length == 10)
    {

        if (RawYear > 53)
            return 1900 + RawYear;

        int year = 2000 + RawYear;

        if (year > DateTime.Now.Year)
            throw new YearDoesNotExistYet();

        return year;
    }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ I even think the switch is messy. The could just be else if. \$\endgroup\$ – paparazzo Jul 30 '17 at 16:11
  • 1
    \$\begingroup\$ @Paparazzi sure, but then you'd probably want to refactor even more to avoid a set of ugly nested if's (there are if's inside the switch as it is). I'm sure there are neater ways to code this logic, but I wanted to preserve the reasoning in the original to keep it recognisable. I was banging away on nice trinaries but that wouldn't increase readability, certainly not for a beginner. \$\endgroup\$ – oerkelens Jul 30 '17 at 16:52
  • \$\begingroup\$ @Paparazzi Switch and If/Else If/Else are essentially the same thing. Sometimes people find Switch makes factoring simpler to use than the If (gets rid of redundant expressions) \$\endgroup\$ – Sometowngeek Jul 30 '17 at 17:05
  • \$\begingroup\$ @Sometowngeek but there is already a if (FSanitizedBirthNumberString.Length < 9) so I would prefer the same \$\endgroup\$ – paparazzo Jul 30 '17 at 17:18
  • \$\begingroup\$ @Paparazzi I guess it's to each their own. I think Switch makes me happier because I can eliminate redundancy if i have to create more than 2 clauses (not including the Else / Default). \$\endgroup\$ – Sometowngeek Jul 30 '17 at 17:20

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