Validate the inputs and perform actions accordingly I have a consumer which is listening to Kafka and from which I am getting avro GenericRecord.

Now basis on GenericRecord payload, I am extracting lot of other things. clientId, deviceId, payId, crossId, appPayload.
And on all those things I have to do sanity check to make sure they are null/empty or not.
And if they are null/empty, then I want to increment the metric counter for that particular metric, log it and continue to the next record from the for loop.
And if they are not null/empty, then I am populating all those fields into a DataPacket builder. All the methods in DataPacket builder have a Preconditions.checkArgument check for null/empty string so I just cannot set them directly without checking them first. If I don't check before setting into DataPacket it will throw exception which will get caught in a catch block and then the whole program will get stopped and it won't move to next record from the for loop as you can see below.

try {
  GenericRecordDomainDataDecoder decoder = new GenericRecordDomainDataDecoder(config);
  while (true) {
    ConsumerRecords<byte[], byte[]> records = consumer.poll(1000);
    for (ConsumerRecord<byte[], byte[]> record : records) {
      DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
          MetricName.TOTAL_NUMBER);
      GenericRecord payload = decoder.decode(record.value());
      String clientId = (String) DataUtils.parseRecord(payload, "clientId");
      String deviceId = (String) DataUtils.parseRecord(payload, "deviceId");
      // can this be improved in any way? the whole if null check for all the below cases?
      if (clientId == null && deviceId == null) {
        DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
            MetricName.INVALID_ID);
        logger.logError("dropping records. invalid clientId and deviceId is coming.);
        continue;
      }
      String payId = (String) DataUtils.parseRecord(payload, "payId");
      if (payId == null) {
        DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
            MetricName.MISSING_PAY_ID);
        logger.logError("dropping records. invalid payId is coming.);
        continue;
      }
      String crossId = (String) DataUtils.parseRecord(payload, "crossId");
      Map<String, String> appPayload =
          (Map<String, String>) DataUtils.parseRecord(payload, "appPayload");
      if (MapUtils.isEmpty(appPayload)) {
        DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
            MetricName.INVALID_PAYLOAD);
        logger.logError("dropping records. invalid payload is coming.);
        continue;
      }
      DataMetricHolder.getInstance().increment(payId, MetricName.TOTAL_NUMBER);
      // can this be improved in any way?
      Builder packetBuilder = new DataPacket.Builder(payload);
      if (clientId != null) {
        packetBuilder.setClientId(clientId);
      }
      if (deviceId != null) {
        packetBuilder.setDeviceId(deviceId);
      }
      if (crossId != null) {
        packetBuilder.setCrossId(crossId);
      }
      DataPacket packet =
          packetBuilder.setPayId(payId).appPayload(appPayload).build();
      System.out.println(packet);
    }
  }
} catch (Exception ex) {
  // logging error
}

I wanted to see if there is any way to improve my code because right now it looks messy with the way I have all sorts of if null checks. Is there any better way to do the above things so that the code looks clean? I am on Java 7.

I think you have some great questions here!

You asked:

  Is there any better way to do the above things so that the code looks clean?

I'm not sure that's your only consideration here because it looks like this code is designed to be as efficient as possible. I say that because of the fact that the validation logic skips over the record as soon as a problem is found and the packetBuilder is only created after the initial validation - thereby avoiding the cost of creating the object every time through the loop regardless of the validity of the data.

So, with all of this in mind I'm afraid that my answer to your question will have to be: there might be slight improvements you can make but you can't avoid the null checking unless the Exception thrown by the builder is very detailed about what exactly went wrong and you can use it to gather your metrics.

If you want to make the code more modular you could split the validation logic into separate methods - perhaps one for each field. I'm not sure exactly how efficient you're trying to be and there is some cost involved with calling methods, so this might not be a good change for you. However, if peak performance is required then I'm sure you're familiar with profiling so just use whatever tool you typically use and determine whether the change impacts performance.

Along the same line of thinking you might even create an object to hold the fields and provide methods for validating each of them as well as a method to validate the data in general (this method could call each of the field validation methods and determine a result based on the results for the fields). Note that this approach also has overhead cost as you would create this object each time through the loop.

Here is what the class to hold the fields might look like:

public class PacketValidation {
    private String clientId, deviceId, payId, crossId;
    private Map<String,String> appPayload;
    // Not sure how you initialize the logger, so just added it as a field.
    private Logger logger;

    public PacketValidation(GenericRecord payload){
        clientId = (String) DataUtils.parseRecord(payload, "clientId");
        deviceId = (String) DataUtils.parseRecord(payload, "deviceId");
        payId = (String) DataUtils.parseRecord(payload, "payId");
        crossId = (String) DataUtils.parseRecord(payload, "crossId");
        appPayload = (Map<String, String>) DataUtils.parseRecord(payload, "appPayload");
    }

    public boolean isValid(){
        return isValidClientIdDeviceId() && isValidPayId() && isValidAppPayload();
    }

    private boolean isValidAppPayload() {
        if (MapUtils.isEmpty(appPayload)) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.INVALID_PAYLOAD);
            logger.logError("dropping records. invalid payload is coming.");
            return false;
          }
        return true;
    }

    private boolean isValidPayId() {
        if (payId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.MISSING_PAY_ID);
            logger.logError("dropping records. invalid payId is coming.");
            return false;
          }
        return true;
    }

    private boolean isValidClientIdDeviceId() {
        if (clientId == null && deviceId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                MetricName.INVALID_ID);
            logger.logError("dropping records. invalid clientId and deviceId is coming.");
            return false;
          }
        return true;
    }

    public String getClientId() {
        return clientId;
    }

    public String getDeviceId() {
        return deviceId;
    }

    public String getPayId() {
        return payId;
    }

    public String getCrossId() {
        return crossId;
    }

    public Map<String, String> getAppPayload() {
        return appPayload;
    }
}

Then your code might look something like:

try {
  GenericRecordDomainDataDecoder decoder = new GenericRecordDomainDataDecoder(config);
  while (true) {
    ConsumerRecords<byte[], byte[]> records = consumer.poll(1000);
    for (ConsumerRecord<byte[], byte[]> record : records) {
      DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
          MetricName.TOTAL_NUMBER);
      GenericRecord payload = decoder.decode(record.value());

      //Replaced validation logic with the following 3 lines:
      PacketValidation validation = new PacketValidation(payload);
      if(!validation.isValid())
          continue;

      DataMetricHolder.getInstance().increment(validation.getPayId(), MetricName.TOTAL_NUMBER);

      Builder packetBuilder = new DataPacket.Builder(payload);
      if (validation.getClientId() != null) {
        packetBuilder.setClientId(validation.getClientId());
      }
      if (validation.getDeviceId() != null) {
        packetBuilder.setDeviceId(validation.getDeviceId());
      }
      if (validation.getCrossId() != null) {
        packetBuilder.setCrossId(validation.getCrossId());
      }
      DataPacket packet =
          packetBuilder.setPayId(validation.getPayId()).appPayload(validation.getAppPayload()).build();
      System.out.println(packet);
    }
  }
} catch (Exception ex) {
  // logging error
}

Some final thoughts:

I'm not sure how often you use the field names, but it's generally a good idea to use constants of some kind if you're going to use them in more than one place. So for something like this:

clientId = (String) DataUtils.parseRecord(payload, "clientId");

I would typically expect to see something more like:

clientId = (String) DataUtils.parseRecord(payload, Constants.CLIENT_ID);

This helps to avoid spelling mistakes, whitespace issues, case sensitivity issues, etc.

I also noticed that your code has the following if statement:

Map<String, String> appPayload...
if (MapUtils.isEmpty(appPayload))

I'm assuming that Map is java.util.Map in which case I'm not sure why you don't simply use Map.isEmpty() unless MapUtils is doing a null check at the same time - meaning treating null as being empty.

I hope this helps you!

EDIT

Regarding your comment about having multiple consumers, I think you'll probably want to use a combination of the composite pattern and the strategy pattern so that you can try to reuse as much code as possible.

As for implementing a solution to this problem - if your consumers all use the GenericRecord class you could do something like the following:

public class PacketValidation {
    //Added a collection of Validatable objects to the class
    private Collection<Validatable> validationList;
    private String clientId, deviceId, payId, crossId;
    private Map<String,String> appPayload;
    private Logger logger;
    private GenericRecord record;

    public PacketValidation(GenericRecord payload){
        record = payload;
        clientId = (String) DataUtils.parseRecord(payload, "clientId");
        deviceId = (String) DataUtils.parseRecord(payload, "deviceId");
        payId = (String) DataUtils.parseRecord(payload, "payId");
        crossId = (String) DataUtils.parseRecord(payload, "crossId");
        appPayload = (Map<String, String>) DataUtils.parseRecord(payload, "appPayload");

        //Added logic in constructor to create the validators
        // only created one as this is a quick example.
        validationList = new ArrayList<Validatable>();
        validationList.add(new ClientIdAndDeviceIdValidator());
    }

    //Determine if the data is valid by calling `isValid` on all validators
    public boolean isValid(){
        for(Validatable validate: validationList){
            if(!validate.isValid(record))
                return false;
        }
        return true;
    }

    //Getters removed for brevity
}

The validator class would look something like:

public class ClientIdAndDeviceIdValidator implements Validatable {

    // Again, not sure how you're initializing this so just added a field for now.
    private Logger logger;

    @Override
    public boolean isValid(GenericRecord record) {
        String clientId = (String) DataUtils.parseRecord(record, "clientId");
        String deviceId = (String) DataUtils.parseRecord(record, "deviceId");

        if (clientId == null && deviceId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                    MetricName.INVALID_ID);
            logger.logError("dropping records. invalid clientId and deviceId is coming.");
            return false;
        }
        return true;
    }

}

and the Validatable interface may look like this:

public interface Validatable {
    public boolean isValid(GenericRecord record);
}

Now what you'll notice is that the implementation, ClientIdAndDeviceIdValidator, has to get the fields out of the GenericRecord because of the way the isValid method is written. Sometimes you want to validate that two fields together and other times just one field so in order to process all of the Validator objects generically we have to go one level above the fields and use the GenericRecord. Also note that I'm assuming you'll have one class for each consumer - therefore PacketValidation is not a generic class but rather one that is specific to the consumer.

Another possible solution would involve using Java Generics; here is an example:

public class PacketValidationGeneric {
    //Added a generic validatable object field
    private GenericValidatable<String> clientIdDeviceIdValidator;
    private String clientId, deviceId, payId, crossId;
    private Map<String,String> appPayload;
    private Logger logger;

