3
\$\begingroup\$

I have a method that is responsible for receiving an object and perform the information saved with Entity Framwork, using the Repository pattern. The issue is that this object is, basically, a DTO. The information contained in this object must be assigned to the models managed by the data layer, which is a bit confusing to me with respect to the way of making said assignments, considering that there are conditions for some of them.

public int RegisterParticipant(ParticipantDTO participant)
{
    try
    {
        EventoParticipante participante = new EventoParticipante();
        if(participant.Token != null)
        {
            var result = RegistrarPagoEnLinea(participant);
            participante.PagoEnLinea = true;
        }
        else
        {
            participante.PagoEnLinea = false;
            participante.TransaccionFecha = DateTime.Now;
            participante.TransaccionCodigo = participant.PaymentRefNumber;
        }
        participante.PersonaId = CreateNaturalPerson(participant).Id;
        participante.InstitucionId = short.Parse(CreateInstitution(participant).ToString());
        participante.EventoId = participant.EventId;
        participante.TipoParticipanteId = _attendeeTypeRepository.SingleOrDefault(x => x.Nombre == participant.TypeParticipant).Id;                
        participante.MontoPago = decimal.Parse(participant.Ammount);
        participante.Asistio = false;
        participante.FechaRegistro = DateTime.Now;
        participante.MontoPago = decimal.Parse(participant.Ammount) - 100;
        _eventAttendeeRepository.Add(participante);
        _eventAttendeeRepository.SaveChanges();
        Evento eventParticipant = _eventRepository.Get(participant.EventId);

        if (participant.CompanionName != null && !string.IsNullOrEmpty(participant.CompanionName))
        {
            Persona companion = new Persona();
            companion.Nombres = participant.CompanionName;
            companion.Apellidos = participant.CompanionLastName;
            companion.NumeroDocumento = participant.CompanionIdentity;
            companion.Ciudad = participant.AttendeeCity;
            companion.Activo = true;
            companion.UsuarioCreacion = "demo";
            companion.UsuarioModificacion = "demo";
            companion.FechaCreacion = DateTime.Now;
            companion.FechaModificacion = DateTime.Now;
            _personRepository.Add(companion);
            _personRepository.SaveChanges();

            EventoParticipante companionParticipant = new EventoParticipante();
            companionParticipant.PersonaId = companion.Id;
            companionParticipant.EventoParticipanteId = participante.Id;
            companionParticipant.EventoId = participante.EventoId;
            companionParticipant.MontoPago = decimal.Parse((100.00).ToString());
            companionParticipant.TipoParticipanteId = 4;
            companionParticipant.InstitucionId = participante.InstitucionId;
            companionParticipant.Asistio = false;
            companionParticipant.FechaRegistro = DateTime.Now;
            _eventAttendeeRepository.Add(companionParticipant);
            _eventAttendeeRepository.SaveChanges();
        }

        if (participant.InvoiceIdentity != null && !string.IsNullOrEmpty(participant.InvoiceIdentity))
        {
            CreateInvoice(participant, participante);
            MailHelper.SendMail(MailAction.EventRegisterSuccessWithInvoice, eventParticipant, participant, _countryRepository.Get(participant.AttendeeCountryId).Nombre, _countryRepository.Get((int)participant.InvoiceCountryId).Nombre);
        }
        else
            MailHelper.SendMail(MailAction.EventRegisterSuccess, eventParticipant, participant, _countryRepository.Get(participant.AttendeeCountryId).Nombre);
        return 0;
    }
    catch (Exception e)
    {
        return 1;
    }
}

As you can see, the method is excessively long. I understand that a method should be in charge of a specific function, so I know that it is wrong from that point.

To try to diminish its extension (and the possibility of errors in it) I have separated certain assignment tasks into methods within the same class, as in the case of CreateNaturalPerson (yes, the method was even longer). I thought to do the same with the IF where the values are assigned to Persona and EventoParticipante, but I think it would be to fall into the same. Is there any design pattern or advice for the above mentioned?

\$\endgroup\$
  • \$\begingroup\$ You call SaveChanges twice. What happens if the first call succeeds and the second fails? Your db would be left in an invalid state. Also, using return codes is not a c# convention. Just let exceptions be thrown and caught by a global handler. Wrapping huge chunks of code in try/catch without logging the exception will just silently mask them, which is really bad. \$\endgroup\$ – Brad M Jul 29 '18 at 12:07
  • \$\begingroup\$ Thanks, @BradM. I understood that SaveChanges saved the data in the database for each context. I considered that, having an instance of my main Repository class for each model of my database, I should make the call each time I made an Add. In addition, I had to obtain the ID of the saved item in order to assign it to a property of the following models. Is there any way to do it in a more effective way and considering everything you tell me? \$\endgroup\$ – Adolfo F. Ibarra Landeo Jul 29 '18 at 22:36
  • 1
    \$\begingroup\$ Why check for null and then string.IsNullOrEmpty: if (participant.CompanionName != null && !string.IsNullOrEmpty(participant.CompanionName))? string.IsNullOrEmpty already include the null check. \$\endgroup\$ – BCdotWEB Jul 30 '18 at 13:06
1
\$\begingroup\$

This method does too much:

  • creates EventoParticipante
  • creates Persona for the companion if necessary
  • creates EventoParticipante for the companion if necessary
  • CreateInvoice
  • SendMail

Each of the first three should be a method or even a class of their own (especially considering each process seems to be fairly complicated), and then this RegisterParticipant method should perhaps be a class of its own to combine the individual parts as a process.

This one method is 60+ lines. That is already hard to keep an overview of, and it makes me wonder what other methods this class contains. Focus on making your methods simpler, apply SOLID,...


The mix of English and Spanish(?) as class names and method names and variable names is IMHO a bad idea.


Why do you do this: decimal.Parse((100.00).ToString());?

Ditto: short.Parse(CreateInstitution(participant).ToString());: why not simply have CreateInstitution return a short?

In the same vein: decimal.Parse(participant.Ammount);. Why isn't Ammount a decimal? (BTW this is spelled incorrectly, the correct word is "Amount".)

Such odd castings are all over your code, see also: (int)participant.InvoiceCountryId.


This will throw an exception if _attendeeTypeRepository cannot find anything:

_attendeeTypeRepository.SingleOrDefault(x => x.Nombre == participant.TypeParticipant).Id; 

Is that something you want?


Don't do this:

MailHelper.SendMail(MailAction.EventRegisterSuccessWithInvoice, 
   eventParticipant, 
   participant, 
   _countryRepository.Get(participant.AttendeeCountryId).Nombre, 
   _countryRepository.Get((int)participant.InvoiceCountryId).Nombre);

Even though it's only five parameters (which is already an amount that is bordering on my upper limit before I'd replace this with a custom class), at least two of these are the result of returns from other methods, and now you have a single line that is 250+ characters. This makes your code hard to read and follow.

Considering that you already pass participant, you could easily make a method that only accepts MailAction and participant, gets the other data itself, and then calls MailHelper.SendMail.


What does "4" mean in this context: companionParticipant.TipoParticipanteId = 4;? Avoid unexplained magic numbers: give them proper names or at least add a comment.

\$\endgroup\$

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.