Python data massager Below is a pretty simple Python script to ingest some data, massage it as necessary, append a column, sort it, and then write it back to another file.  Still learning all the Python best practices and APIs, so as always, any tips and tricks are appreciated!  Feel free to be brutal with me if it's horrible.  It's my first time trying any kind of object-orientation with the language as well, so I have no idea if I'm massacring that or not.

from collections import OrderedDict
import sys

class SkuSequence:
    def __init__(self, iln, ili, sn, psn, ssd, ssc, idc):
        self.inv_loc_nm = iln.strip()
        self.inv_loc_id = ili
        self.sku_nbr = sn
        self.prt_seq_nbr = psn
        self.sign_sz_desc = ssd
        self.sign_sz_cd = ssc
        self.inv_disp_cd = idc.strip()
        self.mod_seq_nbr = "null"

    def get_str_attrs(self):
        attrs = []
        attrs.append(str(self.inv_loc_nm))
        attrs.append(str(self.inv_loc_id))
        attrs.append(str(self.sku_nbr))
        attrs.append(str(self.prt_seq_nbr))
        attrs.append(str(self.sign_sz_desc))
        attrs.append(str(self.sign_sz_cd))
        attrs.append(str(self.inv_disp_cd))
        attrs.append(str(self.mod_seq_nbr))
        return attrs

def sort_sequence(sequence):
    dict = {}
    for sku in sequence:
        if sku.inv_loc_id not in dict:
            dict[sku.inv_loc_id] = {}
        dict[sku.inv_loc_id][sku.prt_seq_nbr] = sku
    for bay in dict:
        dict[bay] = OrderedDict(sorted(dict[bay].items()))
    return OrderedDict(sorted(dict.items()))

def convert_to_csv(header, data):
    rows = []
    if header is not None:
        rows.append(",".join(header.get_str_attrs()))
    for bay, sequence_data in data.items():
        seq_nbr = 0
        for sku in sequence_data.values():
            if int(sku.inv_disp_cd) == 5:
                sku.mod_seq_nbr = "null"
            else:
                seq_nbr += 1
                sku.mod_seq_nbr = seq_nbr
            rows.append(",".join(sku.get_str_attrs()))
    return rows

def consume_data(filename):
    lines = []
    skus = []
    with open(filename) as csv:
        lines = csv.readlines()
    for line in lines:
        attrs = line.split(",")
        skus.append(SkuSequence(attrs[0], attrs[1], attrs[2], attrs[3], attrs[4], attrs[5], attrs[6]))
    return skus

def write_file(lines):
    filename = sys.argv[1]
    file_parts = filename.split(".")
    file_parts[0] += "_resequence"
    filename = ".".join(file_parts)
    with open(filename, "a") as file:
        for line in lines:
            file.write(line + "\n")

def main():
    if len(sys.argv) < 2:
        print("You must provide an input filename as an argument.")
        sys.exit(128)
    data = consume_data(sys.argv[1])
    header = data.pop(0)
    header.mod_seq_nbr = "mod_seq_nbr"
    sorted = sort_sequence(data)
    write_file(convert_to_csv(header, sorted))

if __name__ == "__main__":
    main()

To summarize, the things I expect could be improved include documentation (docstrings and/or comments), variable names (especially the ones that don't convey much, or convey things that don't match behavior), the coupling of functions (write_file may misbehave if sys.argv[1] is wrong, or if its input isn't sorted), and a couple of places that using some specialized collections could clean things up.

The first things I dwell on as a reader are the names. What in the world is iln vs ili, etc. Is this a foreign language? An acronym in heavy usage in your field? If neither, it's generally considered more pythonic to use longer with more obvious meanings. (Note that even the longer attribute names these get on SkuSequence instances are a little abbreviated for my taste.) This can be mitigated with comments or docstrings, but I prefer to have the code stand on its own as much as possible.

The use of OrderedDict in sort_sequence seems fishy to me. Typically OrderedDict is used to remember the insertion order, but here you are sorting the items to create essentially a SortOrderedDict. What utility does this provide that a regular dict does not? Perhaps this should be documented in sort_sequence's docstring, or perhaps the sorting should be handled in convert_to_csv. By the way, the dance to ensure that dict[sku.inv_loc_id] exists would be better written by using a collections.defaultdict(dict) and just pretending it's already there. It also shouldn't be called dict as that shadows the builtin of the same name.

The interface or name of consume_data seems a little off. Calling it consume_data suggests that it could take the output of csv.readlines() instead of taking the name of the file that it then reads. Furthermore, if the file is truly csv, you're better off using a real csv parser such as that in the csv module, especially if any values could ever contain a comma. Finally, unless you are discarding some of the values, the creation of SkuSequence could take SkuSequence(*attrs) or even SkuSequence(line.split(",")).

Alternately SkuSequence could be rewritten to accept a list or tuple, or perhaps even a collections.namedtuple. Using a namedtuple might clean up some code I don't particularly like - namely the very repetitive parts of __init__ and get_str_attrs. My gut tells me it is worth exploring using namedtuple more closely, but I haven't proven it yet.

Referring directly to sys.argv[1] in write_file also seems a little unusual to me; it tightly couples it to the script rather than letting it be used in library, or even with a better command-line parser. Note that by contrast consume_data accepts the name of the file, but write_file magically divines it. It doesn't bother me as much that write_file modifies the name by adding _resequence (although it should probably use helpers from os.path for dicing and splicing the filename); make sure the parameter (that it doesn't yet take reflects) this in its name, say source_file or base_name. Coupling would probably be even lower (better) if main decided the name (even if by calling a new helper function); as this would allow the script to function "in place" if ever directed to.


Merge Two Sorted Linked Lists I solved this problem in C++. I was having trouble though understanding what return headA; and return headB; returns after going back down the recursion tree. A code review is also appreciated!

  You’re given the pointer to the head nodes of two sorted linked lists.
  The data in both lists will be sorted in ascending order. Change the
  next pointers to obtain a single, merged linked list which also has
  data in ascending order. Either head pointer given may be null meaning
  that the corresponding list is empty.

/*
  Merge two sorted lists A and B as one linked list
  Node is defined as
  struct Node
  {
     int data;
     struct Node *next;
  }
*/
#include <iomanip>

Node* MergeLists(Node *headA, Node* headB)
{
    // check if either linked list is empty, then return the other one. when doing recursive, a new headA and headB will be passed in, so it will add the entire list correctly
  if (headA == NULL) {
      return headB;
  } else if (headB == NULL) {
      return headA;
  } else { // if neither empty go into this block
      if (headA->data <= headB->data) {
          headA->next = MergeLists(headA->next, headB);
          // returns the entire linked list after the recursion parses it in the correct order
          // not too sure what is returned going down the recursion tree
          // headA = 1 -> 3 -> 5 -> 6 -> NULL
          // headB = 2 -> 4 -> 7 -> NULL
          // would it return 6, 5, 3, 1, 1->2->3->4->5->6->7 after recursion tree?
          return headA;
      } else {
          headB->next = MergeLists(headA, headB->next);
          // return 7, 4, 2 after recursion tree?
          return headB;
      }
  }
}

Your solution is fine, subject to the caveat that the use of recursion imposes a limit on the length of your lists, due to the possibility of stack overflow.

Your comments betray a sense of insecurity about how it works, though.  Just consider what happens locally, and trust that recursion will take care of the rest.

I also suggest un-nesting the else block.

/**
 * Recursively merge two sorted lists by manipulating node pointers.
 * Returns the head of the merged list.
 */
Node* MergeLists(Node *headA, Node* headB)
{
    if (headA == NULL) {
        return headB;
    } else if (headB == NULL) {
        return headA;
    } else if (headA->data <= headB->data) {
        headA->next = MergeLists(headA->next, headB);
        return headA;
    } else {
        headB->next = MergeLists(headA, headB->next);
        return headB;
    }
}


MVC Improvement - The View Module - 0 * This simply pulls data from mysql and creates the html to be sent to the page.

class view_html extends database
  {
  function __construct($type)
    {
    parent::__construct();
    switch ($type)
      {
      case "bookmraks":
        $this->bookmarks();
        break;
      case "tweets":
        $this->tweets();
        break;
      default:
        echo "Invalid View Type";
        break;
      }
    }
  private function bookmarks()
    {
    $email = $_SESSION['email'];
    $query_return = database::query("SELECT * FROM bo  WHERE email='$email' ORDER BY name ASC");
    while ($ass_array = mysqli_fetch_assoc($query_return))
      {
      $fav=$this->fav($ass_array['url']);
      echo "$ass_array[name]";
      }
    }
  private function tweets()
    {
    $query_return = database::query("SELECT * FROM tw ORDER BY time DESC LIMIT 7");
    $time = time();
    while ($a = mysqli_fetch_assoc($query_return))
      {
      echo  "
$a[fname] posted

$a[message]

";
      }
    }
  private function fav($url)
    {
    $pieces = parse_url($url);
    $domain = isset($pieces['host']) ? $pieces['host'] : '';
    if(preg_match('/(?P<domain>[a-z0-9][a-z0-9\-]{1,63}\.[a-z\.]{2,6})$/i', $domain, $regs))
      {
      return $pieces['scheme'] . '://www.' . $regs['domain'] . '/favicon.ico';
      }
      return false;
    }
  }

Replace all usages of direct Values.

Try assiging values first to a named variable and then concating it, so its sustainable in the future like:

return $pieces['scheme'] . '://www.' . $regs['domain'] . '/favicon.ico';

could be:

$protocol = $pieces['scheme'];
$domainName = $regs['domain'];
$faviconName = . '/favicon.ico';
$favIconLink = $protocol. '://www.' .  $domainName . $faviconName
return  $favIconLink;

This would make it reusable and readable for other developers, and it reduces comments ;-) Currently you have many of them

$fav=$this->fav($ass_array['url']);

I love regex, but once written nobody understands it:

if(preg_match('/(?P<domain>[a-z0-9][a-z0-9\-]{1,63}\.[a-z\.]{2,6})$/i', $domain, $regs))

i would add a comment which explaines detailed what this regex is suppose to do. So if there is an Problem with the Regex everybody can check if the regex is doing it's job or if there is a problem in the expression.

Throw an exception in your constructor, so the errormessage can be cought. Now the error Message is displayed in the frontcode.

function __construct($type)
{
parent::__construct();
switch ($type)
  {
  case "bookmraks":
    $this->bookmarks();
    break;
  case "tweets":
    $this->tweets();
    break;
  default:
    throw new Exception('Invalid View Type');
  }
}

Last but not least:

try using "'" for strings, so its more readable and you make less errors by escaping not escaping closing, whatever

 '
$a[fname] posted

$a[message]

'

instead of :

 "
$a[fname] posted

$a[message]

"


HTTP request for a single URL or list of URLs based on arguments provided I have the following code that includes a simple class AioRequest. I would like to be able to call an instance of this class directly with a single URL or a list of URLs and return the results.