    public PacketValidationGeneric(GenericRecord payload){
        clientId = (String) DataUtils.parseRecord(payload, "clientId");
        deviceId = (String) DataUtils.parseRecord(payload, "deviceId");
        payId = (String) DataUtils.parseRecord(payload, "payId");
        crossId = (String) DataUtils.parseRecord(payload, "crossId");
        appPayload = (Map<String, String>) DataUtils.parseRecord(payload, "appPayload");

        //Creating the specific validator here:
        clientIdDeviceIdValidator = new ClientIdAndDeviceIdGenericValidator();
    }

    public boolean isValid(){
        //Obviously there is more to be done that just validate the two fields, but this is just a quick example.
        return clientIdDeviceIdValidator.isValid(clientId, deviceId);
    }

    //Removed some code for brevity
}

Here is the validator implementation:

public class ClientIdAndDeviceIdGenericValidator implements GenericValidatable<String> {

    // Again, not sure how you're initializing this so just added a field for now.
    private Logger logger;

    @Override
    public boolean isValid(String ... fieldList) {
        //Since the parameter is very generic, you need to validate the input.
        if(fieldList.length < 2)
            throw new RuntimeException("Not enough fields provided, minimum 2 fields required - clientId and deviceId in that order.");
        String clientId = fieldList[0];
        String deviceId = fieldList[1];

        if (clientId == null && deviceId == null) {
            DataMetricHolder.getInstance().increment(DataUtils.EVENT_A,
                    MetricName.INVALID_ID);
            logger.logError("dropping records. invalid clientId and deviceId is coming.");
            return false;
        }
        return true;
    }

}

This is the interface:

public interface GenericValidatable<T> {
    public boolean isValid(T ... fieldList);
}

You'll notice that the PacketValidationGeneric.isValid() method can no longer invoke each validator in the same way because the parameters can differ. However, if you're going to have one class for each consumer then maybe it doesn't matter so much that you have to invoke each validator this way. Also notice, however, that you need to validate the input to each validator as the parameter to GenericValidatable.isValid is generic.

Once again, I hope this helps!


Success page + data entry for a store page The purpose of this code is to log the inputs of a form (which is a menu for a shop) to the database to be kept as records as well as printed out as a receipt at the store.

I have been learning how to code from the internet and tried to put together my knowledge in a project. The code is working, but I would like to adopt elegance and good coding practices.

<!doctype html>

<meta charset="utf-8">
<title>Untitled Document</title>
<?php
require 'databaseaccess.php';
?>

<?php
//check order present
if(!($_POST))
{
    echo "error, order was not received!";
    echo "
";
    echo "return to  order page ";

}
else{
    $total = 0;
    $soba_size = $_POST['soba_size'];
    switch($soba_size){
    case "small":
    $total = $total + 4;
    break;
    case "reg":
    $total = $total + 6;
    break;
    case "large":
    $total = $total + 8;
    break;
    }
    $soba_type = $_POST['soba_type'];
    $dbtoppings = '';
    if ($_POST['topping']){
        foreach($_POST['topping'] as $topping){
            switch($topping){
            case "duck":
            $total = $total + 1;
            $dbtoppings = $dbtoppings.",".$topping;
            break;
            case "egg":
            $total = $total + 2;
            $dbtoppings = $dbtoppings.",".$topping;
            break;
            case "chicken":
            $total = $total + 3;
            $dbtoppings = $dbtoppings.",".$topping;
            break;
            case "teriyaki chicken":
            $total = $total + 4;
            $dbtoppings = $dbtoppings.",".$topping;
            break;
            }
        }
    }
    if($_POST['requests']){
    $request = $_POST['requests'];
    }
    else{
    $request = '';
    }
//begin data entry
$data1 = array('total' => $total);
$STH = $DBH->prepare("INSERT INTO orderlist(total) values (:total)");
$STH->execute($data1);
$lastId = $DBH->lastInsertId();
//dataentry2
$data2 = array('orderid' => $lastId, 'soba_size' => $soba_size, 'soba_type' => $soba_type, 'topping' => $dbtoppings,'request' => $request);
$STH = $DBH->prepare("INSERT INTO orderdetails(orderID,soba_size,soba_type,topping,request) values (:orderid,:soba_size,:soba_type,:topping,:request)");
$STH->execute($data2);
}
?>

Thank you for ordering. Your order has been printed.

Order Number: <?php echo $lastId; ?>

        Amount:$ <?php echo $total;  ?>

Please show this page at the cashier to confirm your order. Thank you!

First and most important thing is to indent your code. Indenting provides an instant visual structure which makes it easy to read. More often than not, bugs and quirks can be easily avoided simply by good code indentation. PHP is especially susceptible to this because its code is mixed directly with the HTML output. There is actually an issue in your posted code where should there be an error, your code will still try to display the order details at the bottom.

Next thing you want to consider is creating your code in blocks. On a high level this is basic MVC code structure where the logic and the output are separated. You want to apply this as often as possible to make your code easier to read and maintain.

Make sure to close your database connection after opening it. I typically declare my database object outside the included class file in order to make it clearer what the variable is. This also helps your IDE out for providing popup advice, and code completion. If you're not using an IDE yet, try NetBeans or CodeLobster for PHP development.

Here's my take on your code. I obviously couldn't test it without your database details but it should give you some ideas.

<?php
class OrderDetails{
    private $Number;
    private $Amount;
    private $db;

    public function OrderDetails(){
        require 'databaseaccess.php';
        $this->db = $DBH;       //should probably declare $DBH outside the included file to make code easier to read, and to help the ide

        $this->ProcessOrder();

        //close database connection here
    }

    public function getNumber(){
        return $this->Number;
    }

    public function getAmount(){
        return $this->Amount;
    }

    private function ProcessOrder(){
        $total = 0;
        $soba_size = $_POST['soba_size'];
        switch($soba_size){
            case "small":
                $total = $total + 4;
                break;
            case "reg":
                $total = $total + 6;
                break;
            case "large":
                $total = $total + 8;
                break;
            default:break;      //always have a default even if you're confident it won't be needed
        }
        $soba_type = $_POST['soba_type'];
        $dbtoppings = '';
        if ($_POST['topping']){
            foreach($_POST['topping'] as $topping){
                switch($topping){
                    case "duck":
                        $total = $total + 1;
                        $dbtoppings = $dbtoppings.",".$topping;
                        break;
                    case "egg":
                        $total = $total + 2;
                        $dbtoppings = $dbtoppings.",".$topping;
                        break;
                    case "chicken":
                        $total = $total + 3;
                        $dbtoppings = $dbtoppings.",".$topping;
                        break;
                    case "teriyaki chicken":
                        $total = $total + 4;
                        $dbtoppings = $dbtoppings.",".$topping;
                        break;
                    default:break;      //always have a default even if you're confident it won't be needed
                }
            }
        }
        if($_POST['requests']){
            $request = $_POST['requests'];
        }
        else{
            $request = '';
        }

        //begin data entry
        $data1 = array('total' => $total);
        $STH = $this->db->prepare("INSERT INTO orderlist(total) values (:total)");
        $STH->execute($data1);
        $lastId = $this->db->lastInsertId();
        //dataentry2
        $data2 = array('orderid' => $lastId, 'soba_size' => $soba_size, 'soba_type' => $soba_type, 'topping' => $dbtoppings,'request' => $request);
        $STH = $this->db->prepare("INSERT INTO orderdetails(orderID,soba_size,soba_type,topping,request) values (:orderid,:soba_size,:soba_type,:topping,:request)");
        $STH->execute($data2);

        //set the class variables for grabbing by your other code
        $this->Amount = $total;
        $this->Number = $lastId;
    }
}
?><!DOCTYPE>

    <meta charset="utf-8">
    <title>Untitled Document</title>

    <?php
    //check order present
    if(!($_POST))
    {
        ?>
        error, order was not received!

        return to  order page
        <?php
    }
    else{
        $orderObj = new OrderDetails();
        ?>

                Thank you for ordering. Your order has been printed.

                Order Number: <?php echo $orderObj->getNumber();?>

                Amount: $ <?php echo $orderObj->getAmount();?>

                Please show this page at the cashier to confirm your order. Thank you!

        <?php
    }
    ?>


Deletion Distance between 2 strings I was solving this problem at Pramp and I have trouble figuring out the algorithm for this problem. I'll paste the problem description and how I kind of solved it. It's the correct solution.  It is similar to the edit distance algorithm and I used the same approach.  I just wanted to see what are other ways to solve this problem.

  The deletion distance of two strings is the minimum number of characters you need to delete in the two strings in order to get the same string. For instance, the deletion distance between "heat" and "hit" is 3:

  By deleting 'e' and 'a' in "heat", and 'i' in "hit", we get the string "ht" in both cases.
  We cannot get the same string from both strings by deleting 2 letters or fewer.
  Given the strings str1 and str2, write an efficient function deletionDistance that returns the deletion distance between them. Explain how your function works, and analyze its time and space complexities.

  Examples:
  input:  str1 = "dog", str2 = "frog"
  output: 3
  input:  str1 = "some", str2 = "some"
  output: 0
  input:  str1 = "some", str2 = "thing"
  output: 9
  input:  str1 = "", str2 = ""
  output: 0

What I want to do in this solution, is to use dynamic programming in order to build a function that calculates opt(str1Len, str2Len). Notice the following:
I use dynamic programming methods to calculate opt(str1Len, str2Len), i.e. the deletion distance for the two strings, by calculating opt(i,j) for all 0 ≤ i ≤ str1Len, 0 ≤ j ≤ str2Len, and saving previous values

  def deletion_distance(s1, s2):
    m = [[0 for j in range(len(s2) +1)] for i in range(len(s1)+1)]
    for i in range(len(s1)+1):
      for j in range(len(s2)+1):
        if i == 0:
          m[i][j] = j
        elif j == 0:
          m[i][j] = i
        elif s1[i-1] == s2[j-1]:
          m[i][j] = m[i-1][j-1]
        else:
          m[i][j] = 1 + min(m[i-1][j], m[i][j-1])
    return m[len(s1)][len(s2)]

I was solving this problem at Pramp and I have trouble figuring out the algorithm for this problem.

On the contrary, you've done a very good job of coming up with a solution. There are ways to improve it though.

As you note, this is just the Longest Common Subsequence problem in a thin disguise.  The "deletion distance" between two strings is just the total length of the strings minus twice the length of the LCS.

That is, the LCS of dogs (4 characters) and frogs (5 characters) is ogs (3 characters), so the deletion distance is (4 + 5) - 2 * 3 = 3.

Therefore, all you need to do to solve the problem is to get the length of the LCS, so let's solve that problem.

Your solution is pretty good but the primary problem is that it takes O(mn) time and memory if the strings are of length m and n.  You can improve this.

