Using synchronized blocks and volatile variables to ask a Java thread to terminate gracefully Edit: Just to clarify, this isn't my original code, I've simplified it to make it smaller/easier to understand what I'm trying to do.

It's my first time working with threads in Java, and I was wondering if I'm synchronizing correctly. I did some quick research on locks, synchronized and volatile, but I'm still not sure where/how to use them. Basically I have a thread that executes some process, and I can cancel it from an outside thread (calling cancelProcess()). Between each step I check if the process should be canceled.

public final class ExecuteProcess {
    private volatile boolean processShouldCancel;
    private volatile boolean processKilled;
    private final Object processLock = new Object();
    private Process process;

    public boolean run() {
        initialize();

        if (processShouldCancel) {
            showCanceledMsg();
            return false;
        }

        synchronized (processLock) {
            process = new Process();
            process.Start();
        }

        if (processShouldCancel) {
            if (!processKilled)
                cancelProcess();

            showCanceledMsg();
            return false;
        }

        process.waitForResponse();

        if (processKilled) {
            showCanceledMsg();
            return false;
        }

        if (process.finished()) {
            process.getResult();
            return true;
        }

        return false;
    }

    public void cancelProcess() {
        processShouldCancel = true;
        synchronized (processLock) {
            if (process != null && !processKilled) {
                boolean killed = process.kill();

                if (killed)
                    processKilled = true;
            }
        }
    }
}

Find a copy of Java Concurrency in Practice, it's got very good chapter on Cancellation and Shutdown.

You've made cancelProcess() private, so your outside threads aren't going to do anything to prevent process.waitForResponse() from blocking.  That's no good - let's make it public.

In addition to that, trying to cancel a running process by setting a volatile variable is an anti-pattern.  For your specific example, consider what happens if the external thread tries to cancel while process.waitForResponse() is running.  It doesn't do much good, does it -- ExecuteProcess can't check processKilled until the waitForReponse returns, and by then there is no point, you've already been forced to wait.

  Interruption is usually the most sensible way to implement cancellation.

As noted above, you can write an entire chapter on that problem; I'll spare us all that.  What that quotation does mean, though, is that task cancellation is a lousy example for learning about volatile variables.

Code Review

There's already a standard interface called Runnable; it's appropriate to use it here.  The runnable method returns a void though, so you should consider other ways to signal success and failure.  An alternative to Runnable would be a Callable<T>, which supports returning values.

  The Callable interface is similar to Runnable, in that both are designed for classes whose instances are potentially executed by another thread. A Runnable, however, does not return a result and cannot throw a checked exception.

Creating a process is a separate responsibility from running a process.  You should probably inject the process to be run, via the constructor.  The class might then look like:

public class ExecuteProcess implements Callable<Boolean> {
    private final Process process;

    public ExecuteProcess(Process process) {
        this.process = process;
    }

    Boolean call () {
        ...
    }
}

The advantage of Runnable and Callable: you can now take advantage of ExecutorService.submit(), which returns a Future.  Future.cancel(), under the covers, arranges for Thread.interrupt() to be called the right way.

Having done that, you can now clean up ExecuteProcess.  Instead of checking synchronized state, your process code is responsible for checking to see if the thread has been interrupted...

Executor Service and Future are part of the java.util.concurrent library; a library that gets the little details right so that you don't have to.  You can find sources on line if you want to look at the details for yourself: FutureTask.Sync.innerCancel might be a good starting point.

Also, it's really poor form to return null in a function that promises a boolean.


SMTP client to send search results I have the following working code to send search results via e-mail.  Since we have segregated domains that cannot talk to each other, the program needs to support multiple SMTP servers.

private static int[] _SMTPport = new int[2] { 25, 587 };
private static string[] _SMTPserver = new string[4]
{
  "smtp.domain1.local",
  "smtp.domain2.local",
  "smtp.domain3.local",
  "smtp.domain4.local"
};

switch (sUserDomain)
{
  case: "domain1":
  case: "DOMAIN1":
    Send_Email(_SMTPserver[0], _SMTPport[0], false);
    break;
  case: "domain2":
  case: "DOMAIN2":
    Send_Email(_SMTPserver[0], _SMTPport[0], false);
    break;
  case: "domain3":
  case: "DOMAIN3":
    Send_Email(_SMTPserver[0], _SMTPport[0], false);
    break;
  case: "domain4":
  case: "DOMAIN4":
    Send_Email(_SMTPserver[0], _SMTPport[0], false);
    break;
  default:
    Send_Email(_SMTPserver[3], _SMTPport[0], false);
    break;
}