request = AioRequest()
single_response = request('get', 'http://httpbin.org/get')

or

url_list = ['http://httpbin.org/get', 'http://google/some_json']
list_of_responses = request('get', url_list)

I'm wondering what the best practice is for two strategies I am using:

Determining if the argument is a list of URLs or a string with a single URL. Should it be two separate arguments like __call__ or should I type check like __call__2?
I would like to use the __call__ method as the main API for the AioRequest instance, but is returning two different types based on the arguments given a good idea? Is is safe to rely on the user to determine the return type (example: give a single string get a single response, give a list of strings, get a list of responses)?

import asyncio
from aiohttp import ClientSession
from collections import namedtuple

Response = namedtuple(
    "Response",
    ["url", "status_code", "json"]
)

class AioRequest:

    def __init__(self, session=None):
        self._loop = asyncio.get_event_loop()
        self._session = session or ClientSession(loop=self._loop)

    def __call__(self, method, url=None, url_list=None, **kwargs):
        urls = url_list or []
        urls.append(url)
        requests = [self._request(method, url, **kwargs)
                    for url in urls
                    if url is not None]
        return self._run(*requests)

    def __call__2(self, method, url_or_list_of, **kwargs):
        if type(url_or_list_of) is str:
            return self._run(method, url_or_list_of, **kwargs)
        else:
            requests = [self._request(method, url, **kwargs)
                        for url in url_or_list_of]
            return self._run(*requests)

    async def _request(self, method, url, **kwargs):
        response = await self._session.request(method, url, **kwargs)
        return Response(url, response.status, await response.json())

    def _run(self, *tasks):
        task_group = asyncio.gather(*tasks)
        results = self._loop.run_until_complete(task_group)  # results = list of responses
        if len(results) == 1:
            return results[0]
        return results

    def __del__(self):
        self._session.close()

__call__ really confuses me. Type checking is the way to go for readability. On top of that, if you need to accept new types later, all you need to do is add an if statement.

In my opinion, if you absolutely want the brevity of __call__, accepting multiple types of arguments is fine, and so is returning multiple types (as is the case with many builtin and standard library functions). It is important that you mention this in the documentation, though.

Some other questionable things:

Instead of comparing type directly, you can use isinstance:

if isinstance(url_or_list_of, str):
    ...

url_or_list_of is a somewhat unfriendly name for a public API. How about url_or_list_of_urls?
It's good that you provide a 'clean exit' method. __del__ isn't the right method to override, however, since the implementation is CPython-specific, and it may never actually be called. Instead, try defining __enter__ and __exit__:

class AioRequest:
    ...
    def __enter__(self):
        return self
        # This should be all in most cases,
        # unless you want to do something with `self` before returning
    ...
    def __exit__(self, exc_type, exc_value, exc_traceback):
        self._session.close()

You can then use with:

  with AioRequest() as request:
      # __enter__ called
      response = request( ... )
  # __exit__ called


Hackerrank regex solution I solved the following problem on Hackerrank.

The challenge is to:

  Parse HTML input and display the tags and attributes in lexicographically ascending order.

This is the input format:

2 //number of test cases/ lines of html

Example Link

More Link Examples...

This is the output format:

tag-1:attribute-1,attribute-2,attribute-3....
tag-2:attribute-1,attribute-2,attribute-3....
tag-3:attribute-1,attribute-2,attribute-3....
...
tag-n:attribute-1,attribute-2,attribute-3....

Example solution:

a:href
div:class
p:

I am trying to get better at writing meaningful code for my solutions. Any comments/suggestions?

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;

public class Solution {

private static StringBuilder htmlBuilder = new StringBuilder("");
private static Pattern rgxForTagAtEnd = Pattern.compile("(<[a-z0-9]+)$");
private static Pattern rgxForTagAtStart = Pattern.compile("^[a-z0-9]+");
private static Pattern rgxToExtractTags = Pattern.compile("(<[a-z]+[^>]*>)");
private static Pattern rgxToExtractTagName = Pattern.compile("(?:<)[a-z0-9]+");
private static Pattern rgxToExtractAttribute = Pattern.compile("[a-z]+(?:=[\"\'])");

public static boolean checkForBeginningOfTag(){
    Matcher beginningOfTag = rgxForTagAtEnd.matcher(htmlBuilder.toString());
    Boolean B = beginningOfTag.find();
    return B;
}

public static boolean doesTagEndInTheSameLine(String remainingHtml){
    Matcher endingOfTag = rgxForTagAtStart.matcher(remainingHtml);
    Boolean B = endingOfTag.find();
    return B;
}

public static void printTagsAndAttributes(){
    Matcher M = rgxToExtractTags.matcher(htmlBuilder.toString());
    Map<String,TreeSet<String>> tagsAndAttributes = new TreeMap<String,TreeSet<String>>();
    while(M.find()){
        String tag = M.group();
        Matcher tagName = rgxToExtractTagName.matcher(tag);
        Matcher attrName = rgxToExtractAttribute.matcher(tag);
        TreeSet<String> attributes = new TreeSet<String>();
        while(attrName.find()){
            attributes.add(attrName.group(0));
        }
        while(tagName.find()){
            String nameOfTag = tagName.group(0);
            if(tagsAndAttributes.containsKey(nameOfTag)){
                TreeSet<String> tmpSet = new TreeSet<String>(tagsAndAttributes.get(nameOfTag));
                tmpSet.addAll(attributes);
                tagsAndAttributes.put(nameOfTag,tmpSet);
            }else{
                tagsAndAttributes.put(tagName.group(),attributes);
            }
        }
    }

    for(String tag: tagsAndAttributes.keySet()){
        String result = "";
        result = result + tag.substring(1,tag.length()) + ":";
        for(String attribute: tagsAndAttributes.get(tag)){
            result+=attribute.substring(0,attribute.length()-2)+",";
        }
        if(result.charAt(result.length()-1) == ','){
            System.out.print(result.substring(0,result.length()-1));
        }else{
            System.out.print(result);
        }
        System.out.println();
    }
}

public static void main(String[] args) {
    Scanner scanner = new Scanner(System.in);
    int T = scanner.nextInt();
    scanner.nextLine();
    for(int i=0;i<T;i++){
        if(scanner.hasNextLine()){
            String nextLine = scanner.nextLine();
            if(checkForBeginningOfTag()){
                if(!doesTagEndInTheSameLine(nextLine)){
                    htmlBuilder.append(" ");
                }
            }
            htmlBuilder.append(nextLine);
        }
    }

    printTagsAndAttributes();
}
}

Single-character variable names when used outside of 'traditional' loop constructs (e.g. i, j) or lambda bodies are bad not recommended, U(ppercase) case ones especially more so as they can get easily be confused with class names or generic type declaration (the usage most often seen). Therefore, please give better names to Matcher M and int T.

public static boolean doesTagEndInTheSameLine(String remainingHtml){
    Matcher endingOfTag = rgxForTagAtStart.matcher(remainingHtml);
    Boolean B = endingOfTag.find();
    return B;
}

I think a primitive boolean will do here. The whole method body can be simplified to just:

public static boolean doesTagEndInTheSameLine(String remainingHtml) {
    return rgxForTagAtStart.matcher(remainingHtml).find();
}

One more thing: You can do with more white spaces between operators...


Python rsync to copy the logs from multiple remote servers I have the below code which i'm working to copy(using rsync) the contents From remote host labserver01 and dumps those into the directory /var/log/infoSec/ on the base system from where scripts runs and this works correctly and sends e-mail to the recipients, However i'm also figuring out to include a way to send e-mail even if it fails.

I'm Just wondering if there is better way do this, i'm sure there will be more elegant ways.

Appreciate any idea and review in advance.

#!/usr/bin/python3
from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText
import smtplib
import subprocess
import argparse
import sys
import os

#Dir Structure
dest_dir = "/infralogs/external_dns_logs"
rsync_user = "root"

email_sender = "dnslogger@udalt.com"
email_receiver = "gusain@udalt.com"
msg = ""

parser = argparse.ArgumentParser()
parser.add_argument("-n","--hosts",dest="hosts",help="enter remote host/hosts name, comma seperated",metavar="HOSTS")
parser.add_argument("-s","--src",dest="source",help="source file/directory",metavar="SOURCE")
parser.add_argument("-e","--exclude",dest="exclude",help="Exclude files/Directories, comma seperated list",metavar="EXCLUDE")

if len(sys.argv) < 7:
    print(len(sys.argv))
    parser.print_help()
    parser.exit()

args = parser.parse_args()

def sync(host,dest_dir):
    exclude = ""
    if not os.path.exists(dest_dir):
        os.mkdir(dest_dir)
    if ',' in args.exclude:
        for excl in args.exclude.split(','):
            exclude = exclude + " --exclude " + excl
        cmd = "rsync -e 'ssh -o StrictHostKeyChecking=no' -auPz %s %s@%s:%s %s/"%(exclude,rsync_user,host,args.source,dest_dir)
    else:
        cmd = "rsync -e 'ssh -o StrictHostKeyChecking=no' -auPz --exclude %s %s@%s:%s %s/"%(args.exclude,rsync_user,host,args.source,dest_dir)
    cmd_content = cmd
    p = subprocess.Popen(cmd,shell=True)
    p.wait()
    print("DONE")
    return cmd_content + " Rsync process completed."  # returns the msg to the caller

msglist = []             # a list to store the cmd_contents for the mail body

if ',' in args.hosts:
    for host in args.hosts.split(','):
        dest = dest_dir + "/" + host
        msglist.append(sync(host,dest))
else:
    dest = dest_dir + "/" + args.hosts
    msglist.append(sync(args.hosts,dest))

msg = "\n".join(msglist)                 # combine all cmd_contents, one per line

try:
    Mail = smtplib.SMTP('mailserver.global.udalt.com', 25, 'localhost.udalt.com')
    mail_obj = MIMEMultipart('alternative')
    mail_obj["From"] = email_sender
    mail_obj["To"] = email_receiver
    mail_obj["Cc"] = "gusain@udalt.com"
    mail_obj["Subject"] = "Rsync process completed Successfully."
    mail_obj.attach(MIMEText(msg, 'plain'))
    Mail.sendmail(from_addr=[email_sender], to_addrs=[email_receiver],msg=mail_obj.as_string())
    print("Mail Sent to %s" % (email_sender))
except Exception as error:
    print("Mail Failed - {}".format(error))

Command Execution method:

  $ /usr/bin/dns_rsync.py  -n labserver01 -s /var/log/infoSec/ -e "null"

Review

Constants should be UPPER_SNAKE_CASE
More (and better) functions!

Lot of this code is in the global namespace, which is bad (not maintainable)
Why is sync doing argument parsing?

You should split up your code into individual instructions

Same goes for the logic of hosts splitting, these really should be part of another function (maybe parse_arguments)