The first thing to notice is that if the strings have a common prefix or suffix then you can automatically eliminate it.  That is, the deletion distance for Who let the big dogs out? and Who let the little frogs out? is the same as the deletion distance for big d and little fr. It is very cheap and easy to determine if two strings have a common prefix and suffix, and you go from having an array with 25*29 elements to an array with 5*9 elements, a huge win.

The next thing to notice is: you build the entire m*n array up front, but while you are filling in the array, m[i][j] only ever looks at m[i-1][j-1] or m[i-1][j] or m[i][j-1]. Since you never look at an array line that is two away, you don't ever need more than two lines! That is, you can:

allocate and compute the first line
allocate and compute the second line given the first line
throw away the first line; we'll never use it again
allocate and compute the third line from the second line
throw away the second line
… and so on

You still do O(mn) operations, and you still allocate in total the same amount of memory, but you only have a small amount of it in memory at the same time.  If the strings are large, that's a considerable savings.


Critique My Codeigniter Custom CMS Pages Model I am currently developing a custom CMS being built on top of Codeigniter and was wondering if you can spot any flaws in my page fetching model code. The page fetching model is not entirely complete but the main functionality for retrieving a page is done, as well as retrieving modules assigned to a page (a module is really just a widget).

Can this model be better in some parts, perhaps in relation to the joins I am doing? although not really joins, but multiple queries to pull out bits of related info the pages like modules and media.

<?php

  class Mpages extends CI_Model {

      public function __construct()
      {
          parent::__construct();
      }

      public function fetch_page($page_slug = 'home')
      {
          $db = $this->db;

          $query =
            $db
               ->where('page_status', 1)
               ->where('page_slug', strtolower($page_slug))
               ->get('pages')
               ->result_array();

          $page_id = $query[0]['id'];

          $query['modules'] =
            $db
                ->select('modules.module_name, modules.module_slug, modules.id moduleid')
                ->where('page_id', $page_id)
                ->join('pages_modules lpm', 'moduleid = lpm.module_id')
                ->order_by('module_order', 'asc')
                ->get('modules')
                ->result_array();

          /*$query['media'] =
            $db
                ->select('lucifer_media.media_file_name, lucifer_media.media_file_extension, lucifer_media.media_directory')
                ->join('lucifer_pages_media', 'lucifer_pages_media.page_id = '.$page_id.'')
                ->get('lucifer_media')
                ->result_array();*/

          if ($query) {

            return $query;

          } else {
            return false;
          }
      }

      public function fetch_navigation()
      {
        $result = $this->db->order_by("nav_order", "asc")->where('published', 1)->get('navigation')->result_array();
        return $result;
      }

      public function fetch_layout($id)
      {

          $result = $this->db->where('id', $id)->get('layouts')->result_array();

          return $result[0];

      }

  }

?>

I don't know Codeigniter so i can't comment on "is this a propper model". For my answer i'm just going to assume it is.

It's not much code so i'm going to focus on some details:

  public function __construct()
  {
      parent::__construct();
  }

Those 5 lines do absolutely nothing. I'd remove them. If you need a constructor later, add it later.

  public function fetch_navigation()
  {
    $result = $this->db->order_by("nav_order", "asc")->where('published', 1)->get('navigation')->result_array();
    return $result;
  }

This is a pretty hard to read (113 Chars in that line). I'd go for something like this (to say with your formatting from above)

  public function fetch_navigation()
  {
      return $this->db
          ->order_by("nav_order", "asc")
          ->where('published', 1)
          ->get('navigation')
          ->result_array();
  }

$db = $this->db;

Seems unnecessary to create a local variable, those 6 extra chars shoudn't hurt. I needed to look twice to see where that variable is from and if its local because changes are made to it.

So far those are really minor notes.

Unreachable code ?

if ($query) {
  return $query;
} else {
  return false;
}

you are setting $query['modules'] = ... so even if that is null $query will at least contain

array("modules" => null);

and that will always be true.


Trapezoidal rule to approximate the integral of x^2 I've implemented the trapezoidal rule to compute the integral for a function \$x^2\$. I would like to see another style of the same code. It seems Matlab hates for a matrix to be expanded without specifying the size.

clear all
clc
n = 5; % number  subdivisions
a = 1.0; % lower limit
b = 2.0; % upper limit
sum = 0.0;
dx = (b-a)/(n-1); % step size

% Generate samples
for i = 1:n
    x(i) = a + (i-1)*dx;
end

% Generate function's values
for i = 1:n
    y(i) = x(i).^2;
end

% Compute area using Trapz. method
for i = 1:n
    if ( i == 1 || i == n) % for the first and last data
        sum = sum + y(i)./2;
    else
        sum = sum + y(i); % for the rest of data
    end
end
area = sum * dx;

area

Avoid variable names that collide with built-in functions

sum is a built-in function.  Redefining it causes unconventional behavior:

>> sum([1 2 3])

ans =

     6

>> sum = 0.0

sum =

     0

>> sum([1 2 3])
Index exceeds matrix dimensions.

>> clear all
>> sum([1 2 3])

ans =

     6

Idiomatic MATLAB

The whole point of using MATLAB, rather than C, is that the language is designed to work on many values at once.  Therefore, you should avoid looping wherever possible, and let MATLAB do its job on vectors.

% Givens
n = 5                                  % number of subdivisions
a = 1.0                                % x coordinate left endpoint
b = 2.0                                % x coordinate right endpoint

x = linspace(a, b, n)                  % x coordinates of samples
y = x .^ 2                             % y coordinates of samples
sum_y = sum(y) - (y(1) + y(end)) ./ 2  % sum y coordinates, taking just half of endpoints
dx = (b - a) / (n - 1)                 % width of each trapezoid
area = sum_y * dx

Reinventing the wheel

For your information, MATLAB has built-in support for numerical integration:

>> integral(@(x) x .^ 2, 1, 2)

ans =

    2.3333


JavaScript - Rock Paper Scissors Game I built a simple console version of the rock paper scissors game. I would love to get some constructive criticism!

Questions

How could I code this in an object-oriented way?
Did my code follow the DRY principle?

Link to code

Repl: https://repl.it/@antgotfan/CrowdedGreenFlatassembler

        var moves, result, images, relativePath, playerSelection, computerSelection;

    moves = {
        rock: 0,
        paper: 1,
        scissors: 2,
    };

    function init() {

        result = {
            player: 0,
            tie: 0,
            computer: 0
        };
    }

    init();

    function convertMoves() {
        // when any rock paper scissor button is clicked

        // then return that string into the moves object

        // return a number corresponding to the handsign.
        playerSelection = prompt("Please choose rock, paper, or scissors!");
        return moves[playerSelection];
    }

    function computerPlay() {
        var movesValues = Object.values(moves);
        var random = Math.floor(Math.random() * movesValues.length);
        return movesValues[random];
    }

    function playRound(playerSelection, computerSelection) {
        computerSelection = computerPlay();
        playerSelection = convertMoves();

        var processResult = (3 + computerSelection - playerSelection) % 3;

        if (!processResult) {
            ++result.tie;
            console.log('tie');
        } else if (1 === processResult) {
            ++result.computer;
            console.log('Computer won');
        } else {
            ++result.player;
            console.log('Player won');
        }

        return result;
    }

    function game() {
        for (var perRound = 1; perRound < 5; perRound++) {
            playRound(playerSelection, computerSelection);
        }
    }

    game();
    console.log(playRound(playerSelection, computerSelection));

This doesn't quite answer your questions, but still hopefully helpful.

var perRound = 1; perRound < 5; perRound++ only executes four times; the fifth execution is coming from the last line of your program. It's customary to use i when simply doing something multiple times, and to start with 0 such as: var i = 0; i < 5; i++. Then remove that extra playRound at the end so you still have five rounds.

I would rename convertMoves to playerMove to better describe what it does instead of how it does it.

playRound isn't using its arguments. So you can change the first few lines to:

    function playRound() {
        var computerSelection = computerPlay();
        var playerSelection = convertMoves();

You might play with changing moves to an array and see if you like it better as moves = ['rock', 'paper', 'scissors'];. This will require several other changes to how you use it but I think it'll read easier in the end.

Just as a matter of style, put the ++ after the variable except for the rare times when you really need the other behavior.

Again, hope this helps.

Edit: I see now why you have the extra playRound at the end. But result is already global so you can simply end with console.log(result) instead of returning it from playRound each time.


How to make selenium in python faster I'm making a bot for PlayStation 5.
And restock  is coming soon so I want to make my bot faster.
The approximate duration is 4.90 seconds.
Is there any way to make this faster?
I found that I should write this in C or C ++ for better results.
#startup
chrome_options = Options()
chrome_options.add_argument("--headless")
driver = webdriver.Chrome(options=chrome_options)
driver = webdriver.Chrome('chromedriver')
driver.maximize_window()
driver.get("https://www.games.rs/ps4-konzole/16794-konzola-playstation-4-1tb-pro-black-dualshock-fifa-21")

Cena and Kupi are unnecessary.
Program clicks to accept cookies.
Then scrolling cuz it needs to.
And then refreshing page until text in button is "Add to Cart".
#order
start_time = time.time()
kupi = driver.find_element_by_xpath('//*[@class="block product-attributes-wrapper clearfix"]/ul[1]/li[1]/div[1]/div[1]')
cena = driver.find_element_by_xpath('//*[@class="block product-attributes-wrapper clearfix"]/ul[1]/li[1]/div[1]/div[1]')
driver.find_element_by_xpath('//*[@id="modal-cookie-info"]/div/div/div/div/button').click()
driver.execute_script("window.scrollTo(712, 200);", kupi)
while True:
    try:
        driver.find_element_by_xpath('//button[text()="Dodaj u korpu"]')
    except NoSuchElementException:
        driver.refresh()
    else:
        driver.find_element_by_xpath('//button[text()="Dodaj u korpu"]').click()
        break

driver.find_element_by_xpath('//*[@id="miniCartContent"]/div[1]/a').click()
driver.execute_script("window.scrollTo(0 , 900);", )

Lastly login for informations and completing order.
#login
driver.find_element_by_xpath('//*[@id="order_address_content"]/div/div/div[2]/div[2]/div[1]/div[1]/div[2]/div/div/a').click()
time.sleep(0.2)
email = driver.find_element_by_css_selector('#login_email')
email.send_keys("mail")

password = driver.find_element_by_id('login_password')
password.send_keys("pass")
driver.find_element_by_xpath('//*[@id="login_modal"]/div/div/form/div[3]/button').click()
time.sleep(0.2)
driver.execute_script("window.scrollTo(0 , 1600);", )
driver.find_element_by_xpath('//*[@class="delivery-options list-unstyled"]/li[3]/div[1]/div[1]').click()
time.sleep(0.5)
driver.execute_script("window.scrollTo(0 , 2500);", )
driver.find_element_by_xpath('//*[@id="submit_order_one_page"]').click()

This prints duration of the program.
print("--- %s seconds ---" % (time.time() - start_time))

Just a short review from me:
The artificial delays (time.sleep(0.5)) are superfluous and not safe. They slow you down sometimes, they fail sometimes, because the pages take longer than usual to load.
Instead you can use Selenium selector functions, to wait until an element appears (or disappears). For example, you could have something like this:
element = WebDriverWait(driver, 10).until(
    EC.element_to_be_clickable((By.XPATH, "//button[text()="Dodaj u korpu"]")))
)

