current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

Sometimes I have to write big code in a class, so what I do is something like this,

Class ABC   //it's a web service class
{
   Public void Method-1() //used "-" for easy to read
   {
       //DoSomething and get something from database
       Method-2(pass parameters that i got from database);  
   }

   Public void Method-2(parameters)
   {
       DoSomething again and get data from another database. and some other source
       do some processing by calling web services (just as example)
       Method-3(parameter);
   }

   Public void Method-3(parameters)
   {
       DoSomething again and get data from another database. and some other source
       do some processing by calling web services (just as example)
       Method-4(parameter);
   }

   // and it keeps going
}

Another way

Class ABC   //it's a web service class
{
   Public void Method-1() //used "-" for easy to read
   {
       Method-2();
       Method-3();
       Method-4();
       // so on....
    }
}

Is this the right way of doing it and if not then what would be best way of doing it ?

share|improve this question

closed as off-topic by Nikita Brizhak, Lstor, palacsint, svick, Jamal♦ Aug 29 '13 at 12:01

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

  • "Questions must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After your code is working you can edit this question for reviewing your working code." – Nikita Brizhak, palacsint
If this question can be reworded to fit the rules in the help center, please edit the question.
1  
This question appears to be off-topic because it is about a design question. The question belongs on Programmers.SE. –  Lstor Aug 29 '13 at 9:51
    
This question appears to be off-topic because it is about example code. Code Review questions have to contain real code. –  svick Aug 29 '13 at 11:07
add comment

2 Answers

up vote 2 down vote accepted

Second approach seems to be more readable than the first one. In general I would like to write each method to have below four steps.

  1. Validate the parameters
  2. Retrieve the data
  3. Perform the business logic using the retrieved data
  4. Return the results
share|improve this answer
add comment
up vote 0 down vote

Think about how client code will call your class. first approach is not so good:

  • client class know a lot about internals
  • every client class would need to write or copy\paste the same code again and again.
  • if method signatures will change, every usage would need to be updated.
  • it is easy for you, or even another team member to pass incorrect arguments to public methods
  • public methods could change or even break internal state so that class will not be usable anymore

Not good:

      public static void Main()
      {
          var processor = new SuperDataProcessor();
          processor.Initialize();
          var parameters = processor.GetParametersFromDatabase(connectionString);

          var data1 = processor.GetDataFromFirstDb(parameters.FirstDatabase /* what if we pass incorect value here? i. e. SecondDatabase instead of first?*/);
          var data2 = processor.GetDataFromSecondDb(parameters.SecondDatabase);

          var finalResult = processor.MergeDataOnWebService(parameters.WebUrl, data1, data2);
          Console.WriteLine(finalResult);
      }

this one is better:

        // lets simplify job for someone who will use this processor. No internal details exposed.
        public static void Main()
        {
            var processor = new SuperDataProcessor(string connectionString);
            var result = processor.GetParametersFromDatabaseAndMergeOnWebService();
            Console.WriteLine(result);
        }

Streamlined class:

        public class SuperDataProcessor
        {
            public SuperDataProcessor(string connectionString)
            {
                _connectionString = connectionString;
            }

            public MyData GetParametersFromDatabaseAndMergeOnWebService()
            {
                // now we have smarter initialization and place for potential error handling - all in one place.
                if (!_initialized) {
                    processor.Initialize();
                }

                var parameters = GetParametersFromDatabase(_connectionString);

                var data1 = GetDataFromFirstDb(parameters.FirstDatabase);
                var data2 = GetDataFromSecondDb(parameters.SecondDatabase);

                var finalResult = MergeDataOnWebService(parameters.WebUrl, data1, data2);
                return finalResult;
            }

            private Parameters GetParametersFromDatabase(string connectionString)
            {
            }

            private MyData1 GetDataFromSecondDb(string secondDatabase)
            {  
               // only get data from second db
            }

            private MyData2 GetDataFromFirstDb(string firstDatabase)
            {
               // only get data from first db
            }

            private void MergeDataOnWebService(string webUrl, MyData1 data1, MyData2 data2)
            {
              // only merge and return merged results
            }

        }
share|improve this answer
add comment

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