Go with new style formatting or fstrings python3.6+
Simplify the split(',') using join()
Don't do manual path appending but use os.path.join()
Top it off with a if __name__ == '__main__'

Code

#!/usr/bin/python3
from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText
import smtplib
import subprocess
import argparse
import os

DEST_DIR = "/infralogs/external_dns_logs"
RSYNC_USER = "root"

EMAIL_SENDER = "dnslogger@udalt.com"
EMAIL_RECIEVER = "gusain@udalt.com"
EMAIL_CC = "gusain@udalt.com"

def parse_arguments():
    parser = argparse.ArgumentParser()
    parser.add_argument("-n","--hosts", dest="hosts", help="enter remote host/hosts name, comma seperated", metavar="HOSTS")
    parser.add_argument("-s","--src", dest="source", help="source file/directory", metavar="SOURCE")
    parser.add_argument("-e","--exclude", dest="exclude", help="Exclude files/Directories, comma seperated list", metavar="EXCLUDE")
    args = parser.parse_args()

    if args.exclude is None or args.source is None or args.hosts is None:
        parser.print_help()
        parser.exit()

    args.exclude = " --exclude " + " --exclude".join(args.exclude.split(','))
    args.hosts = args.hosts.split(',')
    return args.hosts, args.source, args.exclude

def sync(hosts, src, exclude):
    for host in hosts:
        host = host.strip()
        dest = os.path.join(DEST_DIR, host)
        cmd = f"rsync -e 'ssh -o StrictHostKeyChecking=no' -auPz {exclude} {RSYNC_USER}@{host}:{src} {dest}/"
        p = subprocess.Popen(cmd, shell=True)
        p.wait()
        yield f"{cmd} Rsync process completed."

def mail(message):
    try:
        Mail = smtplib.SMTP('mailserver.global.udalt.com', 25, 'localhost.udalt.com')
        mail_obj = MIMEMultipart('alternative')
        mail_obj["From"] = EMAIL_SENDER
        mail_obj["To"] = EMAIL_RECIEVER
        mail_obj["Cc"] = EMAIL_CC
        mail_obj["Subject"] = "Rsync process completed Successfully."
        mail_obj.attach(MIMEText(msg, 'plain'))
        Mail.sendmail(from_addr=[EMAIL_SENDER], to_addrs=[EMAIL_RECIEVER],msg=mail_obj.as_string())
        print(f"Mail Sent to {EMAIL_SENDER}")
        print(message)
    except Exception as error:
        print(f"Mail Failed - {error}")

if __name__ == '__main__':
    if not os.path.exists(DEST_DIR):
        os.mkdir(DEST_DIR)

    for msg in sync(*parse_arguments()):
        mail(msg)


Extension method to run any async method synchronously Sometimes I need to call async code from a synchronous method.  This leads to repetition in code to wait for the task to complete, then ensure that the expected exception is thrown instead of an aggregate exception (assuming there's only 1 exception returned, as is the case unless we're iterating through a list / something like that; in which case we preserve the AggregateException as that's then expected by the handling code).

To avoid repetition, would a helper function called and defined as below be appropriate?  Does something like this already exist in the framework?

Example calling code:

bool MySyncMethod (bool throwException)
{
    var task = MyAsyncMethod(throwException);
    return task.WaitResultUnwrapException<bool>();
    //or even
    //return MyAsyncMethod(throwException).WaitResultUnwrapException<bool>();
}
async Task<bool> MyAsyncMethod (bool throwException)
{
    if (throwException)
        throw new ArgumentException(nameof(throwException));
    return await Task.Run(() => true);
}

Example extension method:

using System;
using System.Runtime.ExceptionServices;
using System.Threading.Tasks;

namespace MyCompany.Threading.Tasks
{
    public static class TaskExtensions
    {
        public static T WaitResultUnwrapException<T>(this Task<T> task)
        {
            if (task == null) throw new ArgumentNullException(nameof(task));
            try
            {
                task.Wait();
                return task.Result;
            }
            catch (AggregateException es)
            {
                if (es.InnerExceptions.Count == 1) {
                    var e = es.InnerExceptions[0];
                    ExceptionDispatchInfo.Capture(e).Throw();
                }
                throw;
            }
        }
        public static void WaitResultUnwrapException(this Task task)
        {
            if (task == null) throw new ArgumentNullException(nameof(task));
            try
            {
                task.Wait();
            }
            catch (AggregateException es)
            {
                if (es.InnerExceptions.Count == 1) {
                    var e = es.InnerExceptions[0];
                    ExceptionDispatchInfo.Capture(e).Throw();
                }
                throw;
            }
        }
    }

Per the comment by @t3chb0t, all I needed was this:

task.GetAwaiter().GetResult()

In context:

bool MySyncMethod (bool throwException)
{
    return MyAsyncMethod(throwException).GetAwaiter().GetResult(); //this is the only line that's changed
}
async Task<bool> MyAsyncMethod (bool throwException)
{
    if (throwException)
        throw new ArgumentException(nameof(throwException));
    return await Task.Run(() => true);
}


Summing all keys in the leaves of a tree My program takes 35 sec to run example 2. How can I make my program faster?

def sum_keys(K, inputs, count=1):
    A, B, M, L1, L2, L3, D, R = list(map(int, inputs))
    x = ((A*K)+B)%M
    y = ((A*K+2*B)%M)

    if K < L1 or count == D:
        my_list.append(K)
    elif L1 <= K < L2:
        sum_keys(x, inputs, count + 1)
    elif L2 <= K < L3:
        sum_keys(y, inputs, count + 1)
    elif L3 <= K:
        sum_keys(x, inputs, count + 1)
        sum_keys(y, inputs, count + 1)

    return sum(my_list)

def read_input(inputstring):
    inputs = inputstring.split()
    A, B, M, L1, L2, L3, D, R = list(map(int, inputs))
    x = ((A*R)+B)%M
    y = ((A*R+2*B)%M)

    if L1 <= R < L2:
        return sum_keys(x, inputs)
    elif L2 <= R < L3:
        return sum_keys(y, inputs)
    elif L3 <= R:
        sum_keys(x, inputs)
        return sum_keys(y, inputs)

my_list = []
if __name__ == '__main__':
    print(read_input(input()))

return sum(my_list)

You don't need to calculate the sum every time.  Do it just when count is 1:

if count == 1:
    return sum(my_list)

You have a recursive function, but your function doesn't care what itself returns.  All it cares about is the side-effect: mylist.append(K).  Just changing that (without any other changes) made the second example take less than a second.  It would be best to return my_list and let whatever is calling the function decide if it wants the sum.

In two places you say:

A, B, M, L1, L2, L3, D, R = list(map(int, inputs))

You don't need to convert to a list just to iterate.  That is what A, B, M, ... = ... does.  Just use A, B, M, ... = map(int, inputs).  That prevents the unnecessary storage of the numbers in memory.

Enough about efficiency.  Let's talk about other things.

Your naming is not ideal.  How does someone looking at your program know what A, B, M, etc. are?  They are just letters.  To tell you the truth, I have no idea what this program is doing.  If you had more descriptive names, it would be easier for future readers (possibly including you) to read the code.  PEP 8, the Python style guide, recommends snake_case for normal variables.

It is better not to use global variables.  I would suggest passing my_list as an argument to sum_keys() instead of relying on the existence of a global variable.


Script that makes an API call to and parses the response to get a URL from where to download another file This is my first script and I am having a hard time writing the shortest possible code.

exportsData=$(curl -X GET \
 -H 'Accept: application/vnd.xxxxx+json; version=3.0' \
 -H 'Authorization: Bearer RCexxxxxxxxxxxxxxxxxxxxxxxxxcVefI5mWy' \
 -H 'Content-Type: application/json' \
https://api.xxxx.com/apps/9xxxxxxxxx15f96fe/exports)
status=$(echo $exportsData | jq '.[0].status')
downloadURL=$(echo $exportsData | jq '.[0].download_url')
 export ENCRYPTED_AES_KEY=$(echo $exportsData | jq '.[0].encrypted_aes_key')
export AES_IV=$(echo $exportsData | jq '.[0].aes_iv')

export ENCRYPTED_TARBALL=encryptedChatDump.tar.gz.enc
 $(curl -X GET -o ENCRYPTED_TARBALL \
 -H 'Accept: application/vnd.xxxxx+json; version=3.0' \
 -H 'Authorization: Bearer RCexxxxxxxxxxxxxxxxxxxxxxxxxcVefI5mWy' \
 -H 'Content-Type: application/json' \
https://storage.googleapis.com/someUrlWhereTheFileIsBeingDownloadedFrom)

export OUTPUT_TAR=finalChatDumpUnencrypted.tar.gz
export PRIVATE_KEY_PATH=~/.ssh/id_rsa

Don't repeat yourself

The two curl commands have significant repeated parts.
You can extract that to a function.

For example:

mycurl() {
    curl -X GET \
         -H 'Accept: application/vnd.xxxxx+json; version=3.0' \
         -H 'Authorization: Bearer RCexxxxxxxxxxxxxxxxxxxxxxxxxcVefI5mWy' \
         -H 'Content-Type: application/json' \
        "$@"
}

And then use it like this:

exportsData=$(mycurl https://api.xxxx.com/apps/9xxxxxxxxx15f96fe/exports)
...
mycurl -o "$ENCRYPTED_TARBALL" https://storage.googleapis.com/someUrlWhereTheFileIsBeingDownloadedFrom

Use here-strings instead of echo ... | ...

Instead of this:

echo $exportsData | jq '.[0].status'

It's better to use here-strings, like this:

jq '.[0].status' <<< "$exportsData"

Small bug

Instead of this:

export ENCRYPTED_TARBALL=encryptedChatDump.tar.gz.enc
 $(curl -X GET -o ENCRYPTED_TARBALL \
...

You probably meant to write this:

export ENCRYPTED_TARBALL=encryptedChatDump.tar.gz.enc
 $(curl -X GET -o "$ENCRYPTED_TARBALL" \
...

Pointless variables?

These variables are set at the end of the script, but what for?

export OUTPUT_TAR=finalChatDumpUnencrypted.tar.gz
export PRIVATE_KEY_PATH=~/.ssh/id_rsa


Random walk terrain generator I was messing around with Javascript in Khanacademy the other day and this "monstrosity" was born. It's a terrain generator that uses a simplified version of the random walk method.

// current tile position
var BlockX = 200;
var BlockY = 200;

// tile size
var SizeX = 2;
var SizeY = 2;

// min and max X positions
var MinX = 0;
var MaxX = 400;

// min and max Y positions
var MinY = 0;
var MaxY = 400;

// possible position changes
var BlockPosChanges = [-1, 1, 0];

// repeats and max repeats
var MaxRepeats = round(random(10*100, 10*1250)); // Do not change! This is at the max safe amount
var Repeats = 0;

// set the background
background(0, 13, 255);

// main program loop
while(Repeats < MaxRepeats) {

    // possible color ranges
    var RedRange = round(random(45, 70));
    var BlueRange = round(random(165, 100));
    var GreenRange = round(random(20, 50));

    // draw a tile
    noStroke();
    fill(RedRange, BlueRange, GreenRange);
    rect(BlockX, BlockY, SizeX, SizeY);

    // change the current tile position
    BlockX += BlockPosChanges[floor(random() * BlockPosChanges.length)];
    BlockY += BlockPosChanges[floor(random() * BlockPosChanges.length)];

    // test if at edge
    if(BlockX <= MinX || BlockX >= MaxX) {
        break;
    }

    // test if at edge
    if(BlockY <= MinY || BlockX >= MaxY) {
        break;
    }

    // increment repeats
    Repeats++;
}

Since this version of Javascript has features that may not be included in other variants, you can test this program here in Khanacademy. What can I improve here?

Style

Variables and functions (except constructors) in JavaScript are normally written in camelCase with a leading lowercase letter. It's not mandatory, but you'll find it easier to work with others if you follow a language's conventions. There are many guides out there, but I mostly follow Google's JavaScript Style Guide.

Some of the variables do not change once set. These should use ALL_CAPS to denote that they are constants and probably assigned at the top.

Another point in the guide is to surround all operators and keywords with a space. Omitting the space between if, while, etc. and the opening parenthesis makes it harder to skim without syntax highlighting.

You can omit comments that merely restate the code in prose. At best they waste the readers time, and at worst they become misleading or incorrect when they aren't changed with the code. Good comments state programmer intent or clarify tricky algorithms. This comment adds no value.

// increment repeats
Repeats++;

Bug

As I noted in the comment, you have a typo bug:

if(BlockY <= MinY || BlockX >= MaxY) {
                     ^^^^^^

This is common when using similarly-named variables, but it's difficult to avoid doing so in rendering code.

Clarity

You can improve the performance of the random choice between -1, 0, and 1--and in my opinion make it more obvious--using a simpler equation without the array:

BlockX += floor(3 * random()) - 1;

You are using a while loop where a for loop would be more obvious. As written, you have to scan the whole body to see that Repeats only gets incremented once at the end. Use this instead:

for (var Repeats = 0; Repeats < MaxRepeats; ++Repeats) { ... }


Float to Byte Array Serialization Over Network I wrote this code for a multiplayer game I am developing to transmit floats over the network. It works, on the systems that I have tested it with. What I am worried about is the little-endian big-endian thing and the possibility of different float representations in general over computers. So here it goes:

private static ByteFloat bytefloat = new ByteFloat();
[StructLayout(LayoutKind.Explicit)] struct ByteFloat
{
    [FieldOffset(0)] public float v;
    [FieldOffset(0)] public byte a;
    [FieldOffset(1)] public byte b;
    [FieldOffset(2)] public byte c;
    [FieldOffset(3)] public byte d;
}
public void setFloat(float v)
{
    bytefloat.v = v;
    setByte(bytefloat.a);
    setByte(bytefloat.b);
    setByte(bytefloat.c);
    setByte(bytefloat.d);
}
public float getFloat()
{
    bytefloat.a = getByte();
    bytefloat.b = getByte();
    bytefloat.c = getByte();
    bytefloat.d = getByte();
    return bytefloat.v;
}

I don't really want advice on variable naming or coding conventions, I just want to know if there are possible flaws with this code when the methods are called on different systems. I want to know if calling setFloat on some float, and sending the resulting bytes to any other computer, then calling getFloat on those bytes will result in the exact same value that I started with.

getByte() and setByte() do exactly what they say they do, this method pertains to a class which is a byte array wrapper, used for reading and writing data. (The backing byte array is sent over the network, and on the receiving side it is wrapped again and read from.)

If this code does not work across all systems, I would like a recommendation (or a solution) on how to write it in a way that is cross-system.

If this changes anything, I am working with the Unity platform and it's networking LLAPI (Raknet).

Endianness matters only if your application (or another one you can communicate with) may run on environments with different CPU architectures. If you write, let's say, a Windows application it will currently run only on little-endian CPUs. Moreover you're designing your own communication protocol then endianness is simply part of your specification. If someone else will write, let's say, a compatible application running on PowerPC (!!!) then she will simply adhere to your protocol...

Also note that floating point were sometimes an exception and they did follow different rules than integers. It's pretty uncommon nowadays and, anyway, it's not something you should care about (see previous paragraph).

My suggestion is then to don't care about endianness and save that few CPU cycles you may need to perform any (unneeded) conversion.

Last point more on-topic with Code Review: you should consider to drop that custom implementation that mimic C union, there already is a fast implementation that performs same task: BitConverter.GetBytes(value) (and its counterpart BitConverter.ToSingle(array, 0)).

Doing this way you may also read a bigger buffer (second parameter of ToSingle() is an offset in array) and this will greatly improve performance (over a byte by byte read or multiple array accesses, each one with its bounds checking).

EDIT: note that if performance in this case are a real (measured) issue and you can have an unsafe assembly then you may pick their implementation dropping arguments checking (in release builds):

unsafe static byte[] GetBytes(float value) {
    var bytes = new byte[4];
    fixed (byte* b = bytes)
        *((int*)b) = *(int*)&value;

    return bytes;
}

If access is synchronized then you may even reuse same buffer:

unsafe static void GetBytes(float value, byte[] bytes) {
    Debug.Assert(bytes != null);
    Debug.Assert(bytes.Length == sizeof(float));

    fixed (byte* b = bytes)
    fixed (float* v = &value)
        *((int*)b) = *(int*)v;
}

Note that if you don't really have any performance problem then I'd keep code easier and verifiable and I'd go with BitConverter. ToSingle() implementation is straightforward.

EDIT: a small performance test (just roughly indicative because it's not your true scenario) averaging 100 executions, repeating each trial Int32.MaxValue times to have a significant execution time, keep this in mind when you compare this values (because you see a big difference only because of this):

BitConverter.ToSingle(): 19 milliseconds
Conversion using union style struct: 16 milliseconds
Conversion using unsafe pointer conversion: 7 milliseconds

Code for unsafe conversion is:

static unsafe float ToSingle(byte[] data, int startIndex) {
    fixed (byte* ptr = &data[startIndex]) {
        return *((float*)(int*)ptr);
    }
}

Vice-versa (same test conditions):

BitConverter.GetBytes(): 28 milliseconds
Conversion using union style struct: 15 milliseconds
Conversion using unsafe pointer conversion: 9 milliseconds


isBalanced function for checking if string with brackets is balanced ([{}]) I wrote simple function for checking if string is balanced (with brackets ([{}])). It takes a string argument and need to return boolean value (true if string is balanced and false if not).

const isBalanced = (string) => {
  const arr = string.split('');
  let open = [];

  const openBrackets = {
    '(': true,
    '[': true,
    '{': true,
  };

  const closedBrackets = {
    ')': '(',
    ']': '[',
    '}': '{',
  };

  for (let i = 0, length = arr.length; i < length; i++) {
    if (openBrackets[arr[i]]) {
      open.push(arr[i]);
    } else if (closedBrackets[arr[i]] && open.pop() !== closedBrackets[arr[i]]) {
      return false;
    }
  }

  return !open.length;
}

console.log(isBalanced('') === true);
console.log(isBalanced('()') === true);
console.log(isBalanced(')()') === false);
console.log(isBalanced(')(') === false);
console.log(isBalanced('()(()()()())') === true);
console.log(isBalanced('[](){}') === true);
console.log(isBalanced('[({})]') === true);
console.log(isBalanced('[(]{)}') === false);
console.log(isBalanced('const sth = () => { animal.sleep();') === false);
console.log(isBalanced('const sth = { bark: animal() }') === true);

Do you think that i could improve it somehow? Can you see any mistakes?

openBrackets can be a Set

openBrackets is only used to test if a character is an opening bracket. So, it can be declared like this:

const openBrackets = new Set(['(', '[', '{']);

and the test will become:

if (openBrackets.has(arr[i])) { ... }

Use for...of to iterate

The for ... of syntax avoids having to work with indices, as in this case you don't need the i variable at all (except to dereference the correct character from arr). Furthermore, you can iterate the string directly, without having to .split('') it first:

for (let c of string) {
  if (openBrackets.has(c)) {
    open.push(c);
  } else if (closedBrackets[c] && open.pop() !== closedBrackets[c]) {
    return false;
  }
}

Rename closedBrackets

Reading the name of this variable I would expect some sort of collection of brackets, not a mapping/correspondence between closing and opening brackets. Therefore, I would give it a different name. Something like bracketCorrespondence might work, although there might be some better names for it out there.

So, here's a version of the code with these suggestions applied:

const isBalanced = (string) => {
  let open = [];

  const openBrackets = new Set(['(', '[', '{']);

  const bracketCorrespondence = {
    ')': '(',
    ']': '[',
    '}': '{',
  };

  for (let c of string) {
    if (openBrackets.has(c)) {
      open.push(c);
    } else if (bracketCorrespondence[c] && open.pop() !== bracketCorrespondence[c]) {
      return false;
    }
  }

  return !open.length;
}

console.log(isBalanced('') === true);
console.log(isBalanced('()') === true);
console.log(isBalanced(')()') === false);
console.log(isBalanced(')(') === false);
console.log(isBalanced('()(()()()())') === true);
console.log(isBalanced('[](){}') === true);
console.log(isBalanced('[({})]') === true);
console.log(isBalanced('[(]{)}') === false);
console.log(isBalanced('const sth = () => { animal.sleep();') === false);
console.log(isBalanced('const sth = { bark: animal() }') === true);


Many CRUD operations for a list of technicians' shifts The following code snippet is causing me a performance problem in my application. The first 5 db queries are not high time consuming, the real problem lays on the foreach loop. I considered to use Dictionary structures instead of List to improve the lookups, but improving the loop will make the real difference.

public void ProcessShifts(IList<NavTechAbsenceDto> navShifts)
{

    var branches = _iagiBranchModelFactory.GetAll().ToList();

    var technicians = _iagiTechnicianModelFactory.GetAll().Where(it => it.IsActive).ToList();

    var activeTechCodes = technicians.Select(t => t.ERPId).ToList();

    var absencesForActiveTech = navShifts.Where(a => activeTechCodes.Contains(a.TechCode)).ToList();

    var absences = _iagiTechnicianModelFactory.GetAllWithAbsences().SelectMany(it => it.Absences);

    IEnumerable<Absence> mwmAbsenceList = absences == null ? null : absences.Where(a => a.Start >= DateTime.Today);

    foreach (NavTechAbsenceDto absence in absencesForActiveTech)
    {
        var mwmAbsence = mwmAbsenceList == null ? null : mwmAbsenceList.FirstOrDefault(it => it.AGIErpId == absence.AGIErpId);

        AGIBranchModel branch = branches.FirstOrDefault(b => b.ERPId == absence.BranchCode);
        AGITechnicianModel tech = technicians.FirstOrDefault(t => t.ERPId == absence.TechCode);

        if (branch == null || tech == null)
        {
            Logger.Trace("NOT AVAILABLE: Shift Branch: {0}, Shift Technician: {1}", absence.BranchCode, absence.TechCode);
            continue;
        }

        if (mwmAbsence == null)
        {
            if (absence.AGILogicallyDeleted == 1)
            {

                Logger.Trace(string.Format("DELETED absence:{0} from Navision, but it does not exist in Scheduler", absence.AGIErpId));
            }
            else
            {
                Logger.Trace(string.Format("ADD absence:{0}", absence.AGIErpId));

                tech.AddAbsence(absence, branch);
            }
        }
        else
        {

            if (absence.AGILogicallyDeleted == 1)
            {
                Logger.Trace(string.Format("DELETE absence:{0}", absence.AGIErpId));
                tech.DeleteAbsence(mwmAbsence);
            }
            else
            {
                Logger.Trace(string.Format("UPDATE absence:{0}", absence.AGIErpId));

                mwmAbsence.Start = absence.Start;
                mwmAbsence.End = absence.End;
                mwmAbsence.AGIBranchId = branch.Id;
                mwmAbsence.TechnicianId = tech.Id;
                mwmAbsence.Technician = tech;
                mwmAbsence.AGIType = absence.AGIType;
                mwmAbsence.AGIMobile = absence.AGIMobile;
                mwmAbsence.AGIErpId = absence.AGIErpId;

                tech.UpdateAbsence(mwmAbsence);
            }
        }
    }
}

For what I see, it should suffice - in order to increase time performance of the cycle - to transform the branches, technicians, and mwmAbsenceList into dictionaries with the respective ids as keys. In this way the following lines:

        var mwmAbsence = mwmAbsenceList == null ? null : mwmAbsenceList.FirstOrDefault(it => it.AGIErpId == absence.AGIErpId);
        AGIBranchModel branch = branches.FirstOrDefault(b => b.ERPId == absence.BranchCode);
        AGITechnicianModel tech = technicians.FirstOrDefault(t => t.ERPId == absence.TechCode);

can be executed in \$O(1)\$ time instead of the current \$O(n)\$. The resulting code after the change should be something in the lines of:

        Absence mwmAbsence = null;
        if(mwmAbsenceList != null)
        {
            mwmAbsenceList.TryGetValue(absence.AGIErpId, out mwmAbsence);
        }
        AGIBranchModel branch;
        branches.TryGetValue(absence.BranchCode, out branch);
        AGITechnicianModel tech;
        technicians.TryGetValue(absence.TechCode, out tech);

Another optimization I would suggest is to try and change the method into a set operation (try to get all items via a LINQ query). Just keep in mind that doing it in this way passes the burden to the database.

As for a review of the code I'd suggest three things:

Reduce the space between the lines of code. Right now is a bit too much.
Reduce indentation.
Reduce width.

The end result - after applying the changes in the first solution and the changes for these 3 points - should be something like the following:

public void ProcessShifts(IList<NavTechAbsenceDto> navShifts)
{
    var branches = _iagiBranchModelFactory.GetAll()
        .ToDictionary(/* insert key and value expressions here */);
    var technicians = _iagiTechnicianModelFactory.GetAll()
        .Where(it => it.IsActive)
        .ToDictionary(/* insert key and value expressions here */);
    var activeTechCodes = technicians.Select(t => t.ERPId).ToList();
    var absencesForActiveTech = navShifts
        .Where(a => activeTechCodes.Contains(a.TechCode)).ToList();
    var absences = _iagiTechnicianModelFactory.GetAllWithAbsences()
        .SelectMany(it => it.Absences);
    Dictionary</*TKey*/, /*TVal*/> mwmAbsenceList = absences == null
        ? null
        : absences.Where(a => a.Start >= DateTime.Today)
                  .ToDictionary(/* Insert key and value expressions here */);

    foreach (NavTechAbsenceDto absence in absencesForActiveTech)
    {
        Absence mwmAbsence = null;
        if(mwmAbsenceList != null)
        {
            mwmAbsenceList.TryGetValue(absence.AGIErpId, out mwmAbsence);
        }

        AGIBranchModel branch;
        branches.TryGetValue(absence.BranchCode, out branch);
        AGITechnicianModel tech;
        technicians.TryGetValue(absence.TechCode, out tech);

        if (branch == null || tech == null)
        {
            Logger.Trace("NOT AVAILABLE: Shift Branch: {0}, Shift Technician: {1}",
                absence.BranchCode,
                absence.TechCode);
            continue;
        }

        if (mwmAbsence == null)
        {
            if (absence.AGILogicallyDeleted == 1)
            {
                // is string.Format() necessary?
                Logger.Trace(string.Format(
                    "DELETED absence:{0} from Navision, but it does not exist in Scheduler",
                    absence.AGIErpId));
            }
            else
            {
                Logger.Trace(string.Format("ADD absence:{0}", absence.AGIErpId));
                tech.AddAbsence(absence, branch);
            }

            continue;
        }

        if (absence.AGILogicallyDeleted == 1)
        {
            Logger.Trace(string.Format("DELETE absence:{0}", absence.AGIErpId));
            tech.DeleteAbsence(mwmAbsence);
        }
        else
        {
            Logger.Trace(string.Format("UPDATE absence:{0}",
                absence.AGIErpId));

            mwmAbsence.Start = absence.Start;
            mwmAbsence.End = absence.End;
            mwmAbsence.AGIBranchId = branch.Id;
            mwmAbsence.TechnicianId = tech.Id;
            mwmAbsence.Technician = tech;
            mwmAbsence.AGIType = absence.AGIType;
            mwmAbsence.AGIMobile = absence.AGIMobile;
            mwmAbsence.AGIErpId = absence.AGIErpId;

            tech.UpdateAbsence(mwmAbsence);
        }
    }
}

Let me know if anything's unclear.


Excel .find Macro to write data to archive I would like to speed up this piece of code. The macro gets a search variable from the worksheet "RAW_DATA_ARCHIVE" and then it should find it in the worksheet "RAW DATA". When it finds accordance then the value of column A from worksheet "RAW DATA" will be written in the last column of "RAW_DATA_ARCHIVE".

 For i = 2 To ws1LRow
    SearchString = ws1.Range("A" & i).Value
    Set aCell = ws2.Columns(5).Find(What:=SearchString, LookIn:=xlValues, _
                LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
                MatchCase:=False, SearchFormat:=False)
    'If match found
    If Not aCell Is Nothing Then
        ws1.Cells(i, 80).Value = ws2.Cells(aCell.Row, 1).Value

    End If
Next

If I understand correctly what you're doing, then VBA is simply not the right tool for this task. Excel has a built-in VLOOKUP function specifically for this purpose - try something like this in column 80 of the RAW_DATA_ARCHIVE sheet:

=IFERROR(VLOOKUP(A2,'RAW DATA'!E:E,1,FALSE),"")

That said, you did post some VBA code, albeit just a loop. Here are some thoughts, mostly on naming style:

ws1 and ws2 would probably be better off as sourceSheet and targetSheet.
ws1LRow would be more meaningful as simply lastRow
i is usually fine as a loop counter, but here it has a clear meaning: targetRow
aCell is really a searchResult

Using such meaningful names instantly makes it easier to read the code, which makes it easier to maintain/modify without breaking anything.

It also eliminates the need for explanatory comments:

'If match found
If Not aCell Is Nothing Then

vs.

If Not searchResult Is Nothing Then

A number of things explain why your code is slow.

You're using procedural code instead of built-in worksheet functions
You're writing to cells in a loop, presumably with Application.ScreenUpdating = True and Application.Calculation = xlCalculationAutomatic, which means many things happen every time you write a value into a cell.


Seperate out validations from a service and use custom exceptions for entities not found I am using spring-boot, spring data, and Java 6. Here is an example from a business layer to save student details.

Srvice interface:

public interface StudentService
{
    public StudentDTO save(Integer stdId, Integer divisionId, StudentDTO studentDTO);
}

Service implementation:

@Service("studentService")
public class StudentServiceImpl implements StudentSerivce
{

     @Autowire
     private StudentDAO studentDAO;

     @Autowire
     private STDDAO stdDAO;

     @Autowire
     private DivisionDAO divisionDAO;

     @Autowire
     private TSBeanMapper beanMapper;

     @Override
     public StudentDTO save(Integer stdId, Integer divisionId, StudentDTO studentDTO)
     {
         STDEntity std = stdDAO.findOne(stdId);
         if (std == null)
         {
              throw new BadRequestException("Invalid standard");
         }

         DivisionEntity division = divisionDAO.finsOne(divisionId);
         if (division == null)
         {
              throw new BadRequestException("Invalid division");
         }
         StudentEntity student = beanMapper.map(studentDTO, StudentEntity.class);
         student.setSTD(stdEntity);
         student.setDivision(divisionEntity);
         return beanMapper(studentDAO.save(studentEntity), StudentDTO.class);

     }
}

For stdId and divisionId, I have called findOne and checked against null.

I would prefer that findOne throws an exception rather that return null for my case. But in this bug report it looks like currently it's not possible.

As for those 3 lines:

Fetch entity
Check null
ifNull then throw an exception

These lines of code are needed in other services for stdId and divsionID.

What is the best practice to separate those validations from serviceImpl and put it in other reusable components?

As I do not know the other entities beside "Student" it becomes very hard to evaluate it.

One general thing: having duplicate code does not mean you have a problem. The problem comes if the duplicate code comes with real duplicate semantic for your business domain.

It may be that you are missing an business relevant abstraction of "Student" and the other entities that have std or division in common. But you should think about that if there is really a proper abstraction. You can test it by making up an artificial requirement that affects the Student and the std or division. If the other entities must change as well to have a consistent model then there is semantic duplication and you should consider an abstraction or composition.

If you have identified an abstraction to be proper then you can abstract the null checks as well. Another way is to encapsulate std and division in one object and provide a validate-method that does the check. Furthermore search the internet for refactoring methods like "introduce parameter object".

If you haven't identified an abstract responsibility then leave your code as it is. Otherwise you may fall into a type II error. (Acting if there is a problem, but there isn't). This is related to the SRP. Robert C. Martin shows this in one of his talks: https://youtu.be/Gt0M_OHKhQE?t=698. As well as you should remove duplicate responsibilities you also should separate different responsibilities.

Some other things:

You said you have a business layer but the separation isn't proper. And here is why: Your "business layer" is responsible to serve a web service by throwing a BadRequestException. You are mixing in technologies that belong to other layers.
You do not distinguish between proposal objects, consistent business objects and communication objects. They are all same (StudentDTO in, StudentDTO out).
The null checks are not business relevant. They are a result of mapping from one representation (String) to another representation (Std, Division) while you do not know if the source made proper assumptions of the presents of the requested business objects. This is related to the web service layer that has to ensure to pass correct non-null values to the business layer. On the other side you have code that composes an entity in the same method. That should be separated at least into another class but after all it is a different layer, maybe the business layer, in my opionion the dao layer.


Python ASCII-Art Text Generator _____    _____   _____   _____              _____    _______
      /\      / ____|  / ____| |_   _| |_   _|     /\     |  __ \  |__   __|
     /  \    | (___   | |        | |     | |      /  \    | |__) |    | |
    / /\ \    \___ \  | |        | |     | |     / /\ \   |  _  /     | |
   / ____ \   ____) | | |____   _| |_   _| |_   / ____ \  | | \ \     | |
  /_/    \_\ |_____/   \_____| |_____| |_____| /_/    \_\ |_|  \_\    |_|

As a part of a console utilities module that I'm building, the central function (after which the module is named) is a function that generates ASCII art representation of given text.

Any comments or suggestions for improvement will be great!

default_conf = open('C://Programs//Python35//Lib//asciiart.txt').read()

...

def asciiart (text, conf=default_conf):
    """Produces an ascii art representation of text using the conf data"""

    if isinstance(conf, str): conf = conf.split('\n')

    height, key, data = int(conf[0]), conf[1], conf[2:]
    figures = dict(map(lambda x: (x, data[key.index(x) * height : key.index(x) * height + height]), key))
    art = '\n'.join(['\n'.join([' '.join([figures[x][i] for x in t.lower() if x in figures]) for i in range(height)]) for t in text.split('\n')])

    return art

Here is a demo:

>>> import asciiart
>>> print(asciiart.asciiart('code-review'))
  _____    ____    _____    ______          _____    ______  __      __  _____   ______  __          __
 / ____|  / __ \  |  __ \  |  ____|        |  __ \  |  ____| \ \    / / |_   _| |  ____| \ \        / /
| |      | |  | | | |  | | | |__     ____  | |__) | | |__     \ \  / /    | |   | |__     \ \  /\  / /
| |      | |  | | | |  | | |  __|   |____| |  _  /  |  __|     \ \/ /     | |   |  __|     \ \/  \/ /
| |____  | |__| | | |__| | | |____         | | \ \  | |____     \  /     _| |_  | |____     \  /\  /
 \_____|  \____/  |_____/  |______|        |_|  \_\ |______|     \/     |_____| |______|     \/  \/

The configuration file format is as follows:

First line: the height of every character in the configuration file (number of lines).
Second line: the key. A string containing the letters in the order they appear in the configuration file.
Next height * len(key) lines: the text representation of the characters.

The full configuration file can be found here (covers most of the printable ASCII characters).

Style

You’ve already been told that by @alexwlchan in your previous question but: "whitespace is cheap" and it improves readability a lot. Compare your code to:

def asciiart(text, conf=default_conf):
    """Produces an ascii art representation of text using the conf data"""
    if isinstance(conf, str):
        conf = conf.split('\n')

    height, key, data = int(conf[0]), conf[1], conf[2:]
    figures = dict(map(
        lambda x: (x, data[key.index(x) * height: (key.index(x) + 1) * height]),
        key))
    art = '\n'.join([
        '\n'.join([
            ' '.join([figures[x][i] for x in t.lower() if x in figures])
            for i in range(height)])
        for t in text.split('\n')])
    return art

Again, you should also consider using generator expressions rather than list-comprehensions to feed into your join calls: turn those square brackets into parenthesis.

Handling font configuration

First of, when reading the file, you should remember to close it:

with open('C://Programs//Python35//Lib//asciiart.txt') as configuration_file:
    default_conf = configuration_file.read()

You may also want to name the resulting string DEFAULT_CONF as it is a constant.

Second, why parse this string at each call of asciiart? You should provide a function that will do the parsing and asciiart will only be responsible of using the parsed configuration:

ASCII_ART_CONFIGURATION = {}

def parse_ascii_art_configuration(filename='C://Programs//Python35//Lib//asciiart.txt'):
    with open(filename) as f:
        conf = f.read().split('\n')
    height, key, data = int(conf[0]), conf[1], conf[2:]
    figures = dict(map(
        lambda x: (x, data[key.index(x) * height: (key.index(x) + 1) * height]),
        key))
    ASCII_ART_CONFIGURATION.update(figures)

def asciiart(text):
    if not ASCII_ART_CONFIGURATION:
        parse_ascii_art_configuration()

    art = '\n'.join(
        '\n'.join(
            ' '.join(ASCII_ART_CONFIGURATION[x][i]
                     for x in t.lower() if x in ASCII_ART_CONFIGURATION)
            for i in range(height))
        for t in text.split('\n'))
    return art

The advantage being:

You give your users direct access to the underlying ASCII_ART_CONFIGURATION that they can populate the way they want if need be;
You give your users an easy way to provide their own configuration filepaths;
You parse the configuration only once.

Obviously, this version wont work since asciiart does not have access to height any more. Time to bring our old friend zip (or itertools.zip_longest) back:

from itertools import zip_longest

def asciiart(text):
    if not ASCII_ART_CONFIGURATION:
        parse_ascii_art_configuration()

    art = '\n'.join(
        '\n'.join(
            ' '.join(row)
            for row in zip_longest(
                (ASCII_ART_CONFIGURATION[x]
                 for x in t.lower() if x in ASCII_ART_CONFIGURATION),
                fillvalue=''))
        for t in text.split('\n'))
    return art

Better iterations

Now that we have a better API, let's focus a bit more on the code:

iteration over a file can be made using either the "reading" API or the iterator one, this can help better handle the separation of each part:

with open(filename) as f:
    height = int(next(f))
    keys = next(f)
    data = [line.rstrip('\n') for line in f]

building of the figure dictionary can be made more efficient using the grouper recipe from itertools:

def grouper(iterable, n):
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3) --> ABC DEF G"
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue='')

def parse_ascii_art_configuration(filename='C://Programs//Python35//Lib//asciiart.txt'):
    with open(filename) as f:
        height = int(next(f))
        keys = next(f)
        data = [line.rstrip('\n') for line in f]

    figures = {
        character: list(group)
        for character, group in zip(keys, grouper(data, height))
    }
    ASCII_ART_CONFIGURATION.update(figures)

now that we use zip_longest to concatenate ascii-art letters, you can simplify handling missing symbols by turning ASCII_ART_CONFIGURATION into a collections.defaultdict:

ASCII_ART_CONFIGURATION = defaultdict(list)

def asciiart(text):
    if not ASCII_ART_CONFIGURATION:
        parse_ascii_art_configuration()

    return '\n'.join(
              '\n'.join(
                  ' '.join(row)
                  for row in zip_longest(
                      (ASCII_ART_CONFIGURATION[x] for x in line_of_text),
                      fillvalue=''))
              for line_of_text in text.lower().splitlines())


Client-side API design I'm working on a client-side API for my ASP.net MVC application and I'm not quite sure I've got the code right. This API allows me to call server-side action methods via Ajax and I've designed it to have a similar call syntax like jQuery uses.

Is the variable HomeActions._ajaxSettings static the way this written right now? I'm concerned that if two or more calls are made simultaneously to this API that they would effectively be using/updating the same static variable and cause intermittent bugs or failure. If this variable is an issue how can it be re-written and still maintain the call pattern shown in the example?

Example usage:

MVC.Home.Actions.RefreshCache("myKey").Start().success(function (result) {
            // refresh the window
            window.location.reload(true);
        }).error(function () {
        console.error("epic fail");
    });

API code:

var MVC;
(function (MVC) {
    var HomeActions = (function () {
        function HomeActions() {}
        HomeActions.RefreshCache = function (key, returnUrl, ajaxSettings) {
            HomeActions._ajaxSettings = ajaxSettings || {};
            HomeActions._ajaxSettings['data'] = JSON.stringify({
                key: key,
                returnUrl: returnUrl
            });
            HomeActions._ajaxSettings['url'] = MVC.Home.ActionNames.RefreshCache;
            if ('type' in HomeActions._ajaxSettings == false) HomeActions._ajaxSettings['type'] = 'POST';
            if ('context' in HomeActions._ajaxSettings == false) HomeActions._ajaxSettings['context'] = this;
            if ('contentType' in HomeActions._ajaxSettings == false) HomeActions._ajaxSettings['contentType'] = 'application/json';
            return this;
        };
        HomeActions.IndexAllRecords = function (ajaxSettings) {
            HomeActions._ajaxSettings = ajaxSettings || {};
            HomeActions._ajaxSettings['data'] = JSON.stringify({
            });
            HomeActions._ajaxSettings['url'] = MVC.Home.ActionNames.IndexAllRecords;
            if ('type' in HomeActions._ajaxSettings == false) HomeActions._ajaxSettings['type'] = 'POST';
            if ('context' in HomeActions._ajaxSettings == false) HomeActions._ajaxSettings['context'] = this;
            if ('contentType' in HomeActions._ajaxSettings == false) HomeActions._ajaxSettings['contentType'] = 'application/json';
            return this;
        };
        HomeActions.Start = function () {
            return $.ajax(HomeActions._ajaxSettings);
        };
        return HomeActions;
    }) ();
    var HomeActionNames = (function () {
        function HomeActionNames() {}
        HomeActionNames.RefreshCache = '/refreshcache';
        HomeActionNames.IndexAllRecords = '/indexallrecords';
        return HomeActionNames;
    }) ();
    var Home = (function () {
        function Home() {}
        Home.Actions = HomeActions;
        Home.ActionNames = HomeActionNames;
        return Home;
    }) ();
    MVC.Home = Home;
}) (MVC || (MVC = {}));

After some testing I discovered that HomeActions._ajaxSettings is indeed static and would cause issues if simultaneous calls were made to the API. After some time away from the code  the obvious solution popped into my head as shown below. I've been too caught up in learning the confusing syntax of javascript that the basics of OOP design fell to the wayside just so I could get a working prototype.

I added a "class" called Action and moved the Start() method to it and in each static method, RefreshCache and IndexAllRecords, I now return a new Action class which maintains its state separately with each call to the API.

var MVC;
(function (MVC) {
    var Action = (function () {
        function Action(ajs) {
            this.ajs = ajs;
        }
        Action.prototype.Start = function () {
            return $.ajax(this.ajs);
        };
        return Action;
    }) ();
    var HomeActions = (function () {
        function HomeActions() {}
        HomeActions.RefreshCache = function (key, returnUrl, ajaxSettings) {
            ajaxSettings = ajaxSettings || {};
            ajaxSettings['data'] = JSON.stringify({
                key: key,
                returnUrl: returnUrl
            });
            ajaxSettings['url'] = MVC.Home.ActionNames.RefreshCache;
            if ('type' in ajaxSettings == false) ajaxSettings['type'] = 'POST';
            if ('context' in ajaxSettings == false) ajaxSettings['context'] = this;
            if ('contentType' in ajaxSettings == false) ajaxSettings['contentType'] = 'application/json';
            return new Action(ajaxSettings);
        };
        HomeActions.IndexAllRecords = function (ajaxSettings) {
            ajaxSettings = ajaxSettings || {};
            ajaxSettings['data'] = JSON.stringify({
            });
            ajaxSettings['url'] = MVC.Home.ActionNames.IndexAllRecords;
            if ('type' in ajaxSettings == false) ajaxSettings['type'] = 'POST';
            if ('context' in ajaxSettings == false) ajaxSettings['context'] = this;
            if ('contentType' in ajaxSettings == false) ajaxSettings['contentType'] = 'application/json';
            return new Action(ajaxSettings);
        };
        return HomeActions;
    }) ();
    var HomeActionNames = (function () {
        function HomeActionNames() {}
        HomeActionNames.RefreshCache = '/refreshcache';
        HomeActionNames.IndexAllRecords = '/indexallrecords';
        return HomeActionNames;
    }) ();
    var Home = (function () {
        function Home() {}
        Home.Actions = HomeActions;
        Home.ActionNames = HomeActionNames;
        return Home;
    }) ();
    MVC.Home = Home;
}) (MVC || (MVC = {}));


Updating data on a blog I want to make post editing on my blog. I get data from a form and want to put it into a database. I'm not sure if using setters is the best way, so maybe you can show me better solution?

$form->handleRequest($request);

if ($form->isSubmitted() && $form->isValid()) {
  $formData = $form->getData();

  $post->setTitle($formData->getTitle());
  $post->setText($formData->getText());
  $post->setAuthor($formData->getAuthor());
  $em->flush();

  $this->addFlash(
    'notice',
    'Post updated!'
  );

  return $this->redirectToRoute('admin');
}

You need to add persist() like so:

...
$post->setAuthor($formData->getAuthor());
$em->persist($post);
$em->flush();
...

That should do it!


Conversion from RGB to CIELAB with a complex formula let deltaF: TransformBlock = { f in
        let transformation = (f > pow((6.0/29.0), 3.0)) ? pow(f, 1.0/3.0) : (1/3) * pow((29.0/6.0), 2.0) * f + 4/29.0

        return (transformation)
    }

Says here that this expression is too complex to be solved in reasonable time. Is there any way to simplify it?

This code is meant to convert RGB colors into CIELAB colors. I've done some research online and couldn't find an updated version of that code.

func CIE_LAB() -> (l: CGFloat, a: CGFloat, b: CGFloat, alpha: CGFloat) {
    // Get XYZ
    let xyzT = xyz()
    let x = xyzT.x/95.047
    let y = xyzT.y/100.000
    let z = xyzT.z/108.883

    // Transfrom XYZ to L*a*b
    let deltaF: TransformBlock = { f in
        let transformation = (f > pow((6.0/29.0), 3.0)) ? pow(f, 1.0/3.0) : (1/3) * pow((29.0/6.0), 2.0) * f + 4/29.0

        return (transformation)
    }
    let X = deltaF(x)
    let Y = deltaF(y)
    let Z = deltaF(z)
    let L = 116*Y - 16
    let a = 500 * (X - Y)
    let b = 200 * (Y - Z)

    return (L, a, b, xyzT.alpha)
}

The main thing that would speed it up is to not constantly calculate constant values. For example, you can replace:

pow ((6.0 / 29.0), 3.0)

with

0.00885645167904

Preferably you'll give it a name like:

let threshold = 0.00885645167904

Likewise for the other call to pow(). pow() is a very expensive function to compute, so removing it can be a big win.


Capturing optional regex segment with PHP I need to check the end of a URL for the possible existence of /news_archive or /news_archive/5 in PHP. The below snippet does exactly what I want, but I know that I could achieve this with one preg_match rather than two. How can I improve this code to treat the /5 as an optional segment and capture it if it exists?

if (preg_match('~/[0-9A-Za-z_-]+_archive/[0-9]+$~', $_SERVER['HTTP_REFERER'], $matches) || preg_match('~/[0-9A-Za-z_-]+_archive$~', $_SERVER['HTTP_REFERER'], $matches)) {
    $page_info['parent_page']['page_label'] = ltrim($matches[0], '/');
}

Consider your first pattern:

~/[0-9A-Za-z_-]+_archive/[0-9]+$~

Let's break it down:

/ a literal string /
[0-9A-Za-z_-]+ one or more of 0-9, A-Z, a-z, _ or -
_archive a literal string _archive
/ literal slash again
[0-9]+ one or more digits
$ the end of the string must follow the one or more digits

So basically you want to make #4 and #5 optional.  To be more specific, you want either both 4 and 5, or neither 4 nor 5.

Consider this:

(a[b]+)?

This means that you have one a followed by one or more b, and that this grouped a/b entity is optional.

Letting a be #4 and b be digits like in #5, we're left with:

(/[0-9]+)?

Or:

~/[0-9A-Za-z_-]+_archive(/[0-9]+)?$~

This will capture the entire group though, like /5:

php -r "preg_match('~/[0-9A-Za-z_-]+_archive(/([0-9]+))?$~', '/news_archive/5', $m); var_dump($m);"
array(2) {
  [0] =>
  string(15) "/news_archive/5"
  [1] =>
  string(2) "/5"
}

You can just add another group to remedy that though:

~/[0-9A-Za-z_-]+_archive(/([0-9]+))?$~

Example:

php -r "preg_match('~/[0-9A-Za-z_-]+_archive(/([0-9]+))?$~', '/news_archive/44', $m); var_dump($m);"
array(3) {
  [0] =>
  string(16) "/news_archive/44"
  [1] =>
  string(3) "/44"
  [2] =>
  string(2) "44"
}

You could technically make the outside group a non-capturing group (like (?:/([0-9]+))?), but I don't think the added complication is worth not grabbing the / part too.

(By the way, sorry if you're familiar with regex and you found this excessive.  I tend to take a very verbose approach to any regex related question :).)


Array absurdity challenge This is a CodeEval challenge taken from here:

  Challenge description:

  Imagine we have an immutable array of size N which we know to be
  filled with integers ranging from 0 to N-2, inclusive. Suppose we know
  that the array contains exactly one duplicated entry and that
  duplicate appears exactly twice. Find the duplicated entry. (For bonus
  points, ensure your solution has constant space and time proportional
  to N)

  Input sample:

  Your program should accept as its first argument a path to a filename.
  Each line in this file is one test case. Ignore all empty lines. Each
  line begins with a positive integer(N) i.e. the size of the array,
  then a semicolon followed by a comma separated list of positive
  numbers ranging from 0 to N-2, inclusive):

1
2
5;0,1,2,3,0
20;0,1,10,3,2,4,5,7,6,8,11,9,15,12,13,4,16,18,17,14

  Output sample:

  Print out the duplicated entry, each one on a new line:

1
2
0
4

I've written this code:

import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.util.Scanner;
import java.util.StringTokenizer;

/**
 *
 * @author Mohammad Faisal
 */
public class ArrayAbsurdity {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner input = new Scanner(new FileInputStream("E:\\java\\temp\\abc.txt"));
        while(input.hasNextLine()){
            String str = input.nextLine();
            if(str.equals("")){
                continue;
            }
            String[] in = str.split(";");
            int i=0;
            int[] a = new int[Integer.parseInt(in[0])];
            StringTokenizer token = new StringTokenizer(in[1], ",");
            while(token.hasMoreTokens()){
                a[i]=Integer.parseInt(token.nextToken());
                i++;
            }

            boolean success=false;
            for(i=0; i<a.length-1; i++){
                for(int j=i+1; j<a.length; j++){
                    if(a[i] == a[j]){
                        System.out.println(a[i]);
                        success=true;
                        break;
                    }
                }
                if(success){
                    break;
                }
            }
        }
    }
}

Can anybody review it and help me in improving the space and time complexity as per the problem?

I don't have a comment on how you parse the input. I just give a solution which runs in O(1) space and O(N) time.

Problem says that all integers are in range 0..N-2 and there's only one integer duplicated. Consider you hadn't that duplicated element, so sum of all input numbers would be (n-2)*(n-1)/2 (sum of integers from 0 to n-2). So now you can read all N integers and calculate their sum. Then if you subtract (n-2)*(n-1)/2 from this sum, the value of your duplicated element would come out.

The code would be:

int sum = 0;
int n = a.length;
for(i=0; i<n; i++)
  sum += a[i];

System.out.println(sum - (n-2)*(n-1)/2);

This method is clearly of O(n) time, but if you want to achieve O(1) space, you should avoid storing all elements in an array. Instead of that, when you parsed nextToken, you add it to sum.


CakePHP login action returning json I have some questions on how I can improve this "add action" (method) in "controller":

I'm using the add action only if post request. Is it correct?
This action doesn't have views ($this->autoRender = false;). Is it correct?
I set a response .json file to this action but I didn't change on routes to routing .json files (the file will be return when access localhost:8765/users/add). Is it correct?
I'm using Enums(handmade) to store messages that will returned to user. Is it correct?
I'm using an object to store the fields of message (that will returned to user), that object will be serialized and returned like this:

$this->response->body(json_encode($response));

Is it correct?

Controller code:

public function login()
{
    $this->autoRender = false;
    $this->response->type('json');
    if ($this->request->is('post')) {
        $user = $this->Auth->identify();
        if ($user) {
            $this->Auth->setUser($user);
            $response = new ResponseMessage();
            $response->code = CodeEnum::LOGIN_GRANTED;
            $response->name = NameEnum::LOGIN_GRANTED;
            $response->type = TypeMessageEnum::SUCCESS;
            $this->response->body(json_encode($response));
        }else {
            $response = new ResponseMessage();
            $response->code = CodeEnum::LOGIN_DENIED;
            $response->name = NameEnum::LOGIN_DENIED;
            $response->message = MessageEnum::USER_PASS_INCORRECT;
            $response->type = TypeMessageEnum::ERROR;
            $this->response->body(json_encode($response));
        }
    }
}

If you have another point to improve, please comment or answer, I will change the accepted answer

To put in CakePHP pattern I remove both lines:

$this->autoRender = false;
$this->response->type('json');

and change how I create and return the JSON file change this:

$this->response->body(json_encode($response));

to this:

$this->set('response', $response);
$this->set('_serialize', 'response');

the result:

public function login()
{
    if ($this->request->is('post')) {
        $user = $this->Auth->identify();
        if ($user) {
            $this->Auth->setUser($user);
            $response = new ResponseMessage();
            $response->code = CodeEnum::LOGIN_GRANTED;
            $response->name = NameEnum::LOGIN_GRANTED;
            $response->type = TypeMessageEnum::SUCCESS;
            $this->set('response', $response);
            $this->set('_serialize', 'response');
        }else {
            $response = new ResponseMessage();
            $response->code = CodeEnum::LOGIN_DENIED;
            $response->name = NameEnum::LOGIN_DENIED;
            $response->message = MessageEnum::USER_PASS_INCORRECT;
            $response->type = TypeMessageEnum::ERROR;
            $this->set('response', $response);
            $this->set('_serialize', 'response');
        }
    }
}


Counting the gems among the rocks John has discovered various rocks. Each rock is composed of various
  elements, and each element is represented by a lowercase Latin letter
  from 'a' to 'z'. An element can be present multiple times in a rock.
  An element is called a 'gem-element' if it occurs at least once in
  each of the rocks.

  Given the list of rocks with their compositions, you have to print how
  many different kinds of gems-elements he has.

  Input Format

  The first line consists of N, the number of rocks. Each of the next N
  lines contain rocks’ composition. Each composition consists of small
  alphabets of English language.

  Output Format

  Print the number of different kinds of gem-elements he has.

  Constraints

  \$1 ≤ N ≤ 100\$

  Each composition consists of only small Latin letters ('a'-'z'). 1 ≤ Length of each composition ≤ 100

  Sample Input

3
abcdde
baccd
eeabg

  Sample Output

2

  Explanation

  Only "a", "b" are the two kind of gem-elements, since
  these characters occur in each of the rocks’ composition.

I tried to solve this HackerRank problem in Java. Do you see any areas for improvement?

public int getGems(List<String> list) {

    if (list != null && !(list.size() > 0)) {
        throw new IllegalArgumentException("Invalid Input");
    }

    int gemsCount = 0;
    int s = list.size();
    int cnt = 1;

    // assume list(0) will always have gems
    String str = list.get(0);
    char[] ch = str.toCharArray();
    for (char c : ch) {
        for (int i = 1; i < s; i++) {
            String st = list.get(i);
            if (st.indexOf(c) > -1) {
                cnt++;
                continue;
            } else {
                cnt = 1;
                break;
            }
        }
        if (cnt == s) {
            gemsCount++;
            cnt = 1;
        }
    }
    return gemsCount;
}

I find your variable naming maddening. For example, list is just about the least informative name possible for a list, and str and st are just about the least informative names possible for strings.

The input validation is a bit awkwardly expressed.  Also, if list is null, then it will still crash with a NullPointerException.

The comment is a bit misleading.

The many places where you reset cnt make your code hard to follow.  Without changing your general strategy too much, here's how you could write it better:

public int getGems(List<String> rocks) {
    if (rocks == null || rocks.isEmpty()) {
        throw new IllegalArgumentException("No Input");
    }

    int gemsCount = 0;
    int n = rocks.size();

    // Use list(0) as a source of candidate gems
    char[] candidateGems = rocks.get(0).toCharArray();
    for (char c : candidateGems) {
        int appearances = 0;
        for (String rock : rocks) {
            if (rock.indexOf(c) >= 0) {
                appearances++;
            } else {
                break;
            }
        }
        if (appearances == n) {
            gemsCount++;
        }
    }
    return gemsCount;
}


Transaction in spring boot I am using spring boot in my application.

@EnableAutoConfiguration
@SpringBootApplication
public class PushMysqlTestApplication implements CommandLineRunner {
    private static Logger LOGGER = LoggerFactory.getLogger(RunQuery.class);
    @Autowired
    JdbcTemplate jdbcTemplate;

    public static void main(String[] args) throws InterruptedException {
        SpringApplication.run(PushMysqlTestApplication.class, args);
    }

    @Override
    public void run(String... args) throws Exception {
        startTransaction();
    }

    public void startTransaction() throws InterruptedException {
        LOGGER.info("=========== Transaction started ===========");
        jdbcTemplate.execute("start transaction");
        jdbcTemplate.execute("select * from push_multithread where status = 1 for update");
        Thread.sleep(5000);
        jdbcTemplate.execute("update push_multithread set status = 30 where id=1");
        Thread.sleep(10000);
        jdbcTemplate.execute("commit");
        LOGGER.info("========= Transaction ending ===========");
    }

}

What the above code does is, it starts transaction, update a table and commit using JDBCTemplate.

Is their any better way to achieve above transation in spring boot using anootations or JPA etc?

Did you try to use google for this? You have multiple way to do this :P

For example: @Transactional annotation on method.

Or using XML file to wrap methods with transaction.

Just look for this on google: https://spring.io/guides/gs/managing-transactions/


How to get the median value faster Using Excel, I have one sheet ("db") with data extracted from MES.
I have to analyze this data and I need: Articolo, Fase, Tipo Confezione, TURNO, Macchina, avg and median.
I searched everywhere how to get the median with SQL, but I found only how to get median for one item alone.
Then I wrote this code, and I loop each item from 1st query to get data from another query, it works but it is extremely slow!
Is there a better way to get my data or, if my code is the correct way, how I can speed up the job?
Any suggestions are appreciated.
My code:
Sub Get_Pivot()
    Dim strsql As String
    Dim cn As New ADODB.Connection, rs As New ADODB.Recordset
    Dim RangeVal As Range, DatiVal

    Sheets("db").Select 'sheet with database

    Const adOpenStatic = 3
    Const adLockOptimistic = 3
    Const adCmdText = &H1

    Set rs = New ADODB.Recordset
    cn.Open "Provider=Microsoft.Jet.OLEDB.4.0; Data Source=" & ThisWorkbook.FullName & "; Extended Properties=""Excel 8.0;HDR=Yes;"";"

   ' select field from db
    strsql = "SELECT Articolo, Fase, [Tipo Confezione], TURNO, Macchina, avg(KgOra) as med FROM [db$] where Time>0 group by Articolo, Fase, [Tipo Confezione], TURNO, Macchina ;"
    rs.Open strsql, cn, adOpenStatic, adLockReadOnly, adCmdUnspecified

    rs.MoveFirst
    Sheets("pvt").Select  'sheet how to put data

    'insert header columns name
    For i = 0 To rs.Fields.Count - 1
        Cells(1, i + 1) = rs.Fields(i).Name
    Next i

    'insert data from recordest
    Range("A2").CopyFromRecordset rs

    'set range and get data from pasted recordset
    Set RangeVal = Range("A1", Cells(Range("A1").End(xlDown).Row, 7))
    DatiVal = RangeVal.Value
    Set rs = Nothing

'loop each row and run one qry to get median
For i = 2 To UBound(DatiVal, 1)
    strsql = "select last(KgOra) FROM (select top 50 percent KgOra from [db$] where " & _
            "Articolo = '" & DatiVal(i, 1) & _
            "' and Fase = '" & DatiVal(i, 2) & _
            "' and [Tipo Confezione] = '" & DatiVal(i, 3) & _
            "' and TURNO = '" & DatiVal(i, 4) & _
            "' and Macchina = '" & DatiVal(i, 5) & "');"

    rs.Open strsql, cn, adOpenStatic, adLockReadOnly, adCmdUnspecified

'if recordset exist add value at 7th filed of DatiVal
    If Not rs.EOF Then DatiVal(i, 7) = rs(0)
    rs.MoveNext
    Set rs = Nothing
Next

'then paste new data with one columns more(median columns)
whit RangeVal
.vaue = DatiVal
End With

rstErr:
    Set rs = Nothing
    Set cn = Nothing

End Sub

The key issue can be that you run the query with the same parameters multiple times. I assume you have quite many records where Articolo/fase/tipo/etc are the same for you are looking for median. You did not state how many records you have but I think you could reduce the number of issued queries. So - without knowing too much details - I would

Collect unique Articolo/fase/tipo/etc values

Issue the queries only once for each combination

Update median values


Logging entries into a website form I am logging entries into a website form using Spring4 and Java 8 (originally Java 7).  I was using the slf4j logging framework firstly to write to file as a PoC and then directly to db as a mistaken final implementation. Mistaken as it shouldn't talk to the db directly. So I now have a very simple REST API server running with a couple of POST end points.  So far so good.  The basic code is working with no problems.

As this is non core functionality (logging NOT auditing) I thought I should make this call asynchronous so that if there are any issues this peripheral functionality won't degrade core functionality so I thought I should make the call asynchronously.

I have a created a Service class which is injected into the controller classes and has methods like this:

public void postLog (EntryItemLog logItem){
    String uri = REST_API_ENDPOINT;

    try {
        RestTemplate restTemplate = new RestTemplate();

        ResponseEntity<EntryItemLog> response = restTemplate.postForEntity(uri, logItem, EntryItemLog class);

        LOGGER.debug("Returned logging POST with status  " + response.getStatusCode());

    }catch (Exception ex) {
        LOGGER.debug("Error Calling " + uri + ": " + ex.getMessage());
    }
}

and I am calling the function in this way:

 @RequestMapping(value = Controller.RESOURCE_NAME, method = RequestMethod.GET)
 public ModelAndView controlMethod(HttpServletRequest request, HttpServletResponse response,
            @ModelAttribute(REQUEST_MODEL_NAME) RequestBean requestBean, Model model) {
//some logging & set up

new Thread(() -> {
               dbRESTClientService.postLog(searchRequestBean.getTerm(), "searchProject", "WEB", (request.getHeader("X-FORWARDED-FOR") == null) ? request.getRemoteAddr() : request.getHeader("X-FORWARDED-FOR"));
            }).start();

//perform main functionality

I imagine this is very basic and unsophisticated for many people on this site, but is it broadly acceptable?  Can anyone offer me any advice on doing it in a better way?

I guess I have 3 queries myself but any other (broadly constructive) criticism  would gratefully accepted as the is a fairly new area to me:

Starting a new thread seems like quite an overhead for this task but I don't see an alternative (other than using a 3rd party tool like Jersey which will presumably do the same thing anyway?)
Should I be doing something more sophisticated than creating a local RestTemplate each time?
Is it generally the right approach anyway (it feels over simplistic)?

1) Starting a new thread seems like quite an overhead for this task
  but I don't see an alternative (other than using a 3rd party tool like
  jersey which will presumably do the same thing anyway ?)

I would use there some message processing tool. Send somewhere and than process what should be processed. But it's much more complicated. I would use ExecutorService which is provided by java-8. I think it's old school to create Thread manually for this case.

  2) Should I be doing something more sophisticated than creating a
  local RestTemplate each time ?

You can use @Configuration FactoryBean where you can create one spring @Bean for RestTemplate and inject this instance into your class where is postLog method located.

@Configuration
public class BeansFactory {
 @Bean public RestTemplate restTemplate(){
       return new RestTemplate();
 }
}

and inject in spring @Component

@Autowire private RestTemplate restTemplate;

  3) Is it generally the right approach anyway (it feels over
  simplistic)

From my perspective doing async work for REST with Threads is little bit tricky. And open to few potential problems like - too many Threads, by default all threads with the same priority etc.

I think that for some async tasks the best way is to have publisher-subscriber approach. You are doing something, then you want to run something in the background to avoid waiting for response and return response without already finised async call.

To avoid doing another call you can push somewhere this event and process event somewhere else which is not depended on the current application.

Check also https://spring.io/guides/gs/messaging-reactor/