current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I need help identifying any significant design issues with some code I have generated to track status updates.

I have a big task A which contains multiple small tasks and each of these small tasks updates the status of A in the data store.

This is how I've implemented: For each type of Task A(A can be of multiple types), I'll create an enum class implementing the interface StatusTask given below.

And then, I'll call StatusTask.Chain.start(starting enum object, param);

public interface StatusTask<T, V extends Enum<V> & StatusTask<T,V>>
{   
    public V execute(T t) throws Exception;

    public void fail(Exception e) throws Exception;

    public static class Chain
    {
        public static <T,V extends Enum<V> & StatusTask<T,V>> void start(V v, T t)
        {
            V n = null;
            try
            {
                n = v.execute(t);
            }
            catch(Exception e)
            {
                v.fail(e);
            }

            if(n == null || n == v)
            {
                return;
            }

            start(n, t);
        }
    }
}

Do you see any significant flaws with this design?
Is there a better design to handle such a scenario?

Example Scenario: Fetching a file and processing it. Following are the task-status: (0. Initial task. Move to Task 1.)

  1. Send Request and mark status as Sent Request
  2. Successful Response Received and mark status as Response Received and move to task 4
  3. Error Response Received and mark status as Response Received
  4. If file present, then mark status as Processing and execute step 4 else step 5
  5. Mark status as File not available and exit
  6. Parse file. If no error, goto step 7 else step 8
  7. Mark status as File parsing succeeded and do something.....
  8. Mark status as File parsing failed and exit
share|improve this question

migrated from programmers.stackexchange.com Nov 14 '14 at 17:45

This question came from our site for professional programmers interested in conceptual questions about software development.
1  
Welcome to Code Review! Very sorry for not having given your question more attention. Could you include an actual usage example of when you're using this design? Right now it's very abstract to read only the interface of it. –  Simon Forsberg♦ Dec 15 '14 at 13:16

2 Answers 2

up vote 1 down vote

In addition to what unholySampler has stated, I would like to add a few:

  • First of all, you are catching all generic exceptions which implies you wish to be on the safe side to avoid any failure which might conceal errors that were not thought of. Even if you intend to do so, atleast handle the exceptions in fail() and throw specific exceptions (coudl be RunTimeException other than just Exception.

    Reason : If someone wishes to retry a particular task exception( say timeout), he could do so based on the exception. So have exception such as RetryableException ,NonretryableException,etc. thrown from fail method with user-understandable message.

  • Not sure if I can make sense with this approach but instead of having a return we could instead have:

    if(n != null && n != v){
    start(n, t);
}

  • Can v be null?

  • Can t be null and if yes, is it to be handled by execute()? I would rather have conditions to terminate here rather than in the functions because execute should be intended at completing a valid task. For the last task which won't request any new task, we could handle it in the current method itself.
  • Lastly, a good documentation helps in faster debugging and swift understanding of the code.
share|improve this answer
    
1) Should handle exceptions. Its a primary design. Need to refine it. 2) Forgive my lack of knowledge, but isnt it the same. I mean both achieve the same purpose right? 3) No. Need to handle it. Thanks 4) Nice idea.. Will change it. Also refer my comments on unholysampler post. Thanks for the suggestions –  bin Dec 29 '14 at 15:13
    
1. We should handle exceptions which we know can occur say NumberFormatException when we call sampleObject.getValue() but if we catch Exception, we might miss out on cases where we might have passed illegal argument or could be a NullPointerException etc. –  thepace Dec 29 '14 at 17:43
    
2. Its the same (that's the reason I wrote not sure). I try avoiding returns in between if possible. I hope I have answered your queries. –  thepace Dec 29 '14 at 17:45
up vote 2 down vote

Single letter variable names are highly discouraged. A variable name should give a reader an idea about what value is stored in it. n, v, and t tells a reader nothing. v and n are the same type, but how would someone who hasn't seen the code before know which means what.

You are checking if n is null. The only way this can happen is if an exception is thrown in execute() or if execute() returnsnull`. In the first case, you can directly return from that catch block. Doing this will more clearly indicate that an exception will stop execution of the chain.

In the second case, that means that null is a valid return value from execute(). However, execute() returns an enum, which is a way to explicitly state all valid values. If null is a valid value, it would just be another state that should be explicitly stated.

The code doesn't have any documentation. While the interface is fairly simple, it does assume that a reader interpreters the signatures properly.

  • fail() takes an Exception as an argument. But what should the implementer do with this exception. The method can also thrown an exception, does that mean a valid implementation could be the following?

    public void fail(Exception ex) throws Exception {
  throw ex;
}

  • If execute() returns the same enum value that it was passed, the chain of execution is stopped. This is extra logic that would not be clear to someone who hasn't seen the implementation. Since there is no documentation, someone needing to use this code has to look at the code to know that this is the way to stop execution.

share|improve this answer
    
Extremely sorry for the bad coding style as I thought the description should suffice. This is just an example of what I was planning to implement and not the actual code. It will definitely have a lot of documentation. –  bin Dec 29 '14 at 14:59
    
I agree with the second part. Need to fail fast on exception –  bin Dec 29 '14 at 15:00
    
Third part - fail can be another task or do nothing. It should be left to the user on how he would like to handle failure. The same enum means that the status of the task has not changed, hence it can exit instead of going to an infinite loop. –  bin Dec 29 '14 at 15:03
    
We have a lot of use-cases where such a structure if given to the task handling part would benefit all the developers and result in better code. That is the reason for this humble attempt. It is too generic but I thought I could always extend this design. And last but not the least, thanks for the advice. –  bin Dec 29 '14 at 15:11

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.