Here, we wait for an element to be in clickable state, but set a timeout of 10 seconds for the condition to be realized. You have a solid range of similar options available. Check out the doc: Waits
Likewise, you can get rid of the exception handling block (NoSuchElementException). My advice is to replace the sleeps with adequate wait functions, depending on the conditions you want to test. This should make your code more reliable and probably a bit faster, because it's not going to wait for longer than necessary.
Since your concern is speed: I would say 4.90 seconds is not bad but I don't know your constraints. Have you benchmarked your code to find out what is taking time ? Use the timeit module or - recommended link: Python Timer Functions: Three Ways to Monitor Your Code to set up your own timing class.
From experience, Selenium takes time to load, so perhaps you might as well leave it running in the background and use a loop to refresh the page at regular intervals, while being gentle and taking care not to overload the site.


Create variable and use in one stored procedure This finds a value via a stored procedure then runs a second process in the same stored procedure rather than calling a second stored procedure.

When a call is 'simple' i.e. not Where x y then... I keep the stored procedure, even when I need to get a variable, in the same procedure.

I'd like to see if the concept that works is the best concept, has any bad habits or can be improved.

USE [Database]
GO
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER FUNCTION [dbo].[GetImage]
(
-- Add the parameters for the function here
@SiteID varchar(100)
)
-- Is the below required?
RETURNS varchar(100)
AS
BEGIN
 -- Declare the return variable here
--We limit the length of the image name on import of images
DECLARE @SiteImage varchar(100)
--Client changes visible ID so we thus need to find 'Database ID' (SiteID)
DECLARE @SiteIDOriginal varchar(100)
-- Add the T-SQL statements to compute the return value here
--Get 'Database' ID
SELECT [@SiteIDOriginal] = SiteID FROM TblPropertyDetails WHERE ClientsSiteID=[@SiteID]
--Find the ImageName based on the Original ID (Database ID)
SELECT @SiteImage = ImageName FROM TblImages WHERE SITEID = @SiteIDOriginal
-- Return the result of the function
RETURN @SiteImage

END

You can make it shorter by having a single query, thus avoiding the extra variable @SiteIDOriginal and possibly make it a little faster:

ALTER FUNCTION [dbo].[GetImage]
(
    -- Add the parameters for the function here
    @SiteID varchar(100)
)
-- It is required as it specifies return value type
RETURNS varchar(100)
AS
BEGIN
    DECLARE @SiteImage varchar(100) = (
        SELECT ImageName
        FROM TblImages Img
            JOIN TblPropertyDetails PD ON PD.ClientsSiteID = Img.SiteID
        WHERE PD.ClientsSiteID = @SiteID
    )

    RETURN @SiteImage
END
GO

Other than that, something more of aesthetics:

try to have uniform formatting (blanks between variables and operators etc.)
use indentation (I usually treat SQL as Java or C# code where every "begin"
and "end" trigger a new level of indentation). This is particularly useful when reading rather large objects.


Codility binary gap solution using regex Below is the code I implemented to solve the binary gap problem:

  Find longest sequence of zeros, bounded by ones, in binary representation of an integer.

However, it seems to be different from the answers out there. Is it because this is a bad approach in terms of performance? I wanted to get some ideas.

function binaryGap(number) {
    let binary = (number >>> 0).toString(2), // convert to binary
        regex = /(?!1)(0+)(?=1)/g; // regex to match zeros between 1s

    let matches = binary.match(regex);

    return matches ? matches.reduce(function(carry, current){
        return carry.length > current.length ? carry : current;
    }).length : 0;
}

The Regex is computationally heavy.  I would consider simply iterating through the bits, keeping track of both the highest binary gap and the current binary gap. You could add optimizations such as that proposed by @konijn to bail out of iteration when you reach a point that you can no longer logically find a larger binary gap than your current max value.

That could be as simple as:

The following is updated per comments:

function binaryGap(number) {
    var binary = (number >>> 0).toString(2);
    var maxGap = 0;
    var currentGap = 0;
    var inGap = false;
    var length = binary.length;
    /*
    Fast return if binary string length is less than 3
    and no gap is possible
    */
    if(length < 3) {
        return 0;
    }
    /*
    Start iterating bits.  We start with second character.
    */
    for(var i = 1; i < length; i++) {
        /*
        See if we should continue evaluation based on whether
        we can actually exceed current maxGap number
        */
        if (maxGap >= currentGap + length - i) {
            break;
        }
        if (inGap === false) {
           // we need to check to see if a new gap is started
           if (binary[i-1] === '1' && binary[i] === '0') {
               // we are in a new gap
               currentGap = 1;
               inGap = true;
           }
        } else {
           // we need to see if gap has ended
           if (binary[i] === '1') {
               // gap has ended
               if (currentGap > maxGap) {
                   maxGap = currentGap;
               }
               inGap = false;
           } else {
               // gap has continued
               currentGap++;
           }
        }
    }
    return maxGap;
}

Here is a simple performance test I set up comparing the approaches.  Typically, I am seeing the bit iteration method working in about 20-25% of the time that the regex works.


Dynamic country, state select options Here is code I wrote to dynamically choose between three country options (United States, Canada, Other). After the user selects a country it will show the related select option or an empty input (when "Other" country is chosen). Then I pass the previous value into an input called spr(state/province/region). The end result is two values are saved: country and spr.
I'm here because I think the code could be refactored, and I'm not thinking about it in an efficient way.

const country = '#country';
const province = '#province';
const region = '#region';
const state = '#state';
const spr = '#spr';
const usa = '#usa';
const canada = '#canada';
const otherCountry = '#otherCountry';

hideCountryOptions(); //initial hide

$(country).on('change', function() {
  hideCountryOptions();
  selectSPR($(country).val());
  switch ($(country).val()) {
    case "UNITED STATES":
      $(usa).show();
      break;
    case "CANADA":
      $(canada).show();
      break;
    default:
      $(otherCountry).show();
  }
})

$(state + "," + province + "," + region).on('change', function() {
  selectSPR($(country).val());
});

function hideCountryOptions() {
  $(canada).hide();
  $(usa).hide();
  $(otherCountry).hide();
}

function selectSPR(country) { //pass value into the state/province/region input
  $(spr).val("");
  switch (country) {
    case "UNITED STATES":
      $(spr).val($(state).val());
      break;
    case "CANADA":
      $(spr).val($(province).val());
      break;
    default:
      $(spr).val($(region).val());
  }
}

  *Country
  <select name="country" id="country" required>
      <option value="">CHOOSE…</option>
      <option value="UNITED STATES">UNITED STATES</option>
      <option value="CANADA">CANADA</option>
      <option value="AFGHANISTAN">AFGHANISTAN</option>
    </select>

  *State
  <select name="state" id="state">
      <option value="">CHOOSE...</option>
      <option value="ALABAMA">ALABAMA</option>
      <option value="ALASKA">ALASKA</option>
    </select>

  *Province
  <select class="custom-select" name="province" id="province">
      <option value="">CHOOSE...</option>
      <option value="ALBERTA">ALBERTA</option>
      <option value="BRITISH COLUMBIA">BRITISH COLUMBIA</option>
    </select>

  *Region
  <input type="text" name="region" id="region" placeholder="Region">

  <input type="text" name="spr" id="spr">

I reduced the script in this way:

The script length (in lines): was 49, now 27
The jQuery lookups: was 18, now 7
The sub-function amount: was 2, now 0

There is still 2 change handlers.

The main changes:

I used classes... You were using any, except for custom-select that was not used here, in fact.
And I used CSS for the onload hiding of countries sub-division selects.

The lookups are made onload, in the const declarations, for all needed elements. Why only store the selector's string when the jQuery object can be stored?

You had the named function hideCountryOptions() to call 3 times the .hide() method on "similar" elements... I gave them the same countryPossible class to hide them in one shot.

The selectSPR(country) named function was replaced by spr.val(this.value); on change of countrySubDiv, which is the lookup made to target all the country sub-division's user inputs. No need for a switch()! Clearly, only the select that is visible can change, so this.value is the one to put into #spr.

const country = $("#country");
const countryPossibilities = $(".countryPossible");
const countrySubDiv = $(".countryPossible select, .countryPossible input");
const usa = $("#usa");
const canada = $("#canada");
const otherCountry = $("#otherCountry");
const spr = $("#spr");

country.on("change", function() {
  countryPossibilities.hide();
  switch (this.value) {
    case "UNITED STATES":
      usa.show();
      break;
    case "CANADA":
      canada.show();
      break;
    default:
      otherCountry.show();
  }
  countrySubDiv.val("");
  spr.val("");
});

countrySubDiv.on("change",function(){
  spr.val(this.value);
});
.countryPossible{
  display: none;
}

  *Country
  <select name="country" id="country" required>
    <option value="">CHOOSE…</option>
    <option value="UNITED STATES">UNITED STATES</option>
    <option value="CANADA">CANADA</option>
    <option value="AFGHANISTAN">AFGHANISTAN</option>
  </select>

  *State
  <select name="state">
    <option value="">CHOOSE...</option>
    <option value="ALABAMA">ALABAMA</option>
    <option value="ALASKA">ALASKA</option>
  </select>

  *Province
  <select name="province">
    <option value="">CHOOSE...</option>
    <option value="ALBERTA">ALBERTA</option>
    <option value="BRITISH COLUMBIA">BRITISH COLUMBIA</option>
  </select>

  *Region
  <input type="text" name="region" placeholder="Region">

  <input type="text" name="spr" id="spr">


Calculate maximum possible helpful flags for n days This program is supposed to calculate the mathemathical maximum of helpful flags you can have on the nth day, starting with 0 helpful flags, using the StackExchange system of gaining more possible flags, meaning every 10 helpful flags you gain one possible one, immediately.

#include <stdio.h>

// <summary>
// Small C program to simulate how many helpful flags someone could have raised on both comments and posts
// Assumptions:
// - Flags would be immediately marked helpful or near immediately
// - Every 10 flags, you get one extra flag per day, up to 100 total flags.
// - No declined flags
// @Author: Magisch - http://stackoverflow.com/users/5389107/magisch
// @License: CC0
// </summary>

int main(void) {
    int nDays = 0; // Days to simulate for
    int totalFlags = 0;
    int dailyFlags = 10;
    int bonusCountdown = 0;
    int totalFlagsBeforeCalc = 0;

    printf("Enter the amount of days you want to simulate flags for:\n");
    scanf("%d ", &nDays);

    printf("Computing maximum helpful comment flags and post flags for %d days.\n",nDays);

    for (int i = 0; i < nDays; i++) {
        totalFlags += dailyFlags;
        bonusCountdown += (totalFlags - totalFlagsBeforeCalc);
        totalFlagsBeforeCalc = totalFlags;
        while (bonusCountdown >= 10 && dailyFlags < 100) {
            dailyFlags++;
            totalFlags++;
            bonusCountdown -= 10;
        }
        printf("Day %d : total helpful flags: %d | daily available flags: %d\n", i+1, 2*totalFlags, dailyFlags);
    }
    printf("Simulated %d days. Total helpful flags: %d",nDays,2*totalFlags);
    return 0;
}

Example of the Results for n = 52 : http://pastebin.com/TbjRPjBn

I'm a beginner still in coding (second year apprentice), so please don't hold back on any critique.

You ought to check scanf return value and also that the input value is not negative. ++i please.


Builder pattern for Codeigniter ActiveRecord queries I am using Codeigniter and it's ActiveRecord.

I had this idea for a base class that provided a generic getter/setter for doing simple queries on a database. I have a lot of database objects that could benefit from not having to duplicate the basic query patterns.

Instead of writing functions whose sole purpose were to chain a few active record queries and return the result, I could instead do it with a generic $parameters array and providing the table name via the function call.

<?php

class ORMModel extends CI_Model {

    public function __call($name, $args) {
        $method = strtolower(substr($name, 0, 3));
        $object = substr($name, 3);

        // Parsing arguments
        $params = (isset($args[0])) ? $args[0] : array();
        $single = (isset($args[1])) ? $args[1] : false;
        $count = (isset($args[2])) ? $args[2] : false;
        $page = (isset($args[3])) ? $args[3] : '';
        $per_page = (isset($args[4])) ? $args[4] : '';

        if ($method === 'get') {
            $this->db->from($object);
        } elseif ($method !== 'set') {
            throw new Exception('Unsupported method ' . $name);
        }

        if (method_exists($this, $name)) {
            $this->{$name}($params);
        } elseif ($method === 'get') {
            $this->__getter($params);
        } else {
            $this->__setter($params);
        };

        if ($method === 'set') {
            // Insert on duplicate method, for both insert/updates, this way the SET can act as the WHERE as well
            return $this->db->on_duplicate($object);
        }

        $this->db->limit($per_page, $page);
        $result = $this->db->get();
        if (!$result) {
            throw new Exception('No result from ' . $name);
        }

        if ($count) {
            return $result->num_rows;
        }

        if ($single) {
            return $result->row_array();
        } else {
            return $result->result_array();
        }
    }

    private function __getter($params) {
        foreach ($params as $field => $value) {
            if (method_exists($this, $field)) {
                $this->where{$field}($value);
            }
            $this->db->where_in($field, $value);
        }
    }

    private function __setter($params) {
        foreach ($params as $field => $value) {
            $this->db->set($field, $value);
        }
    }

}

Example usage:

// Returns all orders
$this->ORMModel->getOrder()

// Returns order ID 3
$this->ORMModel->getOrder(array('id' => 3))

// Returns orders before a certain date
function orderWhereDateBefore() {
    $this->db->where('date <=', $value);
}

$this->ORMModel->getOrder(array('dateBefore' => '2012-01-01'))

// Huge search query (from a form)
$params = array(
    'status' => 'Paid',
    'dateBefore' => '2012-01-01',
    'name' => 'Some Guy',
    ...
);
$this->ORMModel->getOrder($params);

// Set an order
$params = array(
    'status' => 'Paid',
    'dateBefore' => '2012-01-01',
    'name' => 'Some Guy',
    ...
);
$this->ORMModel->setOrder($params);

The additional arguments also provide more flexibility for standard display of results, such as pagination and full count of the result set.

However, I am concerned whether its reusability can trump the concerns of readability, usability and debuggability (for lack of a better word)Maintainability, and if so, can it be improved on?

Well, for a kickoff, you're concerned about your code's reusability can trump its undeniable downsides. But, if you look at one or two things more closely, your code isn't as reusable as perhaps you think it is:

// Parsing arguments
$params = (isset($args[0])) ? $args[0] : array();
$single = (isset($args[1])) ? $args[1] : false;
$count = (isset($args[2])) ? $args[2] : false;
$page = (isset($args[3])) ? $args[3] : '';
$per_page = (isset($args[4])) ? $args[4] : '';

This is just a series of hard-coded statements, with a specific goal in mind. This isn't as generic as your object, called ORMModel would have you believe.
You're also being lead astray by CI's rather confusing name ActiveRecord, which actually doesn't implement the ActiveRecord pattern.

Your code shows signs of a nasty habbit, too, in the sense that you're using magic methods and object overloading. Avoid these things as much as possible, it'll slow your app down when you're accessing/setting properties, and even more so if that requires one or more magic method calls.

There's another issue with using __call too often: it can bypass the access modifiers:

class Unsafe
{
    private function mayOnlyBeCalledInternally()
    {
        //do risky things
    }
    public function __call($method, array $args)
    {
        if (method_exists($this, $method))
        {
            return call_usr_func_array(array($this, $method), $args);
            return $this->{$method}($args);//BTW: this is WRONG
        }
    }
}
$expose = new Unsafe;
$expose->mayOnlyBeCalledInternally();//WORKS!!!!

You're, essentially, exposing all private and protected methods through this __call method. Just don't!
As you may have noticed, I'm calling the methods using call_usr_func_array. The __call method receives the arguments as an array, but that doesn't mean the called method accepts an array. It could well be that this method expects three distinct arguments, not 1 array. The way you're calling existing methods:

if (method_exists($this, $name)) {
    $this->{$name}($params);
}

Will give you grief in such cases, but I'm going off topic slightly...