private static void Send_Email(string _server, int _port, bool _ssl)
{
  StringBuilder emailString = new StringBuilder();
  MailMessage mMessage = new MailMessage();

  SmtpClient smtpClient = new SmtpClient(_server);
  smtpClient.Port = _port;
  smtpClient.DeliveryMethod = SmtpDeliveryMethod.Network;

  if (_ssl.Equals(true))
  { smtpClient.EnableSsl = true; }

  ServicePointManager.ServerCertificateValidationCallback = delegate(object s, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
  { return true; };

  mMessage.From = new MailAddress("from-address");
  mMessage.To.Add(sEmailAddress);

  mMessage.Subject = "Logs from " + Environment.GetEnvironmentVariable("COMPUTERNAME") + " searched " + sDate + " for " + sSearchTerm;
  emailString.Append("
Here are the file(s) on " + Environment.GetEnvironmentVariable("COMPUTERNAME") + " under " + sDate + " that contain " + sSearchTerm + "

");
  emailString.AppendLine("

Filenames
");
  int count = 0;
  foreach (var item in logList)
 {
   if (count % 2 == 0)
   {
     emailString.AppendLine("
" + Path.GetFileName(item) + "
");
   }
   else
  {
    emailString.AppendLine("
" + Path.GetFileName(item) + "
");
  }
  count++;
}
emailString.Append("

");
mMessage.IsBodyHtml = true;

mMessage.Body = emailString.ToString();

I'm working on converting it to be object-oriented.  My concern is that I need to add multiple items to the message before sending.  Here is what I have so far — am I on the correct path?

public class EmailClass
{
  private string _domain;
  private string _emailaddress;
  private string _fromaddress;
  private string _subject;
  private List<string> _message = new List<string>();

  public string Domain
  {
    get { return _domain; }
    set { _domain = value; }
  }

  public string EmailAddress
  {
    get { return _emailaddress; }
    set { _emailaddress = value; }
  }

  public string FromAddress
  {
    get { return _fromaddress; }
    set { _fromaddress = value; }
  }

  public string Subject
  {
    get { return _subject; }
    set { _subject = value; }
  }
  public List<string> Message
  {
    get { return _message; }
    set { _message = value; }
  }
}

SRP - single responsibility principle

The Send_Email() method is responsible for to many things.
The method is

creating and configuring a SmtpClient
creating and configuring a MailMessage
composing the body of the MailMessage

This should be extracted to separate methods.

So let us refactor this method.

First we write a method to create a SmtpClient.

private SmtpClient GetSmtpClient(string serverAddress, int serverPort, bool enableSsl)
{
    SmtpClient smtpClient = new SmtpClient(serverAddress);
    smtpClient.Port = serverPort;
    smtpClient.DeliveryMethod = SmtpDeliveryMethod.Network;
    smtpClient.EnableSsl = enableSsl;

    return smtpClient;
}

This had been pretty easy, hadn't it ? So, next we add a method composing the Subject by using the String.Format() method.

private String ComposeSubject(LogResult logResult)
{
    return String.Format("Logs from {0} searched {1} for {2}", logResult.ComputerName, logResult.SearchDate, logResult.SearchTerm);
}

But wait, what is LogResult ? That is a class we need to create, so let us do it. As we see it should have at least the properties ComputerName, SearchDate and SearchTerm. We add also a ReadOnlyCollection<String> FileNames property.

public class LogResult
{
    public String ComputerName { get; private set; }
    public DateTime SearchDate { get; private set; }
    public String SearchTerm { get; private set; }
    public ReadOnlyCollection<String> FileNames { get; private set; }

    public LogResult(String computerName, DateTime searchDate, String searchTerm, List<String> logList)
    {
        ComputerName = computerName;
        SearchDate = searchDate;
        SearchTerm = searchTerm;
        FileNames = ToFileNames(logList);
    }

    public LogResult(DateTime searchDate, String searchTerm, List<String> logList)
        : this(Environment.GetEnvironmentVariable("COMPUTERNAME"), searchDate, searchTerm, logList)
    { }

    private ReadOnlyCollection<String> ToFileNames(List<String> logList)
    {
        IList<String> fileNames = new List<String>(logList.Count);
        foreach (String completeFileName in logList)
        {
            fileNames.Add(Path.GetFileName(completeFileName));
        }
        return new ReadOnlyCollection<String>(fileNames);
    }
}

So now we will use this class to also compose the email body. Here we are using the feature of the StringBuilder that it is returning a StringBuilder for calling the AppendXX() methods. So we can call StringBuilder.Append().Append()...we are using also the StringBuilder.AppendFormat() method.

private String ComposeMailBody(LogResult logResult)
{
    StringBuilder bodyBuilder = new StringBuilder(1024);

    bodyBuilder.Append("
")
        .AppendFormat("Here are the file(s) on  {0}", logResult.ComputerName)
        .AppendFormat(" under {0}", logResult.SearchDate)
        .AppendFormat(" that contain {0}

",logResult.SearchTerm)
        .AppendLine("

Filenames
");

    int count = 0;
    foreach (String fileName in logResult.FileNames)
    {
        if (count % 2 == 0)
        {
            bodyBuilder.AppendFormat("
{0}
",fileName);
        }
        else
        {
            bodyBuilder.AppendFormat("
{0}
",fileName);
        }
        count++;
    }
    bodyBuilder.Append("

");

    return bodyBuilder.ToString();
}

The last method we introduce is for creating a MailMessage.

private MailMessage CreateMessage(LogResult logResult, String receiver)
{
    MailMessage message = new MailMessage("from-address", receiver);
    message.Subject = ComposeSubkject(logResult);
    message.Body = ComposeMailBody(logResult);

    return message;
}

Now the former Send_Email() method would look like

private static void SendEmail(string server, int port, bool ssl)
{
    LogResult logResult = new LogResult(sDate ,sSearchTerm, logList);
    SmtpClient smtpClient = GetSmtpClient(server, port, ssl);
    MailMessage message = CreateMailMessage(sEmailAddress);

    smtpClient.Send(message);
}

EmailClass

Using auto properties will reduce your code a lot. Auto properties are properties with a hidden backing field generated by the compiler.

public class EmailClass
{
    public string Domain { get; set; }

    public string EmailAddress { get; set; }

    public string FromAddress { get; set; }

    public string Subject { get; set; }

    public List<string> Message { get; set; }

}

Naming

Based on the naming guidlines method names should be named using PascalCase casing. So Send_Email should be renamed to SendEmail.
Based on the same guidlines method parameters should be named using camelCase casing. the signature of the former Send_Email() method should look like

private static void SendEmail(string server, int port, bool ssl)


Simple helper for making things "disposable" I often write small classes to make undisposable things disposable so that .NET can handle something automatically, like stopwatches or logs. I thought why not write a small helper that can be used instead of implementing a full class for simple things.

The Usingifier requries two delegates, one for the initialization and the other one for cleaning-up. (null checks removed for brevity)

class Usingifier<T> : IDisposable
{
    private readonly Func<T> _initialize;
    private readonly Action<T> _cleanUp;
    private T _state;

    public Usingifier(Func<T> initialize, Action<T> cleanUp)
    {
        _initialize = initialize;
        _cleanUp = cleanUp;
    }

    public Usingifier<T> Initialize()
    {
        _state = _initialize();
        return this;
    }
    public virtual void Dispose()
    {
        if (_state != null)
        {
            _cleanUp(_state);
        }
    }
}

and here's an example with a Stopwatch

using (var autoStopwatch = new Usingifier<Stopwatch>(() => Stopwatch.StartNew(), sw => sw.Elapsed.Dump()))
{
    autoStopwatch.Initialize();
    Thread.Sleep(2000);
}

It's a little difficult to give a full review without the removed null checks, but I'll give an observation:

In Dispose(), you depend on null to be the value of _state to judge whether or not to call the cleanup action. That's all well and good, but you should likely then constrain the generic parameter T to be class

class Usingifier<T> : IDisposable where T : class

You also have an issue with multiple-disposals calling the cleanup action with the state - both probably being stale after the fact. The class should be considered unusable after the cleanup. Or at least unusable until the initialize function has been called again. Simple boolean should take care of that:

class Usingifier<T> : IDisposable where T : class
{
    private readonly Func<T> _initialize;

    private readonly Action<T> _cleanUp;

    private bool _disposed;

    private T _state;

    public Usingifier(Func<T> initialize, Action<T> cleanUp)
    {
        _initialize = initialize;
        _cleanUp = cleanUp;
    }

    public Usingifier<T> Initialize()
    {
        _state = _initialize();
        _disposed = false;
        return this;
    }

    public virtual void Dispose()
    {
        if (_disposed)
        {
            return;
        }

        _disposed = true;

        if (_state != null)
        {
            _cleanUp(_state);
        }
    }
}

You may want to also put in some sort of nice (optional) automatic disposal if T itself happens to be IDisposable. Here's a quick take on that:

class Usingifier<T> : IDisposable where T : class
{
    private readonly Func<T> _initialize;

    private readonly Action<T> _cleanUp;

    private readonly bool _autoDispose;

    private bool _disposed;

    private T _state;

    public Usingifier(Func<T> initialize, Action<T> cleanUp, bool autoDispose = false)
    {
        _initialize = initialize;
        _cleanUp = cleanUp;
        _autoDispose = autoDispose;
    }

    public Usingifier<T> Initialize()
    {
        _state = _initialize();
        _disposed = false;
        return this;
    }

    public virtual void Dispose()
    {
        if (_disposed)
        {
            return;
        }

        _disposed = true;

        if (_state != null)
        {
            _cleanUp(_state);
        }

        if (_autoDispose && _state is IDisposable disposable)
        {
            disposable.Dispose();
        }
    }

I had a couple more thoughts: perhaps expose _state via a property (can then be accessed within your using scope) and then perhaps extracting an interface for the whole caboodle for mocking purposes during unit testing. Behold:

interface IUsingifier<T> : IDisposable where T : class
{
    T State { get; }

    Usingifier<T> Initialize();
}

class Usingifier<T> : IUsingifier<T> where T : class
{
    private readonly Func<T> _initialize;

    private readonly Action<T> _cleanUp;

    private readonly bool _autoDispose;

    private bool _disposed;

    public Usingifier(Func<T> initialize, Action<T> cleanUp, bool autoDispose = false)
    {
        _initialize = initialize;
        _cleanUp = cleanUp;
        _autoDispose = autoDispose;
    }

    public T State { get; private set; }

    public Usingifier<T> Initialize()
    {
        State = _initialize();
        _disposed = false;
        return this;
    }

    public virtual void Dispose()
    {
        if (_disposed)
        {
            return;
        }

        _disposed = true;

        if (State != null)
        {
            _cleanUp(State);
        }

        if (_autoDispose && State is IDisposable disposable)
        {
            disposable.Dispose();
        }
    }
}


Reduce code complexity by applying ternary operator I have a condition in my code where I need to put a lot of nested else if statements.  However, I have managed to reduce some if cases by applying ternary operator, so that code looks nice and is readable. But still I am not satisfied with the result. I would like to remove rest of the if cases too and make the code better.

Can anyone suggest more optimization in this code block?

if (!string.IsNullOrEmpty(SessionValues.AccessToken))
    {
        eventstatuscode = "logged";
        EventStatus eventStatus = rb.GetEventStatus();
        if (eventStatus != null)
        {
            if (!string.IsNullOrEmpty(eventStatus.cancelDate))
            {
                eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired": eventstatuscode;

            }

            else if (string.IsNullOrEmpty(eventStatus.cancelDate))
            {
                eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : eventstatuscode;
                eventstatuscode = eventStatus.onWaitingList == false ? "accepted" : eventstatuscode;

            }
        }
        else
        {
            eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : eventstatuscode;
        }
    }

Before deciding to use more ternary operators in your code, let's understand what if/else is about:

When the expression in the if clause is resolved to true, the block right after it is executed. If that expression is resolved to false, and there is an else block indicated, that block is executed.

An else if clause is needed if when the first condition is false there is still more than one execution option, based on a second(different) condition.

In your code you keep checking one condition, and then the opposite one, although this is totally redundant. if (x==false) is true then (x==true) is always false, and vice versa. Asking the same question and then the opposite is not only redundant, but also a maintenance risk, since you need to change the condition, you need to remember also changing the opposite one.

Also you should try to make all your conditions positive conditions - refrain from asking if(!string.IsNullOrEmpty(eventStatus.cancelDate)) - it is far better to switch the if and else blocks and ask if(string.IsNullOrEmpty(eventStatus.cancelDate)) - the ! operator ("not") is easy to miss, and may cause confusion. Of course, if only one block is implemented - no else block is better than no if block - leave the condition as it is.

To sum the above, a better code would look like this:

if (!string.IsNullOrEmpty(SessionValues.AccessToken))
{
    eventstatuscode = "logged";
    EventStatus eventStatus = rb.GetEventStatus();
    if (eventStatus != null)
    {
        if (string.IsNullOrEmpty(eventStatus.cancelDate))
        {
            eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : "accepted";
        } else {
            eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired": eventstatuscode;
        }
    }
    else
    {
        eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : eventstatuscode;
    }
}

Now it is easier to see that the same line is indicated twice - the "expired" line. By combining the two if clauses - we can reduce that to only once (this is called being DRY):

if (!string.IsNullOrEmpty(SessionValues.AccessToken))
{
    eventstatuscode = null;
    EventStatus eventStatus = rb.GetEventStatus();
    if (eventStatus != null && string.IsNullOrEmpty(eventStatus.cancelDate))
    {
        eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : "accepted";
    }
    else
    {
        eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : "logged";
    }
}


jQuery security and efficiency simple calculation code //AMOUNT CALCULATOR
            var totalAmount = 0;
            var serviceAmount = 0;

jQuery('.select-service').click(function(){
    var serviceAmount = parseInt(jQuery(this).parent().html().replace(/\D/g,''));
    if(jQuery(this).is('.selected')){
        jQuery(this).removeClass('selected');
        totalAmount -= serviceAmount;
        jQuery('#total-amount').html(totalAmount);
    }else{
        jQuery(this).addClass('selected');
        totalAmount += serviceAmount;
        jQuery('#total-amount').fadeIn('slow').html(totalAmount);
    }
});
//AMOUNT CALCULATOR

is this code efficient and secure? I am PHP dont know if it is required or not to judge.. Please any Help/feed back would be awesome! really trying to better my coding..

Security:

If you want to check amounts securely, don't do it in the browser alone: you need a server-side validation (e.g. with PHP if it is your language of choice).

Efficiency:

You can use $('...') instead of jQuery('...') to improve the readability and concision (source: jquery.com).
Avoid looking for divs with jQuery every time you need them by assigning them to a variable. In this case, caching $('#total-amount') seems unnecessary, but it will pay off as soon as you call it once more (see jQuery Best Practices for more information).

Thus:

//AMOUNT CALCULATOR
var totalAmount = serviceAmount = 0;
// caching $('#total-amount')
var totalAmountDiv = $('#total-amount');

$('.select-service').click(function(){
    // caching $(this)
    var thisDiv = $(this);
    var serviceAmount = parseInt(thisDiv.parent().html().replace(/\D/g,''));
    if(thisDiv.is('.selected')){
        thisDiv.removeClass('selected');
        totalAmount -= serviceAmount;
        totalAmountDiv.html(totalAmount);
    }else{
        thisDiv.addClass('selected');
        totalAmount += serviceAmount;
        totalAmountDiv.fadeIn('slow').html(totalAmount);
    }
});
//AMOUNT CALCULATOR


JavaScript coding style for initializing variables I've stripped most of the code and comments from what follows.

My question is about the style that it is written in, not about the correctness of the code itself.

In particular, I've never seen anyone use this way of defining and initializing variables.

The idea is to keep all the definitions together, but to attach and isolate the initialization code for each variable.
Ideally each section does nothing but initialize the named variable, with no side effects on anything else.

It works well for me, but want to know whether other people would find it too confusing.

"use strict"
if (typeof RB === 'undefined') var RB = {}
RB.tab = (function() {
   let tab_start = function(id, color1="#EFE", color2="#CDC", foldername="...") {
      let tabs = document.createElement("div"); {
         tabs.setAttribute("id", "tab_" + id)
      }
      let folder = document.getElementById(id); {
         folder.parentNode.insertBefore(tabs, folder)
         folder.appendChild(document.createElement("div"))
         folder.lastElementChild.innerHTML = "" + foldername
      }
      let c1, c2; {
         let get_rgb = function(color) {
            let value; {
               let temp = document.createElement("div")
               document.body.appendChild(temp)
               temp.style.color = color
               value = window.getComputedStyle(temp).getPropertyValue("color")
               document.body.removeChild(temp)
            }
            return value.substring(4, value.length-1).replace(/ /g, '').split(',')
         }
         c1 = get_rgb(color1)
         c2 = get_rgb(color2)
      }
      ...
   }
   ...
   return function(arg) {// RB.tab([ [id, color, color],...,[id, color, color] ])
      let url = new URL(location.href)
      ...
      window.onresize()
   }
}())

As you are not using ; consistently I would say that your init style is dangerous as it will block automatic colon insertion.

For example the following lines will throw a syntax error SyntaxError: Unexpected token '{'

 var a = foo{
    // init stuff
 }

Or syntax error Missing initializer in destructuring declaration

 var a = 0,{
    // init stuff
 }

In a sea of code this type of typo is easily overlooked. Though it is a syntax error so will not lay in wait (in most normal situations)

When you allow the ; to be inserted by moving the { to a new line it is safer.

 var a = foo
 {
    // init stuff
 }

however the concern is that you are doing more than simple assignment within the block which could lead to situation such as

 var a = {}
 {
     let a.bar = foo  // was meant to be a.bar = foo
 }

The let is a typo but will not get caught until the code is run.

If you do continue to use this style I would recommend you use semicolons and be very strict in regards to the extent of the setup code within the block.

Personally I think its ugly.


Data structure to count occurrences of entries I would love to hear any thoughtful suggestions on how to improve my programming style in order to be more understandable.

Listed below is a sample of my code which I wrote in Eclipse:

//Implement using a linked list
public class FrequencyBag<T>
{
// TO DO: Instance Variables
private Node firstNode;
private int numOfItems;

private class Node{
//Node Instance Variables

    private T data;
    private int frequency;
    private Node next;

    private Node(T Data, Node nextNode){
        data= Data;
        next=nextNode;
        frequency=1;
    }

    private void addF(){
        frequency++;
    }

    private int getF(){
        return frequency;
    }

}

/**
 * Constructor
 * Constructs an empty frequency bag.
 */
public FrequencyBag()
{
 firstNode=null;
 numOfItems =0;
}

/**
 * Adds new entry into this frequency bag.
 * @param aData the data to be added into this frequency bag.
 */
public void add(T aData)
{

  boolean found =false;
  Node currNode=firstNode;

  while(currNode !=null){
      if(currNode.data.equals(aData)){
          currNode.addF();
          found=true;
          break;
      }

      currNode= currNode.next;
  }

  if(!found){
      Node tempNode=firstNode;
      firstNode= new Node(aData,tempNode);

  }
  numOfItems++;

  }
/**
 * Gets the number of occurrences of aData in this frequency bag.
 * @param aData the data to be checked for its number of occurrences.
 * @return the number of occurrences of aData in this frequency bag.
 */
public int getFrequencyOf(T aData)
{
    Node currNode= firstNode;
    while(currNode!=null){
        if(currNode.data.equals(aData)){
            return currNode.getF();
        }
     currNode= currNode.next;
    }

    return 0;
}

/**
 * Gets the maximum number of occurrences in this frequency bag.
 * @return the maximum number of occurrences of an entry in this
 * frequency bag.
 */
public int getMaxFrequency()
{
    if(firstNode!=null){
    Node currNode= firstNode;
    Node maxNode= firstNode;
    while(currNode!=null){
        if(currNode.getF()>=maxNode.getF()){
            maxNode=currNode;
        }
        currNode=currNode.next;
    }
    return maxNode.getF();
    }
    else{
        return 0;
    }
}

/**
 * Gets the probability of aData
 * @param aData the specific data to get its probability.
 * @return the probability of aData
 */
public double getProbabilityOf(T aData)
{
  if(firstNode!=null){
    boolean flag=true;
    double dataF=0;
    double prob=0;
    Node currNode= firstNode;
    while(currNode!=null){
        if(currNode.data.equals(aData)){
             dataF=(double)currNode.getF();
             flag=false;
             break;
        }
     currNode= currNode.next;
    }
    if(!flag){
      prob= dataF/(double)numOfItems;
    }
    return prob;
  }
  else{
      return 0;
  }
}

/**
 * Empty this bag.
 */
public void clear()
{
    firstNode=null;
    numOfItems=0;
}

/**
 * Gets the number of entries in this bag.
 * @return the number of entries in this bag.
 */
public int size()
{
    return numOfItems;
}
}

Make the inner Node class static

You can do this, because this class doesn't use anything in the enclosing class.
A non-static class contains a private reference to the enclosing class,
which is just a waste when you don't really need it.

When you make the class static you will also need to add a template parameter of its own, like this:

private static class Node<T> {
    // ...
}

However, this will also mean that when you use variables of type Node, you will also need to specify the template parameter T, changing all occurrences to Node<T>.

Naming

The getF and addF methods are poorly named. It would be easier to read if you spell them out as getFrequency, and incrementFrequency.
On the other hand, in getFrequencyOf, I find the "Of" redundant,
I think you can just drop that.
Data is not a good variable name in Java,
because the convention is to use camelCase, for example data instead.

Unnecessary operations

In getMaxFrequency,
this part pointlessly compares the first node against itself:

    if (firstNode != null) {
        Node currNode = firstNode;
        Node maxNode = firstNode;
        while (currNode != null) {

Since the first if already ensures that the first node is not null,
this way would be more optimal:

    if (firstNode != null) {
        Node maxNode = firstNode;
        Node currNode = firstNode.next;
        while (currNode != null) {

Simplify using early returns

It can be a good idea to return early when the first node is null,
this way you could reduce one nesting level, which is slightly more readable,
like this:

    if (firstNode == null) {
        return 0;
    }
    Node maxNode = firstNode;
    Node currNode = firstNode.next;
    while (currNode != null) {
        if (currNode.getF() >= maxNode.getF()) {
            maxNode = currNode;
        }
        currNode = currNode.next;
    }
    return maxNode.getF();

You can apply the same logic to the getProbabilityOf method (drop the "Of", return early, iterate from firstNode.next).


Small script to re-create a file indefinitely I created a small ruby script at work to recreate a text file I use for a test in the Java application we are supporting.

I'm still really new to the ruby paradigm and wanted to know if that small script respect most of the spirit of it.

The content of the file doesn't need to make sense since the only requirement I have is that a file exist.

Since I'm doing my test every minute mostly, I set the sleeping time between checking the existence of the file to 30 seconds.

require 'pathname'

PATH = "C:/folder/to/file/test1.txt"
SLEEP_TIME = 30

path_to_text_file = Pathname.new(PATH)
while (true)
  unless path_to_text_file.exist? then
    File.open(PATH, "w") {|f| f.write("test") }
    puts "File #{PATH} created"
  end
  puts "Sleeping for #{SLEEP_TIME} seconds"
  sleep(SLEEP_TIME)
end

Could I do better? I'm on Windows and using JRuby, is there something I'm missing with my code being portable ?

Obvisously, I could make the path an argument of my script, but I don't need this at the moment.

Unless you really want to reinvent the wheel, you could use FileUtils.touch for this:

  FileUtils.touch

  Updates modification time (mtime) and access time (atime) of file(s) in list. Files are created if they don’t exist.

For example:

require 'fileutils'

PATH = "C:/folder/to/file/test1.txt"
SLEEP_TIME = 30

loop do
  FileUtils.touch(PATH)
  sleep(SLEEP_TIME)
end


Enumerate directories to a given search depth Since I had been in the need of finding directories from a startpath with different search depth levels, I created this little helper class.

This class is using objects of the System.IO namespace.

public class IOHelper
{
    public const int AllDirectories = -1;
    public const int TopDirectoryOnly = 0;
    private const char WildCard = '*';

    public static IEnumerable<String> EnumerateDirectories(String startDirectory,  int depth)
    {
        return EnumerateDirectories(startDirectory, "*.*", depth);
    }

    public static IEnumerable<String> EnumerateDirectories(String startDirectory, String searchPattern, int depth)
    {
        if (depth < AllDirectories) { throw new ArgumentOutOfRangeException("depth", "Depth needs to be -1(AllDirectories) or greater"); }

        switch (depth)
        {
            case AllDirectories:
                return Directory.EnumerateDirectories(startDirectory, searchPattern, SearchOption.AllDirectories);

            case TopDirectoryOnly:
                return Directory.EnumerateDirectories(startDirectory, searchPattern, SearchOption.TopDirectoryOnly);

            default:
                return EnumerateDirectoriesInDepth(startDirectory, searchPattern, depth);
        }
    }

    private static IEnumerable<String> EnumerateDirectoriesInDepth(String startDirectory, String searchPattern, int depth)
    {
        IEnumerable<String> directories = EnumerateDirectoriesInDepth(startDirectory, depth);

        Boolean isWildCardSearch = searchPattern.Contains(WildCard);

        if (!isWildCardSearch)
        {
            searchPattern = String.Concat("\\", searchPattern);
            return directories.Where(d => d.EndsWith(searchPattern));
        }

        IEnumerable<String> enumeratedDirectories = Enumerable.Empty<String>();
        foreach (String directory in directories)
        {
            enumeratedDirectories = enumeratedDirectories.Concat(Directory.EnumerateDirectories(directory, searchPattern));
        }

        return enumeratedDirectories;
    }

    private static IEnumerable<String> EnumerateDirectoriesInDepth(String startDirectory, int depth)
    {
        if (depth <= TopDirectoryOnly) { return Enumerable.Empty<String>(); }

        IEnumerable<String> directories = Directory.EnumerateDirectories(startDirectory, "*.*", SearchOption.TopDirectoryOnly);

        if (!directories.Any()) { return directories; }

        depth = depth - 1;

        IEnumerable<String> subDirectories = Enumerable.Empty<String>();
        foreach (String directory in directories)
        {
            subDirectories = subDirectories.Concat(EnumerateDirectoriesInDepth(directory, depth));
        }

        return directories.Concat(subDirectories);
    }
}

I would like to get a review on all aspects.

Since the class has only static and constant members, you should make the entire class static.  It eliminates the unnecessary empty instance constructor the framework automatically generates.  (see code analysis rules CA1052 and CA1053).

You should also generally use the type aliases provided (i.e., string instead of String, bool instead of Boolean, etc. - see StyleCop rule SA1121).

Additionally, I would change the recursion in EnumerateDirectoriesInDepth so it does not have to recurse another step when the depth is zero.

private static IEnumerable<string> EnumerateDirectoriesInDepth(string startDirectory, int depth)
{
    var directories = Directory.EnumerateDirectories(startDirectory, "*.*", SearchOption.TopDirectoryOnly);

    if (depth == TopDirectoryOnly || !directories.Any())
    {
        return directories;
    }


Count bits in VBScript I'm trying to write a function that returns the number of bits set in a 32-bit integer in VBScript, it's just for practising the language. The function I've written so far looks okay, but I'm wondering if there was a simpler solution.

Function countBits(value)
  Dim n : n = 0
  Dim mask : mask = 1
  Dim i
  For i = 0 to 30
    If (value And mask) > 0 Then
      n = n +1
    End If
  Next
  If (value And &h8000) then
    n = n+1
  End If
  countBits = n
End Function

I found that there are no shift operators in VBScript and an overflow (Err.Number = 6) if I iterate for 0 to 31, that's why I add the explicitly check of the MSB after the look.

Any solutions to improve (maybe generalize) this?

edit: removed the useless error handling part, as it's a remains of starting with 31 as an upper bound in the first (and at that time only) loop

According to information in VBScript Data Types MSDN article, a 32-bit integer is long variant subtype.

However, the value &h8000 (decimal  -32768) seems to be a 16-bit integer minimal value; it's hexadecimal &hFFFF8000 if converted to long subtype (check Wscript.Echo Hex(&h8000).

This means that your function miscounts 1 additional bit for all long values above 32767 i.e. above 16-bit integer maximal value &h7FFF.

To check the sign bit of a long value correctly, use &h80000000 as follows:

Function countBits(value)
  Dim mask : mask = 1
  Dim i
  countBits = 0                    ' no need to use any auxiliary variable
  For i = 0 to 30
    If ( value And mask ) > 0 Then
      countBits = countBits + 1
    End If
    mask = mask * 2                ' got lost due to your own edit Dec 11 '16 at 14:22
  Next
  If ( value And &h80000000 ) Then ' check the sign bit
    countBits = countBits + 1
  End If
End Function

Sample tests:

Wscript.Echo countBits(2^16)         '  1
Wscript.Echo countBits(2^30)         '  1
Wscript.Echo countBits(1)            '  1
Wscript.Echo countBits(-1)           ' 32
Wscript.Echo countBits(-2)           ' 31
Wscript.Echo countBits(-2147483648)  '  1


Using brute force to do large scale permutations and combinations This problem should self explanatory based on namings of variables and functions. In essence I am doing well over 720*720*90 checks/calculations. Hence, this is taking far, far too long. How do I make my code more optimized with regards to time/performance. Space is not an issue. The question is pretty complicated(at least in my opinion), so please let me know what I should clarify.

Most of the optimization should be doable by just reading my comments, I hope. I don't want to bog down the task by adding in details of the convoluted math problem.

import java.io.*;
import java.util.*;
import java.lang.*;

public class Concat{
    static String[] designs = new String[720]; //6! permutations of 6 unique characters
    static int index = 0; //index used to iterate through designs[]

    public static void main(String[] args){
        Concat concat = new Concat();
        genDesigns("", "123456"); //123456 will be the unique characters

        String[][] interleavedData = new String[720][720];
        for(int i = 0; i < 720; i++)
            interleavedData[i] = //remap using design[i]

        String[][] interleavedSubsets = new String[720][90];
        for(int interleaveIndex = 0; interleaveIndex < 720; interleaveIndex++)
        {
            for(int j = 0; j < 720; j++)
                interleavedSubsets[j] = gen8String(genDelPatterns(designs[j]), genDelPatterns(interleavedData[interleaveIndex][j]));

            System.out.println(checkSubsets(interleavedSubsets));
        }

    }

    //generate all permutations
    public static void genDesigns(String prefix, String data){
        int n = data.length();
        if (n == 0) designs[index++] = prefix;
        else {
            for (int i = 0; i < n; i++)
                genDesigns(prefix + data.charAt(i), data.substring(0, i) + data.substring(i+1, n));
        }
    }

    public static String[] genDelPatterns(String design){
        //stimulating 6choos4 operation
    }

    //generating a subset of 90 eight character strings (unique deletion patterns)
    public static String[] gen8String(String[] pattern1, String[] pattern2){
        String[] combinedSubset = new String[90]; //emty array for the subset of 90 strings
        String  combinedString = ""; //string holder for each combined string
        int index = 0; //used for combinedSubset array
        int present = 0; //used to check if all 6 characters are present

        for(int i = 0; i < 15; i++){

            for(int j = 0; j < 15; j++){
                combinedString = pattern1[i] + pattern2[j]; //combine both 4 letter strings into 8 char length string
                char[] parsedString = combinedString.toCharArray(); //parse into array

                //check if all 6 characters are present
                for(int k = 1; k <= 6; k++)
                {
                    if(new String(parsedString).contains(k+"")) {
                        present++;
                    }
                    else
                        break;
                    //if all 6 are present, then add it to combined subset
                    if(present == 6)
                        combinedSubset[index++] = combinedString;
                }
                present = 0;
            }
        }
        return combinedSubset;
    }

    //check all the subsets of the interleaved data
    public static int checkSubsets(String[][] subsets){
        List subset = new ArrayList();
        int numOfUnique = 0;
        for(int i = 0; i< 720; i++){
            for(int j = 0; j < 90; j++)
                subset.add(subsets[i][j]);
        }
        Object duplicate;
        Iterator itr = subset.iterator();
        while(itr.hasNext()){
            duplicate = itr.next();
            while(itr.hasNext()){
                subset.remove(duplicate);
                itr=subset.iterator(); //to avoid concurrent modification
                itr.next();
            }
        }
        return subset.size();
    }
}

Use integers instead of strings

I don't really understand the problem you are trying to solve, so I don't know if your algorithm can be improved.  But I can see that your program spends a lot of time creating and modifying strings that are between 4 and 8 characters long, containing the letters '1' .. '6'.

Instead of using strings, you could use an integer to represent your string.  Simply use the hex representation as your integer value.  For example, instead of the string "123456", use the int 0x123456.

Integers are more compact than strings, and operations on integers are faster than string operations.  As an example of how to build an integer as opposed to a string, this:

 sb.append(String.valueOf(data[k]));

would become this:

// Assumes k loop goes from 5 to 0 instead of 0 to 5:
val = (val << 4) | ((data >> k) & 0xf);

More efficient check for all characters present

One function that appears to be where much of the time is spent is gen8string(), and within that function the inner loop does this:

    for(int i = 0; i < 15; i++){

        for(int j = 0; j < 15; j++){
            combinedString = pattern1[i] + pattern2[j]; //combine both 4 letter strings into 8 char length string
            char[] parsedString = combinedString.toCharArray(); //parse into array

            //check if all 6 characters are present
            for(int k = 1; k <= 6; k++)
            {
                if(new String(parsedString).contains(k+"")) {
                    present++;
                }
                else
                    break;
                //if all 6 are present, then add it to combined subset
                if(present == 6)
                    combinedSubset[index++] = combinedString;
            }
            present = 0;
        }
    }

There are several inefficiencies, such as concatenating pattern1[i] and rechecking its characters when they could be checked in the outer loop only.  If you converted to integers, you could do this check like this:

// Returns bitmask of numbers present in a 4 digit value
public static int presentBits(int val)
{
    int presentMask = 0;
    for (int i=0; i<16; i+=4) {
        int digit = (val >> i) & 0xf;
        presentMask |= 1 << digit;
    }
    return presentMask;
}

    // Within get8string()...
    for (int i = 0; i < 15; i++) {
        int present1 = presentBits(pattern1[i]);
        for (int j = 0; j < 15; j++) {
            int presentMask = present1 | presentBits(pattern2[j]);

            if (presentMask == 0x7e) {
                // 0x7e is binary 01111110, which means each of
                // the digits 1 through 6 was seen at least once.
                combinedSubset[index++] = (pattern1[i] << 16) |
                                           pattern2[j]
            }
        }
    }

Checking subsets

Another heavily called function is checkSubsets(), which appears to just count the number of unique strings among an array of 720x90 strings.  It does so with an \$O(n^2)\$ duplicate removal algorithm.  You could do better by sorting the strings (or integers preferably) and then counting uniques in one pass.  Even faster would be to add all the strings to a HashSet, which naturally removes duplicates.  The sorting method takes \$O(n \log n)\$ time, and the HashSet method takes \$O(n)\$ time.


Async Controller Play 2.3 Is this example the correct way of writing an Async controller end point in Scala using Play 2.3?

def submitResponse(qkey:String) = CorsAction.async(parse.json){
    req =>
      val json = req.body
      val channel = json.\("section").as[String]

      val currentQuestionKey = Cache.getAs[String](channel).getOrElse("N_A")
       if(!currentQuestionKey.equals(qkey)){
        scala.concurrent.Future{BadRequest("Inactive or Invalid Key")}
      }
      else {

        val futureResponse = scala.concurrent.Future {
          responseService.create(qkey,channel)
        }
        futureResponse.map{response =>
          response match {
            case Failure(x) => { BadRequest("Duplicate " + qkey)}
            case Success(storedResponse) => { Ok(storedResponse)}
          }
        }

      }

  }

Let's assume the following enclosing context for your method, this gives us types to work with (types are very important in scala). Notice I changed back from CorsAction to Action since that shouldn't change anything to the types and I don't have the implementation for CorsAction at hand. Also your code doesn't give enough context to know which execution context you have in scope.

import play.api.cache.Cache
import play.api.mvc.{Action, Controller}

import scala.concurrent.Future
import scala.util.{Try, Success, Failure}

trait ResponseService{
  def create(key:String, channel:String):Try[String]
}

class Temp(responseService:ResponseService) extends Controller  {
  def submitResponse(qkey:String) = Action.async(parse.json){ req =>
    ???
  }
}

I assume you have tests for your code so I can start refactoring right away. The first thing is not going through a thread to produce a future for which we know the result and since we have imports we can use short names:

if(!currentQuestionKey.equals(qkey)){
  scala.concurrent.Future{BadRequest("Inactive or Invalid Key")}
}

should really be

if(!currentQuestionKey.equals(qkey)){
  Future.successful(BadRequest("Inactive or Invalid Key"))
}

the Future.apply will execute asynchronously in a different thread capturing exceptions as needed, here that would mean a useless context switch to assign a static value which can not throw exceptions.

The same more or less holds true for

val futureResponse = scala.concurrent.Future {
  responseService.create(qkey,channel)
}
futureResponse.map{response =>
  response match {
    case Failure(x) => { BadRequest("Duplicate " + qkey)}
    case Success(storedResponse) => { Ok(storedResponse)}
  }
}

Because of the pattern matching which is below  the Future instantiation, it seems that the return type of responseService.create is a Try[T].

futureResponse currently has a Future[Try[T]] type which encodes redondant information (see below). The futureResponse.map call can be simplified as :

futureResponse.map{
  case Failure(x) => BadRequest("Duplicate " + qkey)
  case Success(storedResponse) => Ok(storedResponse)
}

which eliminates a bit of boilerplate. By the way, curly braces are unneeded around case expression's bodies since there is no "fall through" to the next case once a case has been matched.

Try[T] will catch the same exceptions which would be caught by Future (NonFatal exceptions) therefore I will assume that the responseService.create is a very long operation. If it is not the case, then using an asynchronous controller makes no sense. If it is indeed the case and you control the signature of responseService.create, I suggest having it return a Future[T] instead of a Try[T].

Assuming you can't change the signature of responseService.create, there is still a  change (debatable) that can be made.

val futureResponse = Future(responseService.create(qkey,channel).get)
futureResponse.map( storedResponse => Ok(storedResponse) )
              .recover{ case t:Throwable => BadRequest("Duplicate " + qkey) }

I assume the type of the exception in the failure doesn't matter since your pattern matching was ignoring it too.

I have avoided Future.fromTry since that would not execute responseService.create asynchronously.

The complete code in context would look like:

import play.api.cache.Cache
import play.api.mvc.{Action, Controller}

import scala.concurrent.Future
import scala.util.{Try, Success, Failure}

trait ResponseService{
  def create(key:String, channel:String):Try[String]
}

class Temp(responseService:ResponseService) extends Controller  {
  // in scala 2.11, use
  def fromTry[T](t:Try[T]):Future[T] = {
    t.map(Future.successful).recover{case t:Throwable=>Future.failed(t)}.get
  }
  def submitResponse(qkey:String) = Action.async(parse.json){
    req =>
      val json = req.body
      val channel = json.\("section").as[String]

      val currentQuestionKey = Cache.getAs[String](channel).getOrElse("N_A")
      if(!currentQuestionKey.equals(qkey)){
        Future.successful(BadRequest("Inactive or Invalid Key"))
      }
      else {
        val futureResponse = Future(responseService.create(qkey,channel).get)
        futureResponse.map{ storedResponse => Ok(storedResponse) }
                      .recover{ case t:Throwable => BadRequest("Duplicate " + qkey)}
      }

  }
}

Actually there is a bit more which can be improved.

  val json = req.body
  val channel = json.\("section").as[String]

json is never used anywhere else, it can be inlined.

  val channel = req.body.\("section").as[String]

This will throw an exception and return a status 500 if section is not a String.

curl -X POST -H"Content-Type: application/json" -d '{"section":1234}' localhost:9000/temp/ABC

will result in

[error] [play-akka.actor.default-dispatcher-2] play - Cannot invoke the action, eventually got an error: play.api.libs.json.JsResultException: JsResultException(errors:List((,List(ValidationError(error.expected.jsstring,WrappedArray())))))
[error] [play-akka.actor.default-dispatcher-2] application -

! @6l2ocbc00 - Internal server error, for (POST) [/temp/ABC] ->

play.api.Application$$anon$1: Execution exception[[JsResultException: JsResultException(errors:List((,List(ValidationError(error.expected.jsstring,WrappedArray())))))]]
     at play.api.Application$class.handleError(Application.scala:296) ~[play_2.10-2.3.6.jar:2.3.6]
     at play.api.DefaultApplication.handleError(Application.scala:402) [play_2.10-2.3.6.jar:2.3.6]
     at play.core.server.netty.PlayDefaultUpstreamHandler$$anonfun$3$$anonfun$applyOrElse$4.apply(PlayDefaultUpstreamHandler.scala:320) [play_2.10-2.3.6.jar:2.3.6]
     at play.core.server.netty.PlayDefaultUpstreamHandler$$anonfun$3$$anonfun$applyOrElse$4.apply(PlayDefaultUpstreamHandler.scala:320) [play_2.10-2.3.6.jar:2.3.6]
     at scala.Option.map(Option.scala:145) [scala-library.jar:na]
    Caused by: play.api.libs.json.JsResultException: JsResultException(errors:List((,List(ValidationError(error.expected.jsstring,WrappedArray())))))
     at play.api.libs.json.JsValue$$anonfun$2.apply(JsValue.scala:67) ~[play-json_2.10-2.3.6.jar:2.3.6]
    at play.api.libs.json.JsValue$$anonfun$2.apply(JsValue.scala:67) ~[play-json_2.10-2.3.6.jar:2.3.6]
    at play.api.libs.json.JsResult$class.fold(JsResult.scala:77) ~[play-json_2.10-2.3.6.jar:2.3.6]
    at play.api.libs.json.JsError.fold(JsResult.scala:13) ~[play-json_2.10-2.3.6.jar:2.3.6]
    at play.api.libs.json.JsValue$class.as(JsValue.scala:65) ~[play-json_2.10-2.3.6.jar:2.3.6]

To avoid that you can use the validation API.

(req.body \ "section").validate[String]

will return a JsResult[String] which is either a JsSuccess(value, path) or a JsError(errors).

You can fold over that or map+getOrElse depending on your tastes. I suggest extracting the successul case to a specific method for improved readability.

The final code looks like :

import play.api.cache.Cache
import play.api.mvc.{Result, Action, Controller}

import scala.concurrent.{ExecutionContext, Future}
import scala.util.{Try, Success, Failure}

class ResponseService{
  def create(key:String, channel:String):Try[String] = Try("x")
}

class Temp(responseService:ResponseService)(implicit app:play.api.Application, executor:ExecutionContext) extends Controller  {
  // in scala 2.11, use
  def fromTry[T](t:Try[T]):Future[T] = {
    t.map(Future.successful).recover{case t:Throwable=>Future.failed(t)}.get
  }
  def submitResponse(qkey:String) = Action.async(parse.json){  req =>
    def respondWithChannel(channel:String): Future[Result] = {
      val currentQuestionKey = Cache.getAs[String](channel).getOrElse("N_A")
      if(!currentQuestionKey.equals(qkey)){
        Future.successful(BadRequest("Inactive or Invalid Key"))
      } else {
        val futureResponse = Future(responseService.create(qkey,channel).get)
        futureResponse.map{ storedResponse => Ok(storedResponse) }
                      .recover{ case t:Throwable => BadRequest("Duplicate " + qkey)}
      }
    }

    (req.body \ "section").validate[String].fold(
      error => Future.successful(BadRequest("section was not a valid JSON string ")),
      respondWithChannel
    )
  }
}

Notice that I chose to use a class, which means you will have to inject the necessary depencies through the constructor. For local testing, I used a static compile time injection through an object instantiation:

object Temp extends Temp(new ResponseService)(play.api.Play.current,
play.api.libs.concurrent.Execution.defaultContext)

The corresponding route being

POST    /temp/:qkey     controllers.Temp.submitResponse(qkey:String)

Also note that by using the parse.json body parser any call which is made to your action without providing a json body will be rejected with a 400 error generated in the framework.


I have a Java Class that I suspect can use a ReflectionUtil here because there are too many switch/cases, any advice? I have a chunk of code here:
public class ParserFactory
{
    public static ParserClass getParserClass(InputStream is, CustomMap customMap,
                                           DataHandler recordHandler, DataHandler columnHandler) throws JsonProcessingException
    {
        ParserClassType ParserClassType = ParserClassType.getParserClassType(customMap.getParser());
        switch (ParserClassType)
        {
            case SPECIFIC_XML:
                return new SpecificXMLParser(is, customMap.getProperties(), recordHandler, columnHandler);
            case XML:
                return new GenericXMLParser(is, customMap.getProperties(), recordHandler, columnHandler);
            case JSON:
                return new GenericJSONParser(is, customMap.getProperties(), recordHandler, columnHandler);
            case SPECIAL:
                return new SpecialParser(is, customMap.getProperties(), recordHandler, columnHandler);
            case FW:
                return new GenericFixedWidthParser(is, customMap.getProperties(), recordHandler, columnHandler);
            case CSV:
                return new GenericCSVParser(is, customMap.getProperties(), recordHandler, columnHandler);
            default:
                throw new IllegalArgumentException("ParserClassType not found.");
        }
    }

    public static String getFileExtension(CustomMap customMap)
    {
        ParserClassType ParserClassType = ParserClassType.getParserClassType(customMap.getParser());
        switch (ParserClassType)
        {
            case SPECIFIC_XML:
                return SpecificXMLParser.FILE_TYPE_OUTPUT;
            case XML:
                return GenericXMLParser.FILE_TYPE_OUTPUT;
            case JSON:
                return GenericJSONParser.FILE_TYPE_OUTPUT;
            case SPECIAL:
                return SpecialParser.FILE_TYPE_OUTPUT;
            case FW:
                return GenericFixedWidthParser.FILE_TYPE_OUTPUT;
            case CSV:
                return GenericCSVParser.FILE_TYPE_OUTPUT;
            default:
                throw new IllegalArgumentException("ParserClassType not found.");
        }
    }
}

Now, it's not exactly too important what all these arguments are. In getParserClass, I retrieve an enum from ParserClassType. The switch handles all the possible values. A certain value will return a ParserClass.
The other function, getFileExtension, retrieves a static string from a parser class.
As my project grows, I will have many new parsers that I add. I want to make this as flexible and less-hectic as possible. I don't want to keep adding cases to this function. So.
I was looking into EnumMap. I was thinking of mapping an ENUM to a ParserClass name, so for example "XML" would make to "GenericXMLParser.class", and then I could use a ReflectionUtil to create the class with the arguments necessary or retrieve that string. Therefore, if I have to make any additions, all I need to do is add the ENUM to the enum class and make an addition to the map. But, ReflectionUtils seems to require me to provide the path of the parser. Something like "com.package.service.more.bs.SpecificXMLParser". I would rather not do that...
Any ideas?

The only problem I see with this code is, that for extending the enum, you need to change two methods in the code, which spreads out changes instead of keeping them local to a single place.
You could however harness the power of the java enum (which can have fields and methods) to make the changes locally.
Something along the lines of:
enum ParserClassType {
    SPECIFIC_XML(SpecificXMLParser.FILE_TYPE_OUTPUT) {
        public ParserClass createParser(InputStream is, etc, ...) {
            return new SpecificXMLParser(...);
        }
    },
    XML(GenericXMLParser.FILE_TYPE_OUTPUT) {
        public ParserClass createParser(InputStream is, etc, ...) {
            return new GenericXMLParser(...);
        }
    },
    ...

    private final String fileExtension;

    private ParserClassType(String fileExtension) {
        this.fileExtension = fileExtension;
    }

    public String getFileExtension() {
        return fileExtension;
    }

    public abstract ParserClass createParser(InputStream is, etc, ...);
}

public static String getFileExtension(...) {
    ParserClassType parserClassType = ...
    return parserClassType.getFileExtension();
}

public ParserClass getParserClass(...) {
    ParserClassType parserClassType =  ...
    return parserClassType.createParser(...);
}

As you see, when you add a new type, you simply have to extend the enum. All the client code stays unmodified.
Apart from that, please look up the java naming conventions. Variables should start with lower case letters and definitely not shadow their class names.
Furthermore, getParserClass() / ParserClass is badly named, as it clearly involves around a concrete parser / parserInstance, not around a class.


Jon Bentley's bitmap sort implementation I've implemented Jon Bentley's bitmap sorting algorithm from Column 1 of his Programming Pearls book. This code supports user-defined integer range (works from Integer.MIN_VALUE to Integer.MAX_VALUE), but for now only works on a distinct set of integers.

In order to avoid overflow in the integer arithmetic, I use type-casting for intermediate results.

For anyone interested, I've also posted the full description of the algorithm and performance analysis here.

public int[] sortDistinctIntegers(int[] a, int min, int max){

    int N = (int)(((long)max-min) / 8 + 1);
    byte[] bitmap = new byte[N];

    for(int i = 0; i < a.length; i++)
        bitmap[(int)(((long)a[i]-min)/8)] |= 1 << (int)(((long)a[i]-min) % 8);

    int k = 0;
    for(int i = 0; i < N; i++){
        for(int j = 0; j < 8; j++){
            if((bitmap[i] & (1 << j)) > 0){
                a[k] = i * 8 + j + min;
                k++;
            }
        }
    }

    return a;
}

I was wondering if anyone has any comments or ideas on how to improve this code (in terms of prettification, readability and performance).

An idea: I'd try to implement the algorithm with bigger data types (int, long) instead of the byte.

Some notes:

I'd create a BYTE_SIZE constant for 8, try to avoid magic numbers.

private static final int BYTE_SIZE = 8;

It would help a lot if you change from byte to another data type.
The return type of sortDistinctIntegers is really confusing. It returns with the first argument. Users easily could believe that it returns another array despite the fact that it modifies the input array. Make it void.
For easier readability I'd

extract out some method for easier readability: createBitmapArray, processBitmapByte, doSort,
use longer variable and parameter names (outputIndex instead of k, bitmapSize instead of N, bitIndex instead of j),
create local variables for 1 << bitIndex (mask), etc.

Below is the code after a few refactoring steps. (I haven't checked that the algorithm is correct or not.)

private static final int BYTE_SIZE = 8;

public void sortDistinctIntegers(final int[] data, final int min, final int max) {
    final byte[] bitmap = createBitmapArray(data, min, max);

    doSort(data, min, bitmap);
}

private byte[] createBitmapArray(final int[] data, final int min,
        final int max) {
    final int bitmapSize = (int) (((long) max - min) / BYTE_SIZE + 1);
    final byte[] bitmap = new byte[bitmapSize];

    for (int i = 0; i < data.length; i++) {
        final long min2 = (long) data[i] - min; // TODO: need a better name
        final int mask = 1 << (int) (min2 % BYTE_SIZE);
        bitmap[(int) (min2 / BYTE_SIZE)] |= mask;
    }
    return bitmap;
}

private void doSort(final int[] data, final int min, final byte[] bitmap) {
    int outputIndex = 0;
    for (int bitmapArrayIndex = 0; bitmapArrayIndex < bitmap.length;
            bitmapArrayIndex++) {
        final byte currentBitmapByte = bitmap[bitmapArrayIndex];
        outputIndex = processBitmapByte(data, min, currentBitmapByte,
                outputIndex, bitmapArrayIndex);
    }
}

private int processBitmapByte(final int[] data, final int min,
        final byte currentBitmapByte, int outputIndex,
        final int bitmapArrayIndex) {
    for (int bitIndex = 0; bitIndex < BYTE_SIZE; bitIndex++) {
        final int mask = 1 << bitIndex;
        final boolean flagSet = (currentBitmapByte & mask) > 0;
        if (flagSet) {
            data[outputIndex] = bitmapArrayIndex * BYTE_SIZE + bitIndex + min;
            outputIndex++;
        }
    }
    return outputIndex;
}


Extract images from a PDF or ZIP file Input

inputBlob - A PDF or ZIP file containing all the pages of many students' test papers.
extension - The file type ("pdf" or "zip").
numPages - The number of pages in a single test. For example, if numPages is 2 and the input file contains 10 pages, then we must have 5 students' tests.

Routine

Extract all the pages as images.
Upload the images to blob storage.
For each test (i.e. set of images), queue another routine to process it.

Code

I've created PdfPack and ZipPack classes (implementations not included here) which implement a common interface:

public interface IPack : IDisposable
{
  void Open(Stream stream);
  Stream GetPage(int index);
  int PageCount { get; }
}

Here's the function itself:

public static async Task Run(
  CloudBlockBlob inputBlob,
  string extension,
  int numPages,
  CloudBlobContainer container,
  IAsyncCollector<StitchRequest> queue)
{

  IPack pack = null;

  try {

    using (var inputStream = new MemoryStream()) {
      await inputBlob.DownloadToStreamAsync(inputStream);

      switch (extension.ToLowerInvariant()) {
        case "zip":
          pack = new ZipPack();
          break;
        case "pdf":
          pack = new PdfPack();
          break;
        default:
          throw new InvalidOperationException("Invalid file type.");
      }

      pack.Open(inputStream);

      if (pack.PageCount == 0) {
        throw new InvalidOperationException("No pages found.");
      }

      if (pack.PageCount % numPages != 0) {
        throw new InvalidOperationException($"Pages not a multiple of {numPages}.");
      }

      Guid groupId = default;

      for (var i = 0; i < pack.PageCount; i++) {

        if (i % numPages == 0) {
          groupId = Guid.NewGuid();
        }

        using (Stream imageStream = pack.GetPage(i)) {
          var imageBlob = container.GetBlockBlobReference($"{groupId}/{i % numPages}");
          await imageBlob.UploadFromStreamAsync(imageStream);
        }

        if (i % numPages == numPages - 1) {
          await queue.AddAsync(new StitchRequest {
            GroupId = groupId.ToString("D"),
            Count = numPages
          });
        }

      }
    }

    await inputBlob.DeleteAsync();

  } finally {
    pack?.Dispose();
  }

}

"Egyptian" brackets is uncommon practice for C#. You should place an opening brace on separate line.

I suggest to use Dictionary every time you deal with mapping:

private const string ZipExtension = "zip";
private const string PdfExtension = "pdf";

private static readonly Dictionary<string, Func<IPack>> PackCreators =
    new Dictionary<string, Func<IPack>>(StringComparer.OrdinalIgnoreCase)
    {
        [ZipExtension] = () => new ZipPack(),
        [PdfExtension] = () => new PdfPack()
    };

Then you can obtain an instance of pack with this code:

if (!PackCreators.TryGetValue(extension, out var packCreator))
    throw new InvalidOperationException("Invalid file type.");

var pack = packCreator();

Once you've got a pack instance you can perform all actions with it inside using block:

using (pack)
{
    pack.Open(inputStream);
    // ...
}

which eliminates the need to define pack outside of try-finally and the need of try-finally itself with explicit pack disposing.

You calculate i % numPages multiple times in your loop. Define a variable and use it:

var remainder = i % numPages;

if (remainder == 0)
// ...
$"{groupId}/{remainder }"
// ...
if (remainder == numPages - 1)


Java creating augmented array of size 400,000,000 I was solving one competitive coding question having integer bounds 2^100. Fortunately, it was a dp question and I didn't need an array of that size. Practically we cannot create an array of size more than Integer.MAX_VALUE.

But I was thinking of creating a multidimensional array and treat it as a one-dimensional array.

what I need was a very large array with O(1) retrieval time. So Internally it will be a multidimensional array but I will treat it as a one-dimensional array.
So the size of the resultant array will be n*n.

if n is Integer.MAX_VALUE then the resultant augmented array will be of Integer.MAX * Integer.MAX

Here what I did to implement this idea.
BigArray class
public class BigArray
{

    private int[][] arr;
    private int row=0;
    private int col = 0;
    private int size;
    public BigArray(int size)
    {
        arr = new int[size][size];
        this.size = size;
    }
}

add method
public void add(int data)
    {
     if(row > size-1)
        {
            col++;
            row=0;
            this.arr[col][row] = data;
            row++;
        }
        else
        {
            this.arr[col][row] = data;
            row++;
        }
    }

get method
public int get(int pos)
    { //get value in O(1)
        if((int) Math.sqrt(pos) > size)
            return -1;
        int col1 = pos/(size);
        int row1 = pos%(size);
        return arr[col1][row1];
    }

But If I pass Integer.MAX_VALUE in BigArray it throws Exception in thread "main" java.lang.OutOfMemoryError: Requested array size exceeds VM limit
maximum size I've tried is 20,000 so resultant size will be 20,000 x 20,000 = 400,000,000.
But if add one more dimension to this array I'll get more space.
if BigArray has arr[][][] resultant size will be n x n x n;
if BigArray has arr[][][][] resultant size will be n x n x n x n; and so on.
I know I had to implement necessary methods as I add a new dimension to the base array.
But I would like a review on this type of storage class. I don't know

what's the maximum dimensions java array can have?

Also, do tell me

if can use this approach to create arrays size more than the traditional array size
Future scope such as generic BigArray of character will create string greater size
other opinions/suggestions on this approach

The biggest problem I can see with this approach, is that a user can't add or get any index higher than Integer.MAXVALUE.  You might need to parse Strings or use a BigInteger type instead of int for those methods.


Exposing a web API wrapper library to consumers I'm writing a C# library that's a wrapper around the Yahoo Finance YQL API. I'm stuck on how I want to expose my API to consumers. My design thus far consists of separate interfaces for the different parts of the Yahoo Finance API:

public interface IYahooFinanceCharts {
    Task<Image> GetChartAsync(string tickerSymbol, ChartSize size, ChartTimeSpan timeSpan);
}

public interface IYahooFinanceQuotes {
    Task<IEnumerable<Quote>> GetQuotesAsync(params string[] tickerSymbols);
}

public interface IYahooFinanceStocks {
    Task<IEnumerable<Stock>> GetStocksAsync(params string[] tickerSymbols);
}

public interface IYahooFinanceTickerSymbols {
    Task<IEnumerable<TickerSymbol>> SearchAsync(string symbolOrKeyword);
}

I have internal classes that implement these interfaces to handle the details of building a request for the YQL API and parsing the result of the response. I don't want to expose these classes to consumers.

I was thinking of using a facade to reduce the main entry point into my API to one class. Consumers could cast, instantiate, or register (DI container) the facade using the various interfaces to zero-in on the parts they're interested in.

public class YahooFinance : IYahooFinanceCharts, IYahooFinanceQuotes, IYahooFinanceStocks, IYahooFinanceTickerSymbols
{
    // Delegating calls to the internal classes that implement these interfaces...
}

...

IYahooFinanceStocks stocks = new YahooFinance();
var results = await stocks.GetStocksAsync("MSFT", "GOOG");

I'm not sure if I like this because if consumers simply use

var yahooFinance = new YahooFinance();

the entire API is exposed to them, defeating the purpose of the separate interfaces. On the other hand, not using a facade and forcing consumers to instantiate the concrete class for each interface is clumsy, increases the surface area of the API, and I'd have to expose the concrete types...

What you're looking for is actually the factory pattern.

Your implementation could be something like

public class YahooFinanceFactory{

   public IYahooFinanceQuotes GetQuotes()
   {
     return new YahooFinanceQuotesConreteImplementation();
   }

[...]

}

The concrete implementations are still marked as internal so they are not exposed.

Alternatively, you could use reflection for this.

public static T ResolveConcreteType<T>()
{
    return (from t in Assembly.GetExecutingAssembly().GetTypes()
            where t.GetInterfaces().Contains(typeof(T))
            && t.IsClass //Find the classes that implement the interface we want
            select (T)Activator.CreateInstance(t, null))
        .SingleOrDefault();

}

Usage:

var quotes = YahooFinanceFactory.ResolveConcreteType<IYahooQuotes>();

This makes the assumption that your concrete types support empty constructors however.


turn a dict that may contain a pandas dataframe to several dicts I have a dict that may be 'infinitely' nested and contain several pandas DataFrame's (all the DataFrame's have the same amount of rows).

I want to create a new dict for each row in the DataFrame's, with the row being transformed to a dict (the key's are the column names) and the rest of the dictionary staying the same.

Note: I am not making a cartesian product between the rows of the different DataFrame's.

I have an example in my SO question

here is what i came up with:

import pandas as pd
from copy import deepcopy
from functools import partial

def map_keys_by_type(d, typ, path=None):
    for k,v in d.items():
        p = path.copy() if path else []
        p.append(k)
        if isinstance(v, typ):
            yield p
        if isinstance(v, dict):
            yield from map_keys_by_type(v, typ, p)

def nested_get(nested_key, input_dict):
    internal_dict_value = input_dict
    for k in nested_key:
        internal_dict_value = internal_dict_value.get(k, None)
        if internal_dict_value is None:
            return None
    return internal_dict_value

def nested_set(dic, keys, value):
    for key in keys[:-1]:
        dic = dic.setdefault(key, {})
    dic[keys[-1]] = value

def dup_dicts(keys, iter_of_values, init_dict):
    for values in iter_of_values:
        init_dict = deepcopy(init_dict)
        [nested_set(init_dict, key, value) for key, value in zip(keys, values)]
        yield init_dict

if __name__ == '__main__':
    keys = list(map_keys_by_type(d, pd.DataFrame))
    dfs = map(partial(nested_get, input_dict=d), keys)
    dfs_as_dicts = map(partial(pd.DataFrame.to_dict, orient='records'), dfs)
    iter_of_dicts = dup_dicts(keys,zip(*dfs_as_dicts), d)

any improvements?

I'm assuming that the code works as intended.

Firstly, let me tell you that your code looks very pythonic - this is a huge plus.

My comments:

Introduction

You state that

  I have a dict that may be 'infinitely' nested

Keep in mind that since you seem to be doing recursive calls your stack is limited hence is your ability to retrive nested dict.

map_keys_by_type

Argument d is not informative, try using dic (if you want to follow your change type->typ) or dictionary.

Although, this is not extreamly important you might consider changing p to something more informative. When I've read the code first time I almost immediacy forgot what this variable stands for. However, as I said this is not very important because the function is small.

nested_get

This bit

for k in nested_key:
    internal_dict_value = internal_dict_value.get(k, None)
    if internal_dict_value is None:
        return None

in my opinion would be clearer if you did:

if k not in internal_dict_value:
    return None
else:
    internal_dict_value = internal_dict_value[k]

not in shows your intention directly.

Everything else looks okay for me.


Adding elements in two lists as numbers The problem is adding two lists as numbers L1 = [1,9,9] and  L2 = [0,9,9]:

?- sum([1,9,9],[0,9,9], Lo).
Lo = [2,9,8]

But I also wanted to add this:

?- sum([8,1,9],[1,8,2],Lo).
Lo = [1, 0, 0, 1].

I used the backtracking method I've learned:

link([],L,L).
link([Head|Tale],L2,L3):- link(Tale,L2,L), L3=[Head|L].

inve([],[]):-!.
inve([X|Xs],L):- inve(Xs,L2), link(L2,[X],L).

sum(L1,L2,L3):- inve(L1,LI1),inve(L2,LI2), sumID(LI1,LI2,L), inve(L,[Li|LIs]),
                     Li > 9, Li2 is Li-10 , L3 = [1,Li2|LIs] ,!.
sum(L1,L2,L3):- inve(L1,LI1),inve(L2,LI2), sumID(LI1,LI2,L), inve(L,L3),!.

sumID([],[],[]):- !.
sumID([X|[Xs|Xss]],[Y|Ys],[L|Ls] ):- XY is X+Y , XY > 9 , Head is XY - 10,
                                  L = Head, Xs1 is Xs + 1,
                              sumID([Xs1|Xss], Ys, LTail) ,Ls = LTail,!.

sumID([X|Xs],[Y|Ys],[L|Ls]):- L is X+Y, sumID(Xs,Ys,LTail),
                           Ls = LTail.

A friend told me to invest list and add from *Left to Right*, and later invest the final list. How can I improve this solution in order not to be so long? I'd appreciate a better idea to solve this problem, too. I made it in more than 2 hours and in the exam this is supposed to be in 20min.

When describing lists, always consider using DCGs. In this case, try for example:

sum(Xs0, Ys0, Ls) :-
    reverse(Xs0, Xs),
    reverse(Ys0, Ys),
    phrase(sum(Xs, Ys, 0), Ls0),
    reverse(Ls0, Ls).

sum([], [], Carry) -->
    (   { Carry > 0 } -> [Carry]
    ;   []
    ).
sum([X|Xs], [Y|Ys], Carry0) -->
    { N0 is X + Y + Carry0,
      (  N0 > 9 ->  N is N0 - 10, Carry = 1
      ;  N = N0, Carry = 0
      ) },
    [N],
    sum(Xs, Ys, Carry).

Exampe queries and their results:

?- sum([1,9,9], [0,9,9], Ls).
Ls = [2, 9, 8].

?- sum([8,1,9], [1,8,2], Ls).
Ls = [1, 0, 0, 1].


Keeping just the rare characters in a long string I'm working on The Python Challenge. The current code iterates over a 98,766-character string before printing an 8-character string. Current execution time is between 7 and 10 seconds.

How can I optimize my current code and what rookie mistakes am I making that may be adding to the execution time?

import urllib.request
import re

url = 'http://www.pythonchallenge.com/pc/def/ocr.html'

x = urllib.request.urlopen(url).read().decode('utf-8')

text = (re.findall(r'<!--(.*?)-->', x, re.DOTALL))[1]

# source contains two adjacent html comments
# returns second comment

s = ''
for i in text:
    if text.count(i) < 10:
        s += i
print(s)

x is not a good variable name.  I suggest page_source.

Your approach is inefficient because you re-count all of the occurrences of each character, which is O(n2), where n is the length of the string.

It would be better to use a Counter.  It's more efficient because counts all characters using just one pass through the string.  (We call that O(n).)

from collections import Counter

count = Counter(text)
s = ''.join(char for char in text if count[char] < 10)
print(s)

If you wanted to make your own counter with similar performance characteristics, you could do something similar to this:

count = {}
for char in text:
    count[char] = 1 + count.get(char, 0)


Writing and reading of character files I've made the following Java-class just for getting some practice with the creation and reading of files.

package modul;

import static java.lang.System.*;
import java.io.*;

public class Sandbox {
    public static void main (String[] args) {

        // I found out that the project directory is the
        //  working directory by default.
        //  Is there a way to change the working directory in Java?
        //  So that all instructions become relative to that directory.
        String currentDir = System.getProperty("user.dir") + "/src/modul/";
        String fileName = "lorem.txt";

        try {
            PrintWriter writer = new PrintWriter(new BufferedWriter(new FileWriter(currentDir + fileName)));

            // Write a few lines of dummy-text ...
            writer.println("The very first line of text.");
            writer.println("----------------------------");
            writer.println("Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor.");
            writer.println("Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim.");
            writer.println("In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo.");

            // Is it enough just closing the PrintWriter?
            //  What's about the BufferedReader and the FileReader?
            //  Are they closed automatically?
            writer.close();

            // Now open the created file. Read and print it's
            //  content to stdout.
            BufferedReader reader = new BufferedReader(new FileReader(currentDir + fileName));
            String line;
            int lineNumber = 0;

            while ((line = reader.readLine()) != null) {
                out.println(++lineNumber + ": " + line);
            }

            // Is it fair enough just closing the Reader?
            reader.close();
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Although it won't serve a real use-case it's important for me to know if everything is done in a good way. Or does I have to improve the code?

Moreover: Are there ways to accomplish writing-, reading-operations more efficient? Respectively in a simpler way?

A few more questions I've still got about Java IO-operations I've added as comments within the code.

All comments, hints and recommendations of more experienced Java-devs much appreciated.

PrintWriter writer = new PrintWriter(new BufferedWriter(new FileWriter(currentDir + fileName)));
            ...
            BufferedReader reader = new BufferedReader(new FileReader(currentDir + fileName));

Never ever ever open a file for reading or writing characters without explicitly specifying the encoding. You'll think it's all working fine and then someone will run your program on Mac OS X and send you a file encoded in MacRoman because that's the default on their platform.

Yes, I learnt that one the hard way.

Unfortunately the Java standard library tries to make things "easy" by hiding access to the encoding and ends up making it hard for anyone who wants to write portable code (supposedly Java's strong point). You can't use FileWriter, because none of its constructors have an encoding parameter. You have to create a FileOutputStream and wrap it in an OutputStreamWriter. FileReader is similarly useless.


Flask brand API endpoint unit test # unit tests for the /brands endpoint

from flask.ext import restful
import unittest
import os
import json
import sys

# tells sql_models to use the testing database
os.environ['API_TESTING'] = '1'
sys.path.append('/opt/backend_api/')

import api
from models import sql_models
from config import settings

class TestBrandsEndpoint(unittest.TestCase):

    def setUp(self):
        ''' fire up a test instance of the flask app '''
        self.app = api.APP.test_client()
        self.app.testing = True
        self.api = api.API
        self.test_brand_name = 'Momcorp'

    def tearDown(self):
        '''
            tear down app, reverse any changes made
            - remove the brand inserted in test_post()
        '''
        brand_object = sql_models.Brand.get_by_name(self.test_brand_name)
        if brand_object is not None:
            sql_models.Brand.delete_brand(brand_object)
        pass

    def test_post(self):
        '''
            insert a new brand into the database
            - only given field for test is the mandatory brand name but could(should?) be extended for the rest
            - test that the response status code == 201 and the returned brand object has the correct name
        '''
        result = self.app.post( '/' + settings.BASE_API_VERSION + '/brands',
                                headers=[('user-api-key', '<redacted>')],
                                data={'name':self.test_brand_name})
        # print(json.loads(result.data))
        self.assertEqual(result.status_code, 201)
        self.assertEqual(json.loads(result.data)['brand']['name'], self.test_brand_name)

    def test_get(self):
        '''
            get all brands and all currently running offers
            - sets the brand's 'offers_running' status
            - returns serialized brands like so: {'brands':[serialized_brands]}
            - test status code + name, id fields are not null
        '''
        result = self.app.get('/' + settings.BASE_API_VERSION + '/brands', headers=[('user-api-key','<redacted>')])
        self.assertEqual(result.status_code, 200)
        for brand in json.loads(result.data)['brands']:
            self.assertIsNotNone(brand['name'])
            self.assertIsNotNone(brand['id'])

if __name__ == '__main__':
    unittest.main()

Looking for advice about the 'right way' to do this. Not sure if I should have a separate file and class for each endpoint or if all endpoints+request types should just be their own methods in a larger TestAllEndpoints(unittest.TestCase) class. Any advice or opinions are appreciated.

Move the os.environ and sys.path calls into a new setUpClass method instead of have them at the global level?
pass at the end of setUp should be removed, similarly the commented out print.
I'd put the constants for the URL and the headers into fields or methods (self.url() / self.url) to reduce duplication.
The docstrings are a bit much.  Ideally the code would be very self explanatory so that it almost spells out what you currently have (duplicated) in the docstring.
I'd put things that belong together in the same file; so same endpoint and requests definitely in the same file and probably in the same test class as well.  I'd go for practicality first - if you need to have convoluted setup because you want to handle many different tests, then split it into multiple classes such that the tests are again easy to read.


Spotify puzzle problem in python I'm a complete beginner in Python and programming in general. I made a program for Spotify's Best Before puzzle. It was accepted. I have looked a litle around on internet and looked at other solutions to the problem, and everyone I have seen have importet several modules, inclusive the Calendar module. I understand this is probably a good solution, but I wanted to make everything myself as a practice.

I would really appreciate all tips and hint, but mainly without haveing to import code. It's primarily the printer(a) and det dataMaker() that needs modification.

normYear = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
leapYear = [31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
answerList = []

u''' Check if any of the integers are years '''

def yearCheck():
    for x in xrange(0, 3):
        a = dataList[x]
        if len(a) > 2:
            if not len(a) == 4 and int(a) in xrange(2000,3000):
                if int(a) in xrange(100,1000):
                    dataList[x] = int(a) + 2000
                else:
                    print data + u" is illegal"

u''' Make integers and sort '''

def integer():
    for x in xrange(0, 3):
        dataList[x] = int(dataList[x])
    dataList.sort()

u''' Check for possible leap years '''

def leapYears():
    global leapList
    leapList = []
    for x in xrange(0, 3):
        if dataList[x] % 4 == 0:
            if dataList[x] % 100 == 0:
                if dataList[x] % 400 == 0:
                   leapList.append(x)
            else:
                leapList.append(x)

u''' Changes year type '''

def defYear(a):
    global xYear
    if a in leapList:
        xYear = leapYear
    else:
        xYear = normYear

u''' Printer '''

def printer(a):
    if dataList[a] < 2000:
        dataList[a] += 2000
    year = dataList[a]
    del dataList[a]
    if not dataList[0] == 0:
        month = dataList.pop(0)
        day = dataList.pop(0)
        answerList.append(unicode(year))
        answerList.append(unicode(u'%02d' % month))
        answerList.append(unicode(u'%02d' % day))
        print u'-'.join(answerList)
    else:
        print data + u" is illegal"

u''' Looks for legal dates, first [Y<M<D] then [M<Y<D] then [M,D,Y] '''

def dateMaker():
    for x in xrange(0,4):
        defYear(x)
        if x == 0:
            if dataList[1] <= 12 and dataList[2] <= xYear[dataList[1]-1]:
                printer(x)
                break
        elif x == 1:
            if dataList[0] <= 12 and dataList[2] <= xYear[dataList[0]-1]:
                printer(x)
                break
        elif x == 2:
            if dataList[0] <= 12 and dataList[1] <= xYear[dataList[0]-1]:
                printer(x)
                break
        else:
            print data + u" is illegal"

u''' Program '''

data = raw_input()
dataList = data.split(u"/")
yearCheck()
integer()
leapYears()
dateMaker()

You have unicode strings as comments. Just use comments.
You rely on manipulating global state, not a good idea. Instead,
have function return things. You don't use any return statements
anywhere in your code.
You use not a == b instead of a != b in several places
You use a in xrange(b,c) where b <= a < c would be better
All of your for loops use xrange, usually you want to iterate over the container directly
Your yearCheck function doesn't do anything. The if condition is impossible to satisfy. It looks like you discovered this, because you've implemented the logic down in the printer function.
You delay converting the data components to integer, this only serves to complexicate your code.
You generate a list of leap years, but you only ever check the years 0, 1, 2 and 3.
You keep the data inside the list the whole time. Code is easier to follow if you don't do that. Pull the data out of lists into local variables rather then indexing into the list.

Here is a reworking of your approach:

NORMAL_MONTHS = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
LEAP_MONTHS = [31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]

def get_month_lengths(year):
    """
    Returns appropriate months for a particular year
    """
    if year % 4 == 0:
        if year % 100 == 0:
            if year % 400 == 0:
                return LEAP_MONTHS
        else:
            return LEAP_MONTHS
    return NORMAL_MONTHS

def print_date(year, month, day):
    """
    Print the given date
    """
    if year < 2000:
        year += 2000
    print u'%04d-%02d-%02d' % (year, month, day)

# Looks for legal dates, first [Y<M<D] then [M<Y<D] then [M,D,Y]

def dateMaker(data):
    """
    Try different ways of interpretating the date
    """
    a, b, c = data

    for year, month, day in [(c,b,a), (c,a,a), (b,a,c)]:
        month_lengths = get_month_lengths(year)
        if month <= 12 and day <= month_lengths[month - 1]:
            print_date(year, month, day)
            return

    print unicode(data) + u" is illegal"

data = sorted(map(int, raw_input().split(u"/")))
dateMaker(data)


String manipulation complexity I've been reading Cracking the Coding Interview and at the very beginning I got to the strange statement that the next complexity will be \$O(n^2)\$.

public String makeSentence(String[] words){
 String sentence = "";
 for (String w:words){
     sentence+=w;
 }
 return sentence;
}

I can't get it. Why is it \$O(n^2)\$? Here I found another article on this topic.

For every call to String.concat(), a new string instance is created.  To create that instance, the contents of the previous string needs to be copied to the new one plus the contents of the concatenated string.  This is done for every string in the collection.  So if you look at in in terms of how many strings are concatenated (and not the characters copied), you're copying the contents of the n strings by the time you reach the nth iteration and all that matters is the worst case (the last iteration). Therefore it has \$O(n^2)\$ performance.


select_best: yet another partitioning algorithm The title is somewhat a misnomer; my apologies. The goal of the procedure is to partition a range of first, last by mid for the postcondition

max(first, mid) <= min(mid, last)

The actual code (best_n.h):

#include <algorithm>
#include <iterator>

#if !defined(BidirectionalIterator)
#define BidirectionalIterator typename
#endif

template <BidirectionalIterator I>
void select_best(I first, I mid, I last) {
    using T = typename I::value_type;
    while (first != mid) {
        T pivot = *first;
        I pp = std::partition(first, last, [pivot](T elt) { return elt < pivot; });
        if (pp == first) std::advance(pp, 1);
        if (std::distance(first, pp) > std::distance(first, mid)) last = pp;
        else                                                      first = pp;
    }
}

Works fine if I didn't mistype anything.

Questions:

Seeking overall improvements.
What is a right name?
Currently void. Is there anything useful to return?
I don't like passing lambda to std::partition. Any suggestions to avoid it?
A performance is weird. Regardless of a pivot selection strategy, an execution time stays pretty much constant as a function of mid. Obviously outperforms std::partial_sort for somewhat sizable initial ranges, but loses badly for short ones. Any suggestions?

PS: I have benchmarks if anybody's interested.

PPS: I am well aware of the beautiful best 2 Stepanov's approach. Challenging everybody to generalize it.

A couple of points on the style rather than the algorithm itself.

First, I find the #ifdef for the typename peculiar, it detracts from clarity, and is a pointless use of macros, it may be fine for a small snippet but in a large project or a library header such a thing can wreak havoc. Consider simply,

template <class BidirectionalIterator>

That said, note that C++11 relaxes std::partition to a ForwardIterator, so I would simply say,

template <class ForwardIterator>
void select_best(ForwardIterator first, ForwardIterator mid, ForwardIterator last);

The type T is also a bad choice to denote a value_type, it has too much baggage as a template parameter. Simply say,

using value_type = typename I::value_type;

Then consider avoiding the inadvertent copy of the value by changing the code to take a const reference of the pivot,

const T& pivot = *first;

The lambdas are arguably the preferred way to write the predicate (let's not venture into std::bind and std::min territory), but again I would capture the value by a reference to avoid a copy, and be less verbose,

[&](const T& elt) { return elt < pivot; }

Consider either using brackets with if-else clauses or at least put the conditional statement on a separate line.

if ( ... )
    statement;
else

note that using std::distance implies that complexity of your function can blow up to O(n) for that use only (where n is the pp, somewhere around N/2), and if you want O(1) for the distance calculation you have to make sure your iterators are random access.

and last, please get rid of the ghastly space in else first = pp;.

For your question on the return value, would it be useful to return pp itself?


Are nested foreach loops to compare two multidimensional arrays efficient in PHP? I've got the following PHP code, which is taking a search term, and checking if any of the search terms could be or could have a nickname.

$search = $_GET['search-term'];
$indiviudal_terms = explode(" ",$search);
foreach($indiviudal_terms as $term) {
    foreach ($nicknames as $nameset) {//$nicknames is a multidimensional array that contains other arrays featuring sets of names, e.g. array("Abraham","Abe","Abie","Bram")
        foreach ($nameset as $variant_name) {
            if (strtolower($term) == strtolower($variant_name)) { //Checks to see if any of the search terms match any of the variant names in any of the sets of names.
                echo $nameset; //Just prints the set of names for the moment as I'm still writing to code.
            }
        }
    }
}

This works fine, but I worry that all these nested foreach loops may be inefficient. Is there a better way of achieving the same thing?

You call strtolower($term) during each iteration instead of just once. You can preprocess it:

$individual_dumps = array();
foreach (explode(" ", $search) as $term) {
    $individual_dumps[] = strtolower($term);
}

You can also use array_map for this, which may be a little slower, but with a shorter code:

$individual_dumps = array_map('strtolower', explode(" ", $search));

It may be better to change the order of the loops - make the loop going through all the search terms the innermost. In this way you can also call strtolower($variant_name) just once, and not count($individual_dumps) times. (You can also store them in lower case, avoiding this step completely.)

foreach ($nicknames as $nameset) {
    foreach(array_map('strtolower', $nameset) as $variant_name) {
        foreach($individual_dumps as $term) {
            if ($term == $variant_name) {
                echo $nameset;
            }
        }
    }
}

When you have all the elements in lower case, you can use array_intersect, which should cut complexity for each $nameset from O(m * n) to O(n * log n) of the larger array. (You may be even able to improve it to O(n) using this trick). However different, older source mentions other complexity, so YMMV.

foreach ($nicknames as $nameset) {
    if (array_intersect(array_map('strtolower', $nameset), $individual_dumps)) { //true for non-empty array -> some elements are the same
        echo $nameset;
    }
}

Now, previous optimizations only optimized the foreach loops. There is another possibility - you can use something like array_flip and then array_key_exists for each search term, which may be very fast (because array_key_exists is close to O(1)). I say may be because it depends on the cost of preprocessing, which may not be actually faster than searching using the foreach loop.

Therefore, if you search for multiple independent queries (to justify the preprocessing) or especially if you can store $nicknames preprocessed, then this should be a clear winner with complexity very close to O(number of terms). This has a hidden assumption of small number of search terms and large number of nicknames.

Preprocessing:

$nicknames_preprocessed_keys = array();
$nicknames_preprocessed_namesets = array();
foreach ($nicknames as $nameset) {
    $nameset_key = array_push($nicknames_preprocessed_namesets, $nameset) - 1;
    foreach(array_map('strtolower', $nameset) as $variant_name) {
        $nicknames_preprocessed_keys[$variant_name] = $nameset_key;
    }
}

(Notice that two arrays are used: that is because you work with the whole $nameset as a result. Storing this as value for each key would result in large array.)

Search:

$individual_dumps = array_map('strtolower', explode(" ", $search));
foreach ($individual_dumps as $term) {
    if (array_key_exists($nicknames_preprocessed_keys, $term)) {
        echo $nicknames_preprocessed_namesets[$nicknames_preprocessed_keys[$term]];
    }
}