4
\$\begingroup\$

I am going to undergo a code review shortly at work. I feel that my code is too verbose, could log more sensibly, and I feel that my if/else statements are somewhat duplicative/repetitive and can be done better. But I can't seem to be able to put my finger on what I am missing. How would you clean this up such that the code isn't verbose, logs sensible stuff and doesn't look kiddish. I would also like to make it simpler, in case additional filters need to be added in future.

FYI - getDetails method used in the following code, merely contains code for constructing a customer specific "Details" object that contains just 2 things, the name of the customer and a corresponding schedule for invocation.

    /* Filters out invokes that havent reached a size threshold.*/
    private boolean filterBySize(CustomerUnit Customer) {
        CustomerData customerData = new CustomerData(Customer);
        if (customerData.getUsage() > USAGE_THRESHOLD) {
            LOGGER.info("Customer's data usage of " + customerData.getUsage()
                    + "is higher than threshold of " + USAGE_THRESHOLD + " percent for " + Customer.getId());
            return true;
        } else {
            LOGGER.info("Customer's data usage of " + customerData.getUsage()
                    + "is lower than threshold of " + USAGE_THRESHOLD + " percent for " + Customer.getId());
            return false;
        }
    }

    /* Filters out invokes that aren't slated for today.*/    
    private boolean filterByDate(CustomerUnit Customer) {
        if (getDetails(Customer).getDOW().equalsIgnoreCase(LocalDate.now().getDayOfWeek().name())) {
            LOGGER.info("Customer : " + getDetails(Customer).getName() + " is invoked to run import today");
            return true;
        } else {
            LOGGER.info("Customer : " + getDetails(Customer).getName() + " isn't invoked to run import today, ");
            return false;
        }
    }

    /* Applies aforesaid filters */
    private void invokeRequestByCustomer(CustomerUnit Customer) throws InterruptedException, UnexpectedDateOrderException {
        if (filterByDate(Customer) && filterBySize(Customer)) {
            invoker.invoke(getDetails(Customer));
            LOGGER.info("Scheduled Customer : " + getDetails(Customer).getName() + " to run import operations at" + getDetails(Customer).getDOW());
        } else {
            LOGGER.info("Customer : " + getDetails(Customer).getName() + " was filtered, no invoke will be executed for it");
        }
    }

As of now, I have thought about using an IfThenElse function, somewhat similar to the following: IfThenElse example

\$\endgroup\$
  • \$\begingroup\$ I understand marking it down - but it would help me if you were to tell me the reason for doing so. I went through the guidelines and asked my question as per my understanding of those guidelines. \$\endgroup\$ – NotACodeNinja Feb 16 at 8:34
  • 4
    \$\begingroup\$ The close vote is for needing detail / clarity. This may be related to your title, which doesn't reflect what the code is trying to achieve. "Filter customers by date and size" may be a better description of what the codes doing. \$\endgroup\$ – forsvarir Feb 16 at 9:27
  • 3
    \$\begingroup\$ Next time, please post a complete class (including the import declarations) and provide interfaces (or classes) for all mentioned types, to allow us reviewers to paste the code into our IDE and be ready to try it out. If you had done this for this question, you might have received some helpful tips for using the logging framework more efficiently. \$\endgroup\$ – Roland Illig Feb 16 at 12:24
  • \$\begingroup\$ Thanks for your comments - I understand. My initial thought was that it might become too much text for the review, but I guess it is helpful if it provides clarity and context. It would be nice if I could just attach the files with the post and only post the textual description of what I am trying to focus on, with some way of displaying the attached files on the side, if needed anyway. \$\endgroup\$ – NotACodeNinja Feb 16 at 22:03
  • 1
    \$\begingroup\$ We're not easily scared and our character limit is over 65k characters. Next time, don't be afraid to show us more context. \$\endgroup\$ – Mast Feb 17 at 12:30
5
\$\begingroup\$

The comments seem redundant, I'm assuming they're just for this review, if not, I would remove them. The basically just duplicate what the code says.

Your function names seem misleading. filterBySize seems to be filtering by data usage. filterByDate seems to be filtering by the day of the week. It would be better if they said what they did.

Your filtration functions contain two branches, both of which return. Generally you would only wrap the if side. Everything that doesn't trigger the if would be the else...

CustomerData customerData = new CustomerData(Customer);
if (customerData.getUsage() > USAGE_THRESHOLD) {
    return true;
}
// The else is implicit... so just noise
return false;

For me, there's an awful lot of logging across these methods. Maybe that's what's required by your processes, but it seems excessive. We don't have the context for how the code is going to be called, however I would expect that 'USAGE_THRESHOLD' is the same for every invocation, do you really need to log it for each custom to say if they are above/below it? The current day of the week is the same for each invocation, again do you really need to output it for every customer that's selected? Each of your filters logs the specific reason that the customer has been rejected do you need to log the generic message at the top level saying they are being skipped. If you're looking for reuse, then maybe your filterByDate method shouldn't mention the import, that way it could be reused to filter other things (export perhaps) in the future...

As an aside, I'm not sure about this:

CustomerData customerData = new CustomerData(Customer);

It makes me wonder what the constructor for CustomerData is doing... If the information needed for the filtration isn't in the Customer, then is it reading that extra data in the the constructor for CustomerData? That seems wrong...

Another small issue is naming.... variables should start with lower case in java.... CustomerUnit Customer should be CustomerUnit customer, you can see it's confusing the code preview...

It would also be interesting to see how invokeRequestByCustomer is being called... It looks a lot like it could fit into a stream something like:

allCustomers.stream()
    .filter(CustomerFilters::byUsage)
    .filter(CustomerFilters::byDay)
    .foreach(c->invoke(c));
|improve this answer|||||
\$\endgroup\$
5
\$\begingroup\$

I'm agree with considerations by @forsvarir and I'm writing a possible example on how to simplify your messages, you have the following method:

private boolean filterBySize(CustomerUnit Customer) {
    CustomerData customerData = new CustomerData(Customer);
    if (customerData.getUsage() > USAGE_THRESHOLD) {
       LOGGER.info("Customer's data usage of " + customerData.getUsage()
                   + "is higher than threshold of " + USAGE_THRESHOLD + " percent for " + Customer.getId());
       return true;
     } else {
       LOGGER.info("Customer's data usage of " + customerData.getUsage()
                  + "is lower than threshold of " + USAGE_THRESHOLD + " percent for " + Customer.getId());
       return false;
     }
}

The two messages differs just for the use of word "lower" in one branch and "higher" in the other: in this case you can rewrite your method using a ternary operator and String.format:

private boolean filterBySize(CustomerUnit Customer) {
    CustomerData customerData = new CustomerData(Customer);
    String template = "Customer's data usage is %s than threshold of %f percent for %d";
    boolean condition = customerData.getUsage() > USAGE_THRESHOLD;
    String s = condition ? "lower" : "higher";
    LOGGER.info(String.format(template, s, customerData.getUsage(), Customer.getId()));
    return condition;
}

I don't know the exact types of your data so I used %f and %d inside String.format and I chose to preserve the name Customer for the parameter for this example, but as highlighted by @forsvarir change it to lowercase.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I like your inputs as well. It is just that @forsvarir had a suggestion about using the stream which I found quite useful in addition to the inputs about the methods. Appreciate your help still though. \$\endgroup\$ – NotACodeNinja Feb 18 at 20:03
  • \$\begingroup\$ @NotACodeNinja You are welcome, if you have other questions post them here in the Code Review site. \$\endgroup\$ – dariosicily Feb 18 at 22:04

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.