Basically, you've observed the limitations of CI's ActiveRecord and set about building something a bit more powerful. Now that's not a bad idea, except for one thing: It's all been done before, and (without wanting to suggest you're a bad dev) better than you'll be able to implement it. You're just 1 person, writing your own thing, Doctrine and the like have been around for some time, and have been worked on by many people. You can't compete with them.

If, however, you're not quite ready to switch to an existing sollution, I'd split your __call method into truly defined functions. For example, the call getOrder is being translated into get and Order, where Order is the tbl/object name. Why don't you just create a method like:

public function getFrom($tbl, array $params)
{
    $this->db->from($tbl);
    foreach($params as $field => $value)
    {
        $this->where{$field}($value);
    }
    //... and so on
}

In give time, you might find yourself creating an abstract model for your tables, and passing table objects to this function:

abstract class TableObject
{
    private $name = null;
    protected $fields = array();
    abstract public function __construct($name);
    final protected function _setName($name)
    {
        if ($this->name !== null)
        {
            throw new BadMethodCallExcpetion('Can\'t change tbl name once instance is created');
        }
        $this->name = $name;
        return $this;
    }
    final public function _getName()
    {
        return $this->name;
    }
    //bulk setter
    final public function setByRow(stdClass $record)
    {//or hint for array, or Traversable, or whatever
        foreach($record as $name => $value)
        {
            if (method_exists($this, 'set'.ucfirst($name)))
            {
                $this->{'set'.ucfirst($name)}($value);
            }
        }
    }
    //get object values as array
    final public funtion toArray()
    {
        $array = array();
        foreach($this->fields as $name)
        {
            $array[$name] = $this->{'get'.ucfirst($name)}();
        }
        return array_filter($array);
    }
}
//then
class Order extends TableObject
{
    public function __construct()
    {
        //set $this->fields here, if not in definition
        $this->_setName('Order');
    }
}

In accordance with my linked answers, it goes without saying that I'd strongly suggest your declaring all properties explicitly, and implementing a getter and setter for each property, too.
Anyhow, if you've done that, you can change the getObject method to:

public function getObject(TableObject $model)
{
    $this->db->form($model->_getName());
    $asArray = $model->toArray();//treat as $params in the snippet above
}

Now this is a more generic, and OO way of working. You could also create an TblArguments object, instead of using an array to determine if there is to be a limit, group by, count set/returned... But if I were you, I'd create specifc methods for count and single and per_page, really...

But really, read through a couple of my more verbose answers on this site, and perhaps on SO, too, I've given more detailed code examples of this approach before, and expanded on certain other benefits, too. But to recap: Your code is not terrible, not at all... it didn't make my eyes water or anything. But your __call method is doing more than it should be doing: it's checking if the particular call was meant to do a specific thing.
If you know that get + someString is meant to be a query, why bother relying on __call, why not implement a generic method for just that purpouse? it'll clean up your code, it'll be more maintainable, and even more reusable.


Find if the string contains ALL the vowels Problem:

  You are given a randoms string containing only lowercase letters
   and you need to find if the string contains ALL the vowels.

Input:

  First line contains N , the size of the string.
   Second line contains the letters (only lowercase).

Output:

  Print "YES" (without the quotes) if all vowels are found in the string, "NO" (without the quotes) otherwise.

Constraints:

  The size of the string will not be greater than 10,000
   1 ≤ N ≤ 10000

The above problem is from hackerearth.com and below is my attempt to solve this problem and I need your review on my codestyle, and efficiency and need your help to make it better.

public class TestClass {

public static void main(String[] args) throws IOException {

    BufferedReader input = new BufferedReader(new InputStreamReader(System.in));

    System.out.println("Enter the Length of the String");
    final int inputLength = Integer.parseInt(input.readLine());//the size of the string

    if (inputLength >= 10000) {
        System.err.println("The size of the string can not be greater than 10,000");
        System.exit(1);
    }

    System.out.println("Enter letters");
    String word = input.readLine().toLowerCase().trim();

    //remove all white space
    word = word.replaceAll(" ", "");

    final char[] vowels = {'a', 'e', 'i', 'o', 'u'};
    short found = 0;

    for (final char c : vowels) if (searchForVowel(c, word)) found++;

    String result = (found == vowels.length) ? "yes" : "no";

    System.out.println(result);
}

private static boolean searchForVowel(char vowel, String word) {
    final char[] chars = word.toCharArray();
    boolean isContains = false;

    for (char c : chars) {
        if (c == vowel) {
            isContains = true;
            break;
        }
    }

    return isContains;
  }

}

I would like to make one remark concerning the performance of your solution: you need to check if the word contains all vowels ('a', 'e', 'i', 'o' and 'u'), which means the following:

If the word does not contain 'a', then it's not good, else:
If the word does not contain 'e' (we already know it contains 'a'), it's not good, else:
...

But: in the English language, some vowels are more popular than others. Imagine following percentages (not real ones):
60% of the English words contain an 'a'
80% of the English words contain an 'e'
40% of the English words contain an 'i'
20% of the English words contain an 'o'
10% of the English words contain an 'u'

If you start searching with 'a', then with 'e', then with 'i', 'o', and 'u', you have following situation:
40% probability that you have found out that the word is not good, then:
20% probability that you have found out that the word is not good, then:
60% probability that you have found out that the word is not good, then:
80% probability that you have found out that the word is not good, then:
90% probability that you have found out that the word is not good.

=> in other words, you'll spend a lot of time investigating letters which are not popular.

However, if you re-order your search criteria:

first search for 'u'
then search for 'o'
then search for 'i'
then search for 'a'
and only finally search for 'e'

=> like this, you will find much sooner the words without 'u' (90% of the words), or the ones without an 'o' (80% of the words), ..., and this might heavily improve the performance of your search.

I must add that the percentages I have written down are just examples (for the correct percentages you need to search on frequency analysis of the English language).

Good luck


Powershell search millions of files as fast as possible I once asked a similar question but in C#. Now I have the same problem in powershell..

What is the fastest way, to search files newer than 15 minutes, in a file system with more than 1 million files?

Is there any faster way than using pipe?

Get-ChildItem -Path $path -Recurse | Select Name, PSIsContainer, Directory, LastWriteTime, Length| where {($_.LastWriteTime -gt (Get-Date).AddMinutes(-15))}

I already cut off some attributes to minimize the object size. It still takes ages.

First, you don't need to call Get-Date for every file. Just call it once at the beginning:

$t = (Get-Date).AddMinutes(-15)

Get-ChildItem -Path $path -Recurse |
    Select Name, PSIsContainer, Directory, LastWriteTime, Length |
    where {($_.LastWriteTime -gt $t)}

That's saves about 10% (as measured by Measure-Command).

Secondly, you don't need to call Select-Object for each file either. Just change the processing order:

$t = (Get-Date).AddMinutes(-15)

Get-ChildItem -Path $path -Recurse |
    where {($_.LastWriteTime -gt $t)} |
    Select Name, PSIsContainer, Directory, LastWriteTime, Length

Thirdly, try increasing the buffer size using the OutBuffer parameter:

$t = (Get-Date).AddMinutes(-15)

Get-ChildItem -Path $path -Recurse -OutBuffer 1000 |
    where {($_.LastWriteTime -gt $t)} |
    Select Name, PSIsContainer, Directory, LastWriteTime, Length

I've used 1000, but you can experiment with the value.

Those three changes reduced the running time to under one half on my system.


Infinitely iterable list class in Python itertools.cycle is cool, but it has an internal "storage" that uses up memory. And I wanted to practice "tampering" with built-in methods of classes, which is a new field for me. I tried to implement a class which mostly behaves like a list, but when we try to iterate over it (with for loop, for example), it would jump back to the beginning when it reaches the end, therefore cycling indefinitely. Here's what I've come up with:

class InfiniteListEmptyError(Exception):
    pass

class InfiniteList(list):
    """
    Pretty much a regular list, but when iterated over, it does not stop in the end.
    Instead, it iterates indefinitely.
    """

    def __init__(self, arg):
        super(InfiniteList, self).__init__(arg)

    def __getattr__(self, name):
        if name == "it":
            # get iterator
            result = object.__getattribute__(self, name)
        else:
            try:
                result = super(InfiniteList, self).__getattribute__(name)
            except AttributeError:
                try:
                    result = self.it.__getattribute__(name)
                except AttributeError:
                    # Initialize iterator cuz it's not initialized
                    self.__iter__()
                    result = self.it.__getattribute__(name)
        return result

    def __iter__(self):
        it = super(InfiniteList, self).__iter__()
        self.it = it
        return self

    def __next__(self):
        try:
            result = next(self.it)
        except StopIteration:
            self.__iter__()
            try:
                result = next(self.it)
            except StopIteration:
                raise InfiniteListEmptyError("Could not iterate. List is empty!")

        return result

# TESTS

a = InfiniteList(tuple()) #empty list

print(a)
print(a.__length_hint__()) #testing iterator attributes
print(a.__eq__([1,3,5])) # false

# should raise exception, since the list is empty
try:
    for i in a:
        print(i)
except InfiniteListEmptyError:
    print("List is empty!")

a.append(1)
a.extend([3,5])
print(a)
print(a.__eq__([1,3,5])) #true

# infinite loop
for i in a:
    print(i)

It works fine, it does not store an auxiliary list, so it does not consume twice as much memory, like itertools.cycle. But I'm not sure about the possible caveats when it comes to handling attributes. And yeah, I had to implement iterator within the InfiniteList class itself, so I had to redirect the calls to iterator's methods (like __length_hint__) to the saved iterator self.it.

This seems kind of complicated compared to something like this:

def cycle(seq):
    "Generator that yields the elements of a non-empty sequence cyclically."
    i = 0
    while True:
        yield seq[i]
        i = (i + 1) % len(seq)

Or, as Joe Wallis points out in comments:

def cycle(seq):
    "Generator that yields the elements of a non-empty sequence cyclically."
    while True:
        yield from seq


Reading/writing null-terminated strings from socket I need to handle C style ('\0' delimited) strings from/to a socket and came up with this as a first attempt:

#lang racket
(require racket/tcp)

(define (read-c-str inp)
  (define (read-to-zero acc)
    (define b (read-byte inp))
    (cond
      [(equal? b 0) (bytes->string/utf-8 (apply bytes (reverse acc)))]
      [else         (read-to-zero (cons b acc))]))
 (read-to-zero null))

(define (write-c-str val p)
  (write-bytes (bytes-append (string->bytes/utf-8 val) (bytes 0)) p)
  (flush-output p))

Writing is pretty simple, but I have the feeling I'm missing a more simple way to handle the accumulator there, and mainly looking for feedback on read-c-str. I'm suspicious of having to reverse, making me think there is a better way.

Here was my second attempt which I'm not convinced is a better way:

(define (blist->string blist)
  (bytes->string/utf-8 (foldr (lambda (inb l) (bytes-append l (bytes inb))) #"" blist)))

(define (read-c-str inp)
  (define (read-to-zero acc)
    (define b (read-byte inp))
    (if (equal? b 0)
        (blist->string acc)
        (read-to-zero (cons b acc))))
  (read-to-zero null))

I would accumulate bytes instead of integers.

(define (read-c-str inp)
  (define (read-to-zero acc)
    (define b (read-byte inp))
    (if (equal? b 0)
        (blist->string acc)
        (read-to-zero (bytes-append acc (bytes b)))))
  (read-to-zero #""))

And change blist->string accordingly.

Edit: in this case blist->string will become just bytes->string/utf-8 .


Extract last delivery for each name on each date I've been using Java 8 Stream features quite a lot these days and am very happy with it. I've written a code that uses groupingBy and max but was wondering if it can still be optimized or even just be tweaked for better readability.

Some sample data:

Name, OrderDate, DeliveryTime
abc, 2010-01-15, 2010-01-15T11:00:00
abc, 2010-01-15, 2010-01-31T07:00:00 //should be marked as latest
zzz, 2010-01-15, 2010-01-13T11:00:00
zzz, 2010-01-15, 2010-01-23T07:00:00 //should be marked as latest

Here's the code I've written:

public class DeliveryTest {
   class Delivery {
      String name;
      LocalDate orderDate;
      LocalDateTime deliveryTime;
      boolean isLatest = false;
      //getters and setters here
   }

   @Test
   public void testLatestDelivery() {
      List<Delivery> deliveries = new ArrayList<>();
      //populate the test data here

      //group the deliveries by name and orderDate
      Map<String, Map<LocalDate, List<Delivery>>> groupByNameAndOrderDate = deliveries.stream()
            .collect(groupingBy(Delivery::getName, groupingBy(Delivery::getOrderDate)));

      //find and mark as "Latest" for each of the the max deliveryTime per name and orderDate grouping,
      groupByNameAndOrderDate.forEach((name, byNameMap) -> {
          byNameMap.forEach((orderDate, byOrderDateList) -> {
              byOrderDateList.stream()
                  .max((n1, n2) -> n1.getDeliveryTime().compareTo(n2.getDeliveryTime())).get()
                  .setIsLatest(true);
          });
      });
      //do assertions here
   }
}

Can this Java 8 code still be improved (for example the part where I have nested forEach)?

Or written shorter but still readable (for example, can the group and max be combined)?

You could short-hand your max() function with the use of Comparator.comparing()

byOrderDateList.stream()
  .max(Comparator.comparing(Delivery::deliveryTime)).get()
  .setIsLatest(true);


Using MOQ in unit-testing for a shipping calculator I am fairly new to writing unit tests, and I was wondering if this approach I have taken is on the right track?

Given the following interfaces/classes:

public interface ILoggingService
{
    void Log(string message);
}

public interface IShoppingCart
{
    IList<IShoppingCartItem> Items { get; }
}

public interface IShoppingCartItem
{
    string Sku { get; set; }
    string Name { get; set; }
    string Description { get; set; }
    decimal Price { get; set; }
    int Quantity { get; set; }
}

public interface IShippingCalculator
{
    decimal Calculate();
}

public class ShoppingCartItem : IShoppingCartItem
{
    public string Sku { get; set; }
    public string Name { get; set; }
    public string Description { get; set; }
    public decimal Price { get; set; }
    public int Quantity { get; set; }
}

public class ShippingCalculator : IShippingCalculator
{
    private readonly ILoggingService loggingService;
    private readonly IShoppingCart shoppingCart;

    public ShippingCalculator(IShoppingCart shoppingCart, ILoggingService loggingService)
    {
        this.shoppingCart = shoppingCart;
        this.loggingService = loggingService;
    }

    public decimal Calculate()
    {
        if (shoppingCart.Items.Count == 0)
            return 0;

        var pricePerItem = 3;

        return shoppingCart.Items.Sum(x => x.Quantity * pricePerItem);
    }
}

Given the requirements of:

  The Shipping Calculator must calculate shipping at the rate of $3.00 per item.

Does this unit test look correct?

public class ShippingCalculatorTests
    {
        [Fact]
        public void Test1()
        {
            //Arrange
            var cart = new Mock<IShoppingCart>();
            cart.Setup(m => m.Items).Returns(
                new List<IShoppingCartItem>() {
                    new ShoppingCartItem() { Quantity = 5 },
                    new ShoppingCartItem() { Quantity = 3 }
                }
            );

            var logger = new Mock<ILoggingService>();

            var calc = new ShippingCalculator(cart.Object, logger.Object);

            //Act
            var result = calc.Calculate();

            //Assert
            Assert.Equal(24, result);
        }
    }

Your test is testing one thing only which is a very good thing. It is better to have many simple tests instead of one massive and it makes bug finding much easier.

First of all, I would suggest using more descriptive names for both methods and test. Consider calling Calculate() method differently, i.e. CalculateTotalShippingPrice, so that it indicates what it is doing.

Use test names which specify what is being tested, what is the state under test and expected behaviour as in: https://stackoverflow.com/questions/155436/unit-test-naming-best-practices.
Test1 could be called Calculate_WhenShoppingCartNotEmpty_CorrectlyCalculatesTotalShippingPrice

Make sure you take into account different scenarios, it is worth adding another test: Calculate_WhenShoppingCartEmpty_ReturnsZero.


Using both setInterval and setTimeout for simple image carousel I have a really basic image carousel that I just wrote.

Basically I'm trying to keep it as small and light weight as possible, but I get the feeling that I've gotten way off the beaten path of how these things normally work.

function hide(element, index, array) {
  if (index > 0) {
    slides[index].setAttribute('style', 'opacity:0;');
  }
}

var carousel = document.getElementById("carousel"),
  slides = carousel.getElementsByTagName('li'),
  counter = 0,
  liList = Array.prototype.slice.call(slides);

setInterval(function() {
  slides[counter].setAttribute('style', 'opacity:1;');
  counter++;

  if (counter == slides.length) {
    counter = 0;
    setTimeout(function() {
      liList.forEach(hide);
    }, 3000); // setTimeout
  }
}, 3000); // setInterval
#carousel {
  padding: 0;
  margin: 0;
  position: relative;
  width: 315px;
  height: 177px;
}
#carousel li {
  opacity: 0;
  list-style: outside none none;
  width: 315px;
  position: absolute;
  background: #fff;
  transition: opacity 1s;
}

Could this be trimmed down further, is this a bad way to go about something this simple, or am I just over thinking it?

How about this: every 3 seconds, you hide the previous image and show the next image. This is simple.

Some changes I made:

I prefer .style.opacity = as opposed to setAttribute("style"). This may be personal preference, but it looks cleaner to me because it uses javascript's object access instead of setting it via a string.
You can use a modulus (%) instead of checking if counter === slides.length.

var carousel = document.getElementById("carousel"),
  slides = carousel.getElementsByTagName('li'),
  counter = 0,
  liList = Array.prototype.slice.call(slides);

setInterval(function() {
  slides[counter].style.opacity = 0; // Hide the previous image
  counter = (counter + 1) % slides.length; // Increment counter
  slides[counter].style.opacity = 1; // Show the next image
}, 3000); // setInterval
#carousel {
  padding: 0;
  margin: 0;
  position: relative;
  width: 315px;
  height: 177px;
}
#carousel li {
  opacity: 0;
  list-style: outside none none;
  width: 315px;
  position: absolute;
  background: #fff;
  transition: opacity 1s;
}


Counting the number of consecutive 1's in binary hackerrank.com Task:

  Given a base-10 integer, n, convert it to binary (base-2). Then find and print the base-10 integer denoting the maximum number of consecutive 1's in n's binary representation.

This question has already been answered but it was in Ruby and I couldn't quite understand the concept, I am just starting to code. What can you suggest me to improve on this piece of code? I think its quite long considering the work it does or is this the efficient solution?

count = 0
max_value = 1

if __name__ == '__main__':
    n = int(input())
    binary_num = bin(n).split("b")
    binary_num = binary_num[1]
    print(binary_num)
    for index, i in enumerate(binary_num):
        if i == "1":
            count += 1
            next_item = index + 1
            while next_item < len(binary_num):
                if binary_num[next_item] == "1":
                    count += 1
                    counter = count
                    temp = counter
                    next_item += 1
                    if temp > max_value:
                        max_value = temp
                else:
                    counter = count
                    count = 0
                    break
        if next_item == (len(binary_num)):
            break

    print("count is =",max_value)

I think its quite long considering the work it does

Your instinct is right - it's quite long. It can be implemented with much fewer lines of code, especially in Python. A solution from @RomanPerekhrest highlights it quite well.

  is this the efficient solution?

From the time complexity prospective, it's not an efficient solution. It's O(n2), where n - number of binary digits in your number. Why? Because in your outer loop, you process each digit one by one regardless of the results in your inner loop. So, if your inner loop finds a segment of ones you don't move ì and index to a bit position following this segment. Instead, you pick a second bit in this segment and find the same segment without the first bit again.

For instance, let's say we have a number which binary representation is 111110. Then your algorithm will find the following segments of ones:

11111  | 4 inner loop steps
 1111  | 3 inner loop steps
  111  | 2 inner loop steps
   11  | 1 inner loop steps
    1  | 0 inner loop steps

An optimal solution has a O(n) time complexity. I'll leave it up to you to update your code based on the explanation above.

Bonus

I noticed that each answer uses bin() to calculate the longest segment of ones. However, it's not really needed since this approach has an additional O(n) space complexity (as you recall it from the previous paragraph, n - number of binary digits).

We could just use bitwise operators to achieve the same result. For example, let's say there's a number 1100011101. It has 3 separate segments of ones: Now, let's play a bit - we will calculate a left shifted version of x and then perform bitwise AND on it. Then, for the shifted version we will calculate its left shifted number and so on.

1 1 0 0 0 1 1 1 0 1 0  (x)
         &
1 0 0 0 1 1 1 0 1 0 0  (x << 1)
---------------------
1 0 0 0 0 1 1 0 0 0 0  y = ((x) & (x << 1))

1 0 0 0 0 1 1 0 0 0 0  (y)
         &
0 0 0 0 1 1 0 0 0 0 0  (y << 1)
---------------------
0 0 0 0 0 1 0 0 0 0 0  z = ((y) & (y << 1))

0 0 0 0 0 1 0 0 0 0 0  (z)
         &
0 0 0 0 1 0 0 0 0 0 0  (z << 1)
---------------------
0 0 0 0 0 0 0 0 0 0 0  ((z) & (z << 1))

So, as you can see it took us 3 steps to reach 0. But 3 is also the length of the longest segment of ones. And it's not a coincidence because bitwise AND of a number and its shifted version shortens each segment of ones by 1. Hence, the longest segment will lose all its ones after < length of the longest segment of ones > steps.

Code

def find_max_ones(number):
    maxLength = 0

    while number > 0:
        left_shifted_number = number << 1
        number = number & left_shifted_number
        maxLength += 1

    return maxLength

if __name__ == '__main__':
    number = int(input('Enter integer number:'))
    max_ones = find_max_ones(number)

    print("Length of the longest segment is ", str(max_ones))


*nix, *bsd, etc basic `tput` color setup My goal with the below piece of POSIX shell code was to address the more platforms the better with shell tput colors. With this code, I now start all of my scripts, and so it's time to review it for some things I overlooked, did not think of too well, and such. Note: I start my scripts with set -u, which is why I set all empty in unspecified cases. Thanks.

#!/bin/sh

set -u

if tput setaf > /dev/null 2>&1; then
    # Linux-like
    tput_number_of_colors=$(tput colors 2> /dev/null)
    tput_bold=$(tput bold 2> /dev/null)
    tput_reset=$(tput sgr0 2> /dev/null)
    tput_cmd_set_fg_color='tput setaf'
elif tput AF > /dev/null 2>&1; then
    # BSD-like
    tput_number_of_colors=$(tput Co 2> /dev/null)
    tput_bold=$(tput md 2> /dev/null)
    tput_reset=$(tput me 2> /dev/null)
    tput_cmd_set_fg_color='tput AF'
else
    # Console-like
    tput_number_of_colors=2
    tput_cmd_set_fg_color=
    tput_bold=
    tput_reset=
fi

tput_test ()
{
    [ -n "$tput_number_of_colors" ] && [ -n "$tput_bold" ] && [ -n "$tput_reset" ] &&
    { [ "$tput_number_of_colors" -ge 8 ] && printf '%s' "$tput_bold" && $tput_cmd_set_fg_color 1; } > /dev/null 2>&1
}

if tput_test; then
    color_red=$tput_bold$($tput_cmd_set_fg_color 1)
    color_green=$tput_bold$($tput_cmd_set_fg_color 2)
    color_yellow=$tput_bold$($tput_cmd_set_fg_color 3)
    color_blue=$tput_bold$($tput_cmd_set_fg_color 4)
    color_magenta=$tput_bold$($tput_cmd_set_fg_color 5)
    color_cyan=$tput_bold$($tput_cmd_set_fg_color 6)
    color_white=$tput_bold$($tput_cmd_set_fg_color 7)
else
    color_red=; color_green=; color_yellow=; color_blue=; color_magenta=; color_cyan=; color_white=
fi

Self-review
Since no one replied thus far, I decided to re-think and re-write the code myself this morning.

terminfo (*nix) vs termcap (*bsd)
I never really thought about how important to make note of this actually is. For searching info online, one needs to know these keywords. So, I added them as comments.
Structure into functions
I believe this code should be structured into suitable functions where available.
Find a way for an exit code from those functions
It's needed to simply chain (&&) the commands like tput_bold=$(tput bold 2> /dev/null) && tput_reset=$(tput sgr0 2> /dev/null) in order for the imagined functions to return a reliable exit code.
The most basic test missing
command -v tput was missing in my code, which is remedied now.
No unset variables
Since I use set -u in my scripts, it's necessary to set variables empty in case of failure. This feature has been enhanced.

Modified code
#!/bin/sh

set -u

tput_setup_nix ()
{
    # terminfo
    tput_cmd_set_fg_color='tput setaf'
    tput_number_of_colors=$(tput colors 2> /dev/null) &&
    tput_bold=$(tput bold 2> /dev/null) &&
    tput_reset=$(tput sgr0 2> /dev/null)
}

tput_setup_bsd ()
{
    # termcap
    tput_cmd_set_fg_color='tput AF'
    tput_number_of_colors=$(tput Co 2> /dev/null) &&
    tput_bold=$(tput md 2> /dev/null) &&
    tput_reset=$(tput me 2> /dev/null)
}

tput_setup_none ()
{
    # no unset variables
    tput_cmd_set_fg_color=
    tput_number_of_colors=
    tput_bold=
    tput_reset=
}

if command -v tput > /dev/null 2>&1; then

    if tput setaf > /dev/null 2>&1; then

        if ! tput_setup_nix; then tput_setup_none; fi

    elif tput AF > /dev/null 2>&1; then

        if ! tput_setup_bsd; then tput_setup_none; fi

    else
        tput_setup_none
    fi

else
    tput_setup_none
fi

tput_capability_test ()
{
    [ -n "$tput_cmd_set_fg_color" ] && [ -n "$tput_number_of_colors" ] && [ -n "$tput_bold" ] && [ -n "$tput_reset" ] &&
    [ "$tput_number_of_colors" -ge 8 ] && { $tput_cmd_set_fg_color 1 && printf '%s' "$tput_bold$tput_reset"; } > /dev/null 2>&1

}

if tput_capability_test; then
    color_red=$tput_bold$($tput_cmd_set_fg_color 1)
    color_green=$tput_bold$($tput_cmd_set_fg_color 2)
    color_yellow=$tput_bold$($tput_cmd_set_fg_color 3)
    color_blue=$tput_bold$($tput_cmd_set_fg_color 4)
    color_magenta=$tput_bold$($tput_cmd_set_fg_color 5)
    color_cyan=$tput_bold$($tput_cmd_set_fg_color 6)
    color_white=$tput_bold$($tput_cmd_set_fg_color 7)
else
    color_red=
    color_green=
    color_yellow=
    color_blue=
    color_magenta=
    color_cyan=
    color_white=
fi


Saving a recommended web article from form in Rails controller I need criticisms and refactoring advice for my controller code:

class ItemsController < ApplicationController

    def new

    end

    def create
        params = items_params
        if not titles_and_urls_are_same_length(params)
            render :new
        end

        collection = false
        if params[:titles].length > 1
            collection = true
        end

        element = nil
        if collection
            collection = RecommendedCollection.new
            params[:titles].each_with_index do |title, index|
                resource = RecommendedResource.new(title: title, url: params[:urls][index])
                collection.resources << resource
            end
            if not collection.save
                return render :new
            end
        else
            resource = RecommendedResource.new(title: params[:titles][0], url: params[:urls][0])
            if not resource.save
                return render :new
            end
        end

        if not RecommendedItem.create(element: element)
            return render :new
        end
        render :new

    end

    private
        def titles_and_urls_are_same_length(params)
            params[:titles].length == params[:urls].length
        end

        def items_params
            params.permit(:description, titles: [], urls: [])
        end
end

Parameter example:

Parameters: {"utf8"=>"✓", "authenticity_token"=>"......", "titles"=>["Learning to program in ruby", "Refactoring Code blog"], "urls"=>["http://www.hi.com", "http://www.ruby-master.com"], "description"=>"Improve your ruby skills", "commit"=>"Recommend"}

The idea behind the controller is that the user submits a link along with a URL and description to a site they like. If you submit a single link, then it is just a RecommendedResource, if you submit multiple links then it is to be turned into a "collection" or RecommendedCollection. That is why I check for the length of the params (which is probably not a good way to write the code).

They all go into a RecommendedItem to show in the feed, so I loop through one model only.

Here is an image that explains the visual results:

Your controller is very creative but its got some pretty big problems. In Rails controllers are notoriously hard to test and can quickly become very buggy if they contain too much logic. Skinny controllers are a vital part of good MVC code.

Any time your controller action has more than two possible code branches than you should stop and rethink it.

The Rails way of approaching this problem is nested routes and nested parameters.

Lets start out with the nested routes:

resources :collections, shallow: true do
  resources :recommendations
end

This would give us routes like:

                      Prefix Verb   URI Pattern                                               Controller#Action
   collection_recommendations GET    /collections/:collection_id/recommendations(.:format)     recommendations#index
                              POST   /collections/:collection_id/recommendations(.:format)     recommendations#create
new_collection_recommendation GET    /collections/:collection_id/recommendations/new(.:format) recommendations#new
          edit_recommendation GET    /recommendations/:id/edit(.:format)                       recommendations#edit
               recommendation GET    /recommendations/:id(.:format)                            recommendations#show
                              PATCH  /recommendations/:id(.:format)                            recommendations#update
                              PUT    /recommendations/:id(.:format)                            recommendations#update
                              DELETE /recommendations/:id(.:format)                            recommendations#destroy
                  collections GET    /collections(.:format)                                    collections#index
                              POST   /collections(.:format)                                    collections#create
               new_collection GET    /collections/new(.:format)                                collections#new
              edit_collection GET    /collections/:id/edit(.:format)                           collections#edit
                   collection GET    /collections/:id(.:format)                                collections#show
                              PATCH  /collections/:id(.:format)                                collections#update
                              PUT    /collections/:id(.:format)                                collections#update
                              DELETE /collections/:id(.:format)                                collections#destroy

So the user would create a brand new collection from /collections/new -
and POST to /collections.

but we want the user to be able to create a recommendation on the fly as well. So lets make so that we can pass recommendations into the collection.

So what we do is add accepts_nested_attributes_for to RecommendedCollection:

class RecommendedCollection < ActiveRecord::Base
  has_many :recommendations, class_name: 'RecommendedResource'
  accepts_nested_attributes_for :recommendations, reject_if: :all_blank?
end

So from our Collection controller we can create both the collection and the recommendation

class CollectionController < ApplicationController

  def new
    @collection = RecommendedCollection.new
    @collection.recommendations.build # create an empty recommendation for the form.
  end

  def create
    @collection = RecommendedCollection.create(collection_parameters)
    if @collection.persisted?
      redirect_to @collection
    else
      render :new, notice: 'Collection could not be created.'
    end
  end

  def collection_parameters
    params.require(:collection).allow(recommendation_attributes: [:title, :url])
  end
end

So now if we do POST /collection  collection: { recommendation_attributes: ['Test', 'http://www.example.com'] } we create a collection and a recommendation.

The form would look something like this:

<%= form_for(@ollection) do |f| %>
   <%= f.fields_for(:recommendations) do |nested_fields| %>
     <%= nested_fields.text_field :url %>
     <%= nested_fields.text_field :title %>
   <% end %>
<%= end %>

Later when other users or the same user wants to add more recommendations to a collection they would post to /collections/1/recommendations.


Program to find the element in an array I'd like this code to be improved.

import java.util.Scanner;

public class ArrayElementFind {

    public static void main(String[] args) {
        // TODO Auto-generated method stub

        String[] arr= { "A","B","C","D"};

        Scanner in=new Scanner(System.in);

        System.out.println("Enter the char to search");
        String a=in.nextLine();

        //int len=arr.length;

        boolean check=loopcheck(arr,a);

        if(check){
            System.out.println("Element is present");
        }
        else{
            System.out.println("Element is not present");
        }

    }

    private static boolean loopcheck(String[] arr, String a) {
        // TODO Auto-generated method stub

        for(String s:arr){
            if(s.equals(a)){
                return true;
            }
        }

        return false;
    }
}

Since you're not using the try-with-resources statement with Scanner, you must close it at the end of main() to avoid leaking resources:

in.close();

You should consider converting a to uppercase, in case the input is in lowercase.

I would also rename a to something like guess.  The name a says nothing about this variable, and it's generally discouraged to use single-letter variable names.

String guess = in.nextLine().toUpperCase();

The renaming also applies to arr.  You could name it something like elemsToCheck.


Checking opcode values What troubles me are the empty /**/ brackets. Is there a cleaner way of handling this logic?

if (opcode == OP_0) { /* continue */}
    else if (opcode == OP_1NEGATE) { /* continue */}
    else if (opcode >= OP_1 && opcode <= OP_16) { /* continue */}
    else { throw new Exception("decodeFromOpN called on non OP_N opcode"); }

What you could use is a function that returns whether an opcode is valid.

private boolean isOpCode(Object opCode){
    return opcode == OP_0 || opcode == OP_1NEGATE || (opcode >= OP_1 && opcode <= OP_16);
}

Then in your code you posted, call the function, and throw an exception if it returns false.

if(!isOpCode(opCode)){
    throw new OpCodeInvalidException("decodeFromOpN called on non OP_N opcode");
}

Replace Object with whatever the opcodes are.

And make a new class that extends from Exception. It's generally bad practice to throw Exception.


React timer application I am writing a simple count down timer in React:

index.js

import React from "react";
import ReactDOM from "react-dom";
import "./index.css";
import bootstrap from "bootstrap/dist/css/bootstrap.css";
import App from "./App";

ReactDOM.render(
  <React.StrictMode>
    <App />
  </React.StrictMode>,
  document.getElementById("root")
);

App.js:

import React, { useEffect, useState } from "react";

const App = () => {
  const oneSec = 1000;
  const [secCount, setSecCount] = useState(60);
  const [minCount, setMinCount] = useState(60);
  const [hrCount, setHrCount] = useState(24);

  useEffect(() => {
    const timer;    //<= here is my problem.
    if (secCount > 0) {
      timer = setInterval(() => {
        setSecCount(secCount - 1);
      }, oneSec);
    }
    if (secCount === 0 && minCount > 0) {
      timer = setInterval(() => {
        setSecCount(60);
      }, oneSec);
    }
    return () => clearInterval(timer);
  }, [secCount]);

  useEffect(() => {
    const timer =
      minCount > 0 &&
      setInterval(() => {
        setMinCount(minCount - 1);
      }, 60 * oneSec);
    return () => clearInterval(timer);
  }, [minCount]);

  return (
    <div>
      {minCount}:{secCount}
    </div>
  );
};

export default App;

I am having a hard time with my useEffect hooks. The first useEffect hook is supposed to update the second field, the logic is simply:

if secCount is bigger than 0, continue to countdown every second
if secCount is zero and minCount is bigger than 0, reset secCount to 60 and continue to countdown.

But since there are two branches in my first useEffect hook, I need to declare the timer variable and reassign it later. Can I get away with declaring it as let timer? A const variable does not allow me to change its value.

let vs const is fundamentally an ES6+ issue:

constCreates a constant whose scope can be either global or local to the block in which it is declared. An initializer for a constant is required. You must specify its value in the same statement in which it's declared.

// wrong
const timer;
// Uncaught SyntaxError: Missing initializer in const declaration

// right
const timer = 60;

letAllows you to declare variables that are limited to a scope of a block statement, or expression on which it is used, unlike the var keyword, which defines a variable globally, or locally to an entire function regardless of block scope. Please note that a let variable is initialized to a value only when a parser evaluates it.

function do_something() {
  console.log(bar); // undefined
  console.log(foo); // ReferenceError
  var bar = 1;
  let foo = 2;
};

With that said, change the declaration to let timer;

// Within the first `useEffect`
useEffect(() => {
     let timer; // change this
     if (secCount > 0) {
       timer = setInterval(() => {setSecCount(secCount - 1)}, oneSec);
     }
     if (secCount === 0 && minCount > 0) {
       timer = setInterval(() => {setSecCount(60)}, oneSec);
     }
     return () => clearInterval(timer);
   }, [secCount]);
```