Stack with a minimum A practice interview question:

  How would you design a stack which, in addition to push and pop, also has a function getMin which returns the minimum element? Push, pop and min should all operate in O(1) time.

class stackWithMin
{
private:
    std::vector<int> stack;
    std::vector<int> minLoc;
    int min, current, minCurrent;

public:
    stackWithMin()
    {
        min = NULL;
        current = 0;
        minCurrent = 0;
    }

    void print()
    {
        std::cout<<"Min: "<<min<<" --- ";

        for (int i=0;i<stack.size();i++)
        {
            std::cout<<""<<stack.at(i)<<", ";
        }
        std::cout<<""<<std::endl;
    }

    void push(int num)
    {
        stack.push_back(num);

        if(num<=min || current == 0)
        {
            minLoc.push_back(current);
            minCurrent++;
            min = num;
        }
        current++;
        print();
    }

    void pop()
    {
        if(current==0)
        {
            std::cout<<"Stack is empty"<<std::endl;
            return;
        }

        // Check if the number to be popped is the current lowests
        if(minLoc.at(minCurrent-1)==current-1)
        {
            if(minCurrent>1)
            {
                int loc = minLoc.at(--minCurrent-1);
                minLoc.pop_back();
                min = stack.at(loc);
            }
            // No more elements
            else
            {
                min = NULL;
            }
        }

        current--;
        stack.pop_back();
        print();
        return;
    }

    int getMin(){return min;}

};

Here are some things that may help you improve your code:

Simplify the algorithm

Although it would take more space to store, a simpler algorithm would be to simply use minLoc as the current minimum.  If you did so, then getMin() would simply be return minLoc.back();

Eliminate redundant variables

Even if you don't care for that particular algorithm, you can remove the variables min, current and minCurrent and also simplify the code:

void push(int num)
{
    stack.push_back(num);
    if (minLoc.size() == 0 || num < stack[minLoc.back()])
        minLoc.push_back(stack.size()-1);
    print();
}

void pop()
{
    // Check if the number to be popped is the current lowest
    if(minLoc.size() && minLoc.back() == stack.size()-1)
        minLoc.pop_back();
    stack.pop_back();
    print();
}

int getMin() const {
    return minLoc.size() ? stack[minLoc.back()] : 0;
}

Use const where possible

The getMin function doesn't (and shouldn't) modify the underlying stack, so it should be declared const.  The same is true for the print function.

Avoid using index variables

Rather than using an index variable i in the print routine and then calling at() for each iteration, use iterators instead.  For example, you could use this:

std::copy(stack.cbegin(), stack.cend(),
          std::ostream_iterator<int>(std::cout, ", "));

Omit empty strings

The current code for print includes this line:

std::cout<<""<<stack.at(i)<<", ";

There's no need for the empty string in that line.

Use whitespace to improve readability

To take the previously quoted line as an example, rewriting it with more whitespace makes it easier to read:

std::cout << stack.at(i) << ", ";

Don't use std::endl when '\n' will do

Using std::endl emits a \n and flushes the stream.  Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Take care with signed versus unsigned

The code includes this line in the print function:

for (int i=0;i<stack.size();i++)

However, stack.size() is unsigned and i is signed.  For consistency, it would be better to declare i as std::size_t which is the type returned by size().

Consider the user of the code

The std::vector::pop_back() returns void but provides a member function back() to allow a user of the code to access the last member before removing it from the vector.  You might consider either returning the popped value in your implementation of pop or implementing a back function such as the one std::vector supplies.


Enabling or disabling buttons on certain events I need to run the following checks in two occasions:

When a certain modal is first shown or clicked,
When one of two inputs is filled.

How can I remove redundancy in the following code?

jQuery(document).ready( function($) {
    $(document.body).on( 'DOMNodeInserted click',  '.media-modal', function( event ) {
        var test = $('label.setting[data-setting=alt] input:visible').val();
        var test2 = $('input[data-setting=alt]:visible').val();
        if (!test && !test2) {
            $('.media-modal .media-button-insert').prop("disabled",true);
            $('.media-modal .media-button-select').prop("disabled",true);
        } else {
            $('.media-modal .media-button-insert').prop("disabled",false);
            $('.media-modal .media-button-select').prop("disabled",false);
        }
        $(document.body).on( 'input propertychange paste',  'label.setting[data-setting=alt] input, input[data-setting=alt]', function( event ) {
            var test = $('label.setting[data-setting=alt] input:visible').val();
            var test2 = $('input[data-setting=alt]:visible').val();
            if (!test && !test2) {
                $('.media-modal .media-button-insert').prop("disabled",true);
                $('.media-modal .media-button-select').prop("disabled",true);
            } else {
                $('.media-modal .media-button-insert').prop("disabled",false);
                $('.media-modal .media-button-select').prop("disabled",false);
            }
        });
    });
});

I would write it as something like:

jQuery(document).ready( function($) {

    function updateDisabled() {
        var test     = $('label.setting[data-setting=alt] input:visible').val();
        var test2    = $('input[data-setting=alt]:visible').val();
        var disabled = !test && !test2;
        $('.media-modal .media-button-insert').prop("disabled", disabled);
        $('.media-modal .media-button-select').prop("disabled", disabled);
    }

    $(document.body).on( 'DOMNodeInserted click', '.media-modal', updateDisabled);
    $(document.body).on( 'input propertychange paste', 'label.setting[data-setting=alt] input, input[data-setting=alt]', updateDisabled);

}

You could also rewrite:

$('.media-modal .media-button-insert').prop("disabled", disabled);
$('.media-modal .media-button-select').prop("disabled", disabled);

As

$('.media-modal .media-button-insert, .media-modal .media-button-select').prop("disabled", disabled);

(Although I prefer keeping them separate) or maybe better giving your buttons an extra class and writing:

$('.media-modal .media-button').prop("disabled", disabled);


Hackerrank Gemstones Solution I'm looking for any possible improvements in terms of memory usage and speed, but really any advice is welcome.

  Problem statement:

  John has discovered various rocks. Each rock is composed of various
  elements, and each element is represented by a lower-case 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, display the number
  of gem-elements that exist in those rocks.

  Input Format

  The first line consists of an integer, , the number of rocks.  Each of
  the next  lines contains a rock's composition. Each composition
  consists of lower-case letters of English alphabet.

  Constraints    Each composition consists of only lower-case Latin letters ('a'-'z').   length of each composition

  Output Format

  Print the number of gem-elements that are common in these rocks. If
  there are none, print 0.

My code: Essentially I build N lists of True or False values (since the number of times the same element appears doesn't matter), and at the end just "and" them two by two using reduce to end up with a final list of True values for the elements that were True in each rock, and then just sum up the True values.

import sys
from functools import reduce

# this is just to get test cases input
N = int(input())
strings = []
for i in range(N):
    strings.append(input())

# actual implementation starts here
lists = [[False] * 26 for i in range(N)]

for i in range(N):
    for c in strings[i]:
        # ord() gives the ASCII value of a character
        lists[i][ord(c) - ord('a')] = True

final_list = reduce(lambda x, y: [x[i] and y[i] for i in range(26)], lists)

print(reduce(lambda x, y: x + y, final_list))

There are several ways you can save memory, where you’re storing more information than you really need. In some cases, way more information than you need.

If you only save what you need, you’ll reduce memory and get a performance benefit to boot!

Memory improvement #1

These three lines are wasting memory:

strings = []
for i in range(N):
    strings.append(input())

You don’t actually need to gather up all the rocks into a list – you’re only ever looking at one at a time. Although in this case it’s unlikely to matter, creating a list when you don’t need one is a bad antipattern.

You can rewrite your code to make the strings list unnecessary, thus saving memory, like so:

lists = [[False] * 26 for i in range(N)]

for i in range(N):
    rock = input()
    for c in rock:
        # ord() gives the ASCII value of a character
        lists[i][ord(c) - ord('a')] = True

Note that this also only features one for loop of length N, which will likely have a (small) benefit to performance.

Memory improvement #2

The lists variable (that’s not a great name, btw) is a 26 x N-sized matrix. Again, this has the potential to get quite large if you have lots of rocks, or lots of different elements. And you’re storing more data than you need to – all we’re interested in is which elements were present in each rock. A simpler way to store that would be something like:

rock_elements = []

for _ in range(N):
    rock = input()
    rock_elements.append(set(rock))

final_list = reduce(lambda x, y: [i in x and i in y for i in range(26)], lists)

Now the rock_elements list only contains the set of elements in each rock. We’re not provisioning lots of unwanted memory for small rocks.

Memory improvement #3

We’re still storing more information than we need. For example, suppose the first rock omits a, and every other rock contains a. Remembering that those later rocks contain a is pointless, because we know that a isn’t a gem-element.

So an even more efficient solution would be to keep a set of possible gem-elements, and throw elements away whenever we find a rock that doesn’t contain them. Like so:

import string

gem_elements = set(string.ascii_lowercase)

for _ in range(N):
    rock = input()
    gem_elements = gem_elements & set(rock)

print(len(gem_elements))

Now our gem_elements set will never be larger than the number of possible elements, and we’re only storing the data for one rock at a time. Much better!

And as a nice side benefit, we’ve got rid of the two calls to reduce(), which are:

Generally not very Pythonic – idiomatic Python tends towards list comprehensions and similar rather than reduce() and map().
Moderately expensive – I think at least \$O(N)\$ in the number of rocks?


Sending GET/POST requests in .NET I have a RequestsExtensions class with two functions for sending GET/POST requests.

using System;
using System.Net;
using System.Text;

    public static class RequestsExtensions
    {
        /// <summary>
        /// Sending POST request.
        /// </summary>
        /// <param name="Url">Request Url.</param>
        /// <param name="Data">Data for request.</param>
        /// <returns>Response body.</returns>
        public static string HTTP_POST(string Url, string Data)
        {
            string Out = String.Empty;
            System.Net.WebRequest req = System.Net.WebRequest.Create(Url);
            try
            {
                req.Method = "POST";
                req.Timeout = 100000;
                req.ContentType = "application/x-www-form-urlencoded";
                byte[] sentData = Encoding.UTF8.GetBytes(Data);
                req.ContentLength = sentData.Length;
                using (System.IO.Stream sendStream = req.GetRequestStream())
                {
                    sendStream.Write(sentData, 0, sentData.Length);
                    sendStream.Close();
                }
                System.Net.WebResponse res = req.GetResponse();
                System.IO.Stream ReceiveStream = res.GetResponseStream();
                using (System.IO.StreamReader sr = new System.IO.StreamReader(ReceiveStream, Encoding.UTF8))
                {
                    Char[] read = new Char[256];
                    int count = sr.Read(read, 0, 256);

                    while (count > 0)
                    {
                        String str = new String(read, 0, count);
                        Out += str;
                        count = sr.Read(read, 0, 256);
                    }
                }
            }
            catch (ArgumentException ex)
            {
                Out = string.Format("HTTP_ERROR :: The second HttpWebRequest object has raised an Argument Exception as 'Connection' Property is set to 'Close' :: {0}", ex.Message);
            }
            catch (WebException ex)
            {
                Out = string.Format("HTTP_ERROR :: WebException raised! :: {0}", ex.Message);
            }
            catch (Exception ex)
            {
                Out = string.Format("HTTP_ERROR :: Exception raised! :: {0}", ex.Message);
            }

            return Out;
        }

        /// <summary>
        /// Sending GET request.
        /// </summary>
        /// <param name="Url">Request Url.</param>
        /// <param name="Data">Data for request.</param>
        /// <returns>Response body.</returns>
        public static string HTTP_GET(string Url, string Data)
        {
            string Out = String.Empty;
            System.Net.WebRequest req = System.Net.WebRequest.Create(Url + (string.IsNullOrEmpty(Data) ? "" : "?" + Data));
            try
            {
                System.Net.WebResponse resp = req.GetResponse();
                using (System.IO.Stream stream = resp.GetResponseStream())
                {
                    using (System.IO.StreamReader sr = new System.IO.StreamReader(stream))
                    {
                        Out = sr.ReadToEnd();
                        sr.Close();
                    }
                }
            }
            catch (ArgumentException ex)
            {
                Out = string.Format("HTTP_ERROR :: The second HttpWebRequest object has raised an Argument Exception as 'Connection' Property is set to 'Close' :: {0}", ex.Message);
            }
            catch (WebException ex)
            {
                Out = string.Format("HTTP_ERROR :: WebException raised! :: {0}", ex.Message);
            }
            catch (Exception ex)
            {
                Out = string.Format("HTTP_ERROR :: Exception raised! :: {0}", ex.Message);
            }

            return Out;
        }
    }

Do you see any improvement / issue?

First off your methods are not extension methods as the name RequestsExtensions would indicate. Not sure if this intentional or. If you want to make them true extension methods then I'd consider passing the Url parameter as Uri rather than as string.

Other things I noticed:

The use of a magic number here: req.Timeout = 100000; - This should be a parameter (maybe with a default value) you can pass in.
Your naming convention is a bit weird. Standard naming convention for methods in C# is PascalCase. Also local variables are normally camelCase (your Out variable is not while other variables like req are).
In HTTP_POST you use a block-wise read to get the response while in HTTP_GET you simply read to the end of the response stream - why the difference? The HTTP_GET version seems much simpler so why not do it the same way? In fact both those method get a response and read it and deal with the exceptions - this is duplicated code which you should move into a dedicated function which you can then call from both main methods like this:

private static string GetResponse(System.Net.WebRequest request)
{
    string result = "";
    try
    {
        System.Net.WebResponse resp = req.GetResponse();
        using (System.IO.Stream stream = resp.GetResponseStream())
        {
            using (System.IO.StreamReader sr = new System.IO.StreamReader(stream))
            {
                result = sr.ReadToEnd();
                sr.Close();
            }
        }
    }
    catch
      ....

    return result;
}

The way you handle exceptions is less than ideal. The caller of your methods has no easy way to find out if the call actually succeeded or not - except for parsing the return value of the method.

A better way to deal with it is to let exceptions bubble up to the caller and let the caller decide what to do (e.g. terminate the process, retry later, etc.). In addition you could log the exception (requires passing in some kind of logger)

Alternatively you could make the return value an out parameter and return a bool indicating if the call was successful instead if you really want to hide the exceptions from the caller. In this case I'd rename the methods to something like TryHttpPost and TryHttpGet


Ubuntu-Nginx lightweight server environment setup Please review the following Bash script that uses to establish a "lightweight" Ubuntu-Nginx server environment mainly aimed to run small WordPress sites (about 5 plugins, about 25 webpages, about 25 images) on a cloud hosting platform.

The BASHRC heredocument sets aliases for personal scripts I already have and might use me later on.

apt-get update -y && apt-get upgrade -y && add-apt-repository ppa:certbot/certbot -y
ufw enable && ufw allow 22/tcp 80/tcp 443/tcp 9000/tcp && ufw allow 53/upd
apt-get install zip unzip tree unattended-upgrades sshguard postfix nginx python-certbot-nginx mysql-server php-fpm php-mysql php-mbstring php-mcrypt -y

sed -i "s/# gzip_/gzip_/g" /etc/nginx/nginx.conf
sed -i "s/max_size = .M/max_size = 200M/g" /etc/php/*/fpm/php.ini
sed -i "s/;cgi.fix_pathinfo=1/cgi.fix_pathinfo=0/g" /etc/php/*/fpm/php.ini
/etc/init.d/php*-fpm restart && systemctl restart nginx.service

cat <<-"BASHRC" >> /etc/bash.bashrc
    alias rss="/etc/init.d/php*-fpm restart && systemctl restart nginx.service"
    alias brc="nano /etc/bash.bashrc"
    alias www="cd /var/www/html"
    alias pma="tmux new-session -d 'bash ~/scripts/pma.sh"
    alias imb="bash ~/scripts/imb.sh"
BASHRC
source /etc/bash.bashrc

Good

Tying together related commands with && is a good error-avoidance strategy.

Trouble

The code is not re-entrant because the aliases will keep getting appended every time.  This is bad because you can only run or "enter" the code once safely.  You could touch file if the aliases are added and use that as a flag.  I would create the aliases in file and add a sourceing line to bashrc.  If you add the source line before creating the aliases file the aliases file could serve as your flag file for whether to add that line or not.
Running source on anything that doesn't have some side effect at the end of your script is pointless.  The source is causing the aliases you added to get read into the current shell.  But since you're at the end of your script there's no command that will use the aliases so it was a waste of effort to read them in.  Maybe you intend to do more with these later in the script, but it seems they are intended for the interactive user.  So sourceing the new aliases in an interactive shell would make them available to the interactive user sooner than logging out and logging back in would.
Some comments might save you from rethinking what you did someday.

Maybe trouble

Did you leave out a single quote in the pma alias?
Can you have more than one version of php-fpm installed?  If so your * in the command will expand in a weird way.  Maybe you can put the version you expect in a variable or use packaging commands to the find the version that is installed.
No sanity checking.  It might be good to error out if the files you are looking for turn out to not be around.
I hope you're storing your code in git somewhere.  The lack of a github link makes me wonder about this.


Performance problem: Ranking with ties and skipping rows on passmark condition I am using the following code for calculating ranking and it is working great for not large data. But when it comes to large data, processing time takes almost a minute. Please can anyone suggest a better way or atleast a faster way to put the same mysql statement?

 SELECT id, Names, TOTALSCORE, Rank
 FROM
 (
 SELECT t.*,
     IF(@p = TOTALSCORE, @n, @n := @n + 1) AS Rank,
     @p := TOTALSCORE
  FROM(
  SELECT id, Names,SUM(score) TOTALSCORE
     FROM exam e1, (SELECT @n := 0, @p := 0) n
     WHERE NOT EXISTS(
        SELECT null FROM exam e2
        WHERE e1.id = e2.id
        AND e2.score/e2.Fullmark < 0.33
      )
     GROUP BY id
     ORDER BY TOTALSCORE DESC
  ) t
 ) r
 ;

Fiddle is here:ranking with ties and skipping rows on passmark condition

First, let's confirm the intention of your query.  According to my interpretation, it ranks students according to the sum of their exam scores, best students first, but disqualifying anyone who has ever failed an exam by scoring lower than 33%.  (Why does the disqualification rule exist?  Does failing an exam allow a student to take it again?  If so, a fairer rule would be to disqualify students who have retaken an exam.)

Next, I would like to point out problems in your schema.  You included this table definition in your SQL Fiddle, which you should also have stated within the question itself (it's essential information for reviewing your code):

CREATE TABLE exam
    (`id` int, `Names` varchar(6), `Subject` varchar(8), `Exam` varchar(5), `Fullmark` int, `score` int)
;

Naming the first column id is weird and tricky: at first glance, one would expect the id column of an exam table to identify an exam, but you use it as a student ID.

That oddity is an indication of another anomaly: your schema is denormalized.  There is a M : N relationship between students and exam scores.  (Each student takes M exams; each exam is taken by N students.)  There should be three tables:

CREATE TABLE student
( id SERIAL PRIMARY KEY
, `Names` VARCHAR(6) NOT NULL  -- Probably too short
);

-- Using the long name to distinguish it from your exam table
CREATE TABLE examination
( id SERIAL PRIMARY KEY
, `Subject` VARCHAR(8) NOT NULL
, `Type` VARCHAR(5) NOT NULL
, `Fullmark` INTEGER NOT NULL
);

CREATE TABLE examination_score
( student_id INTEGER NOT NULL
, examination_id INTEGER NOT NULL
, `Score` INTEGER NOT NULL
, FOREIGN KEY (student_id) REFERENCES student (id)
, FOREIGN KEY (examination_id) REFERENCES examination (id)
);

To populate the new tables, you would use the query

INSERT INTO student
    SELECT DISTINCT `id`, `Names`
        FROM exam;

INSERT INTO examination (`Subject`, `Type`, `Fullmark`)
    SELECT DISTINCT `Subject`, `Exam`, `Fullmark`
        FROM exam
        ORDER BY `Subject`, `Exam`;

INSERT INTO examination_score
    SELECT exam.id AS student_id
         , examination.id AS examination_id
         , exam.score AS score
        FROM exam
            INNER JOIN examination
                ON examination.`Subject` = exam.`Subject`
                AND examination.`Type` = exam.`Exam`
                AND examination.`Fullmark` = exam.`Fullmark`;

Notice that you misspelled "Science" in two different ways, as well as "Mizo" (whatever that is).  The PRIMARY KEY constraint caught the misspelling of the student names "Lawma" and "Thanga".

Verify that this query works:

SELECT student_id
     , SUM(`Score`) AS TOTALSCORE
    FROM examination_score AS e
        WHERE NOT EXISTS (
            SELECT dq.student_id
                FROM examination_score AS dq
                    INNER JOIN examination
                        ON examination.id = dq.examination_id
                WHERE
                    dq.`Score` / `Fullmark` < 0.33
                    AND dq.student_id = e.student_id
        )
    GROUP BY student_id
    ORDER BY TOTALSCORE DESC;

Compare its performance against the equivalent query based on the denormalized schema:

SELECT id, Names,SUM(score) TOTALSCORE
   FROM exam e1
   WHERE NOT EXISTS(
      SELECT null FROM exam e2
      WHERE e1.id = e2.id
      AND e2.score/e2.Fullmark < 0.33
    )
   GROUP BY id
   ORDER BY TOTALSCORE DESC;

My guess is that the revised query will be faster.  The root cause is that you didn't have an index on the id column of your exam table.  You could just create the index, but normalizing the schema and writing the appropriate PRIMARY KEY constraints would implicitly create such indexes, and is better practice.


Class with TryParse static method I am receiving and Expression (String) as follows:

String expression = "+name;-age;-city";

I parse this expression by splitting it using ";" and interpreting the +/- signs and create a List<OrderRule>. To wrap this I created the following:

public class OrderExpression {

  public HashSet<OrderRule> Rules { get; set; }

  public static Boolean TryParse(String expression, out OrderExpression orderExpression) {
    // Parse expression into a List<OrderRule>
    if (_parser.ExpressionIsValid(expression)) {
       orderExpression = new OrderExpression { Rules = _parser.Parse(expression) }
       return true;
    else
       return false;
  }
}

So I would use it as follows:

String expression = "+name;-age;-city";
OrderExpression orderExpression;
OrderExpression.TryParse(expression, out orderExpression);

Does this make sense? I am not sure if this architecture and naming is the way to go.

I am being picky about this because I will use it as a standard for an API to convert an order expression into a List.

The TryParse or TryGetValue pattern is often used in the .NET Class Library and is a good approach. In order to fully comply with it, you should always return the type's default value default(T) for the output parameter when the parsing fails. For reference types this is always null:

public static bool TryParse(string expression, out OrderExpression orderExpression) {
    if (_parser.ExpressionIsValid(expression)) {
        orderExpression = new OrderExpression { Rules = _parser.Parse(expression) };
        return true;
    } else {
        orderExpression = null;
        return false;
    }
}

An out parameter must be assigned before leaving the method anyway. You'll get a compiler error otherwise.

Also, in C#, the C# type aliases are usually preferred over the .NET type names. I.e.: use string instead of System.String, bool instead of System.Boolean, int instead of System.Int32 and so on. (Personally, I prefer to use the .NET type name when accessing static members: Int32.TryPare(...).)

When calling a method, the return value can be ignored in C#, but here, it seems logical to react differently, depending on the outcome of the parsing:

if(OrderExpression.TryParse(expression, out orderExpression)) {
    // Use orderExpression.
} else {
    // Handle the error case.
}

A comment regarding the logic: Order expressions usually have to be executed in a specific order, however, a HashSet<T> is unordered. Return a List<OrderExpression> instead. Also, the setter can be private.

public List<OrderRule> Rules { get; private set; }

UPDATE

Since C# 6.0 you can declare getter-only auto-properties. You can set them in the constructor or in a property initializer.

To set the Rules property I declare a constructor. It can be private to force a client to use the static TryParse factory method.

public class OrderExpression
{
    private OrderExpression(List<OrderRule> rules)
    {
        Rules = rules;
    }

    public List<OrderRule> Rules { get; }

    public static bool TryParse(string expression, out OrderExpression orderExpression)
    {
        if (_parser.ExpressionIsValid(expression)) {
            orderExpression = new OrderExpression(_parser.Parse(expression));
            return true;
        } else {
            orderExpression = null;
            return false;
        }
    }
}

Since C# 7.0 you can declare the out-parameter in-line

string expression = "+name;-age;-city";
if (OrderExpression.TryParse(expression, out var orderExpression)) {
    //TODO: use orderExpression.
} else {
    //TODO: handle pare error.
}


Checking database value script I made a PHP script to check value before update. This script is working fine. But, I wonder how to simplify this. If there are hundreds of columns to be checked then it's hundreds of PHP lines.

This is used for saving a log file for update. So, if data is updated then it stores a log in a table.

public function edit_project_master(){
$no_pro     = 'PRJ-2017-501'; //
$na_pro     = $this->input->post('na_pro'); //value for edit
$ja_pro     = $this->input->post('i_metode'); //value for edit

$check_pro  = $this->MProject->get_det_pro($no_pro); //get data form table

//checking script
foreach($check_pro as $res_check_pro){

    if($na_pro != $res_check_pro['NAMA_PROJECT']){  //IF VALUE DIFFERENT
        $col_name;

        foreach($res_check_pro as $key_pro => $val_pro){
            if($res_check_pro['NAMA_PROJECT'] == $val_pro){
                $col_name = $key_pro;            //GET COLUMN NAME
            }
        }

        $data['ACTIVITY']       = "UPDATE";
        $data['OLD_VALUE']      = $res_check_pro['NAMA_PROJECT'];
        $data['NEW_VALUE']      = $na_pro;
        $data['COL_AFFECTED']   = $col_name;
        $data['TABLE_AFFECTED'] = "PM_REQUIREMENT_PROJECT";  //How to get table name automatically??
        $data['ANNOTATION']     = NULL;
        $data['EXECUTOR']       = $this->session->userdata('nip');
        $data['CHANGED_ON']     = date("Y-m-d H:i:s");
        $data['CHANGED_BY']     = $_SERVER['REMOTE_ADDR'];
        $data['STATUS']         = 1;

        $this->MProject->ins_system_log($data);
    }

    if($ja_pro != $res_check_pro['JADWAL_PROJECT']){
        $col_name;

        foreach($res_check_pro as $key_pro => $val_pro){
            if($res_check_pro['JADWAL_PROJECT'] == $val_pro){
                $col_name = $key_pro;
            }
        }

        $data['ACTIVITY']       = "UPDATE";
        $data['OLD_VALUE']      = $res_check_pro['JADWAL_PROJECT'];
        $data['NEW_VALUE']      = $na_pro;
        $data['COL_AFFECTED']   = $col_name;
        $data['TABLE_AFFECTED'] = "PM_REQUIREMENT_PROJECT"; //How to get table name automatically??
        $data['ANNOTATION']     = NULL;
        $data['EXECUTOR']       = $this->session->userdata('nip');
        $data['CHANGED_ON']     = date("Y-m-d H:i:s");
        $data['CHANGED_BY']     = $_SERVER['REMOTE_ADDR'];
        $data['STATUS']         = 1;

        $this->MProject->ins_system_log($data);
    }

}

}

You can avoid repeating whole blocks of code by isolating the dynamic components of your function and assign them to an array.  Then as you wish to modify the fields/columns to be checked, you merely need to update the array and not the functional/checking parts.

public function edit_project_master(){
    $no_pro     = 'PRJ-2017-501';
    $check_pro  = $this->MProject->get_det_pro($no_pro); //get data form table

    $checklist=[
        ['input'=>'na_pro','column'=>'NAMA_PROJECT','table'=>'PM_REQUIREMENT_PROJECT'],
        ['input'=>'i_metode','column'=>'JADWAL_PROJECT','table'=>'PM_REQUIREMENT_PROJECT']
    ];

    //checking script
    foreach($check_pro as $res_check_pro){
        foreach($checklist as $listrow){
            $pro = $this->input->post($listrow['input']); //value for edit
            if($pro != $res_check_pro[$listrow['column']]){  //IF VALUE DIFFERENT
                foreach($res_check_pro as $key_pro => $val_pro){
                    if($res_check_pro[$listrow['column']] == $val_pro){
                        $col_name = $key_pro;            //GET COLUMN NAME
                        break;  // I assume you want to break as soon as you find a match
                    }
                }
                $data['ACTIVITY']       = "UPDATE";
                $data['OLD_VALUE']      = $res_check_pro[$listrow['column']];
                $data['NEW_VALUE']      = $pro;
                $data['COL_AFFECTED']   = $col_name;
                $data['TABLE_AFFECTED'] = $listrow['table'];  // I'm not sure how you mean "automatically", this is "dynamically"
                $data['ANNOTATION']     = NULL;
                $data['EXECUTOR']       = $this->session->userdata('nip');
                $data['CHANGED_ON']     = date("Y-m-d H:i:s");
                $data['CHANGED_BY']     = $_SERVER['REMOTE_ADDR'];
                $data['STATUS']         = 1;
                $this->MProject->ins_system_log($data);
            }
        }
    }
}

Also...

I added break; to the innermost foreach() assuming there will only be one match.  This is best practice because it avoids doing pointless iterations.

I made the value for $data['TABLE_AFFECTED'] "dynamic" (modifiable via the $checklist array).  I don't know if this is what you had in mind by "automatic".


Python Time Class definition and methods, use of __init__ I've written a Time class that records the time of day and performs simple timing operations (add 2 times, convert a time object to an integer and back again,etc.) following the prompts in How to Think Like a Computer Scientist: Learning with Python.

class Time(object):
"""Attributes: hours, minutes, seconds"""
def __init__(self,hours,minutes,seconds):
    self.hours =hours
    self.minutes=minutes
    self.seconds=seconds

def print_time(self):
    """prints time object as a string"""
    print "%.2d:%.2d:%.2d" % (self.hours, self.minutes, self.seconds)

def _str_(self):
    """returns time object as a string"""
    return "%.2d:%.2d:%.2d" % (self.hours, self.minutes, self.seconds)

def name(self,name):
    """names an instance"""
    self.name=name
    return self.name

def after(t1,t2):
    """checks to see which of two time objects is later"""
    if t1.convert_to_seconds()<t2.convert_to_seconds():
        return "%s is later" %(t2.name)
    elif t1.convert_to_seconds>t2.convert_to_seconds:
        return "%s is later" %(t1.name)
    else:
        return "these events occur simultaneously"

def convert_to_seconds(self):
    """converts Time object to an integer(# of seconds)"""
    minutes=self.hours*60+self.minutes
    seconds=minutes*60+self.seconds
    return seconds

def make_time(self,seconds):
    """converts from an integer to a Time object"""
    self.hours=seconds/3600
    seconds=seconds-self.hours*3600
    self.minutes=seconds/60
    seconds=seconds-self.minutes*60
    self.seconds=seconds

def increment_time(self, seconds):
    """Modifier adding a given # of seconds to a time object
    which has been converted to an integer(seconds); permanently
    alters object"""
    sum=self.convert_to_seconds()+seconds
    self.make_time(sum)
    return self._str_()

def add_time(self, addedTime):
    """adds 2 Time objects represented as seconds;
       does not permanently modify either object"""
    import copy
    end_time=copy.deepcopy(self)
    seconds=self.convert_to_seconds()+addedTime.convert_to_seconds()
    end_time.make_time(seconds)
    return end_time._str_()

Usage examples:

breakfast=Time(8,30,7)
dinner=Time(19,0,0)
smokebreak=Time(19,0,0)
interval=(4,30,0)

print dinner.after(smokebreak)
print breakfast.add_time(interval)
print breakfast.increment_time(3600)

The Time Class example in the text I'm following does not use an init method, but passes straight into creating a time object and assigning attributes. Is there any advantage to including an init function, as I have done? Removing the init method seems to make it easier to adopt a terser and more functional style.
Is including a method to name instances bad form, as I suspect? Would it be better write a functional-style version of add_time without importing deepcopy?
I would appreciate any advice on best practices in Python 2.x OOP.

It is perfectly fine to implement an __init__ method in this case. I think the only thing you should note is, by the way it's defined, the Time class forces the programmer to give values for hours, minutes and seconds to define a Time object. So, with that constraint in mind, it's really up to you as to whether this is an advantage or disadvantage. Do you want to force the programmer (most likely yourself) to enter these values here? Or should you allow him to first construct an object and then define them later? This is your decision; I don't think Pythoneers will try to sway you one way or another.

Alternatives are (1)  as you've already implied: removal or (2) giving these variables default values.

For example:

def __init__(self,hours=None,minutes=None,seconds=None):
    self.hours =hours
    self.minutes=minutes
    self.seconds=seconds

With the above you are giving the user the option to define these later or now with no penalty either way. Just keep the following simple philosophy of Python in mind:

  Easier to ask for forgiveness than permission

(From the Python Glossary)

I don't really see a point in naming instances. Do you have a rationale for that? Also, if you choose to name them, I think that returning said name after setting is an unexpected behavior and gives the name function more responsibility than it needs.

In your add_time method, I would suggest constructing a new Time object using the values of the self object and then returning that with the incrementation. And, in general, import statements occur at the top of the Python module, unless it is a really special case.

Overall, everything looks pretty good. I hope this was somewhat helpful, if you have any questions be sure to comment.


Using pytest fixtures for the expected answers to a test? I want to test a number of different functions I have written to solve the Fizzbuzz challenge and would like to know what is the best way to check the result?
Each function returns a list of the answers and I want to check the list is correct. One of the functions is shown below
def fizz_buzz_using_boolean():
    fizz_buzz_list = []

    for x in range(1, 101):

        fizz = x % 3 == 0
        buzz = x % 5 == 0

        if fizz and buzz:
            fizz_buzz_list.append('fizzbuzz')
            continue
        elif fizz:
            fizz_buzz_list.append('fizz')
            continue
        elif buzz:
            fizz_buzz_list.append('buzz')
            continue
        else:
            fizz_buzz_list.append(x)

    return fizz_buzz_list

My tests look like this:
import pytest
from fizz_buzz import fizz_buzz_if_calc, fizz_buzz_using_boolean

@pytest.fixture
def fizzbuzz_expected_answer():
    """A list of the fizzbuzz answers for 1 - 100"""
    fizz_buzz = [1, 2, 'fizz', 4, 'buzz', 'fizz', 7, 8, 'fizz', 'buzz', 11, 'fizz', 13, 14, 'fizzbuzz', 16, 17, 'fizz', 19, 'buzz', 'fizz', 22, 23, 'fizz', 'buzz', 26, 'fizz', 28, 29, 'fizzbuzz', 31, 32, 'fizz', 34, 'buzz', 'fizz', 37, 38, 'fizz', 'buzz', 41, 'fizz', 43, 44, 'fizzbuzz', 46, 47, 'fizz', 49, 'buzz', 'fizz', 52, 53, 'fizz', 'buzz', 56, 'fizz', 58, 59, 'fizzbuzz', 61, 62, 'fizz', 64, 'buzz', 'fizz', 67, 68, 'fizz', 'buzz', 71, 'fizz', 73, 74, 'fizzbuzz', 76, 77, 'fizz', 79, 'buzz', 'fizz', 82, 83, 'fizz', 'buzz', 86, 'fizz', 88, 89, 'fizzbuzz', 91, 92, 'fizz', 94, 'buzz', 'fizz', 97, 98, 'fizz', 'buzz']

    return fizz_buzz

def test_fizz_buzz_using_boolean_has_correct_items(fizzbuzz_expected_answer):

    actual = fizz_buzz_using_boolean()

    assert actual == fizzbuzz_expected_answer

I have used a fixture to return the correct answer. Whilst the tests pass I can't find other examples of fixtures being used this way.
The other solutions come to mind is to use a global
import pytest
from fizz_buzz import fizz_buzz_if_calc, fizz_buzz_using_boolean

FIZZBUZZ_EXPECTED_ANSWER = [1, 2, 'fizz', 4, 'buzz', 'fizz', 7, 8, 'fizz', 'buzz', 11, 'fizz', 13, 14, 'fizzbuzz', 16, 17, 'fizz', 19, 'buzz', 'fizz', 22, 23, 'fizz', 'buzz', 26, 'fizz', 28, 29, 'fizzbuzz', 31, 32, 'fizz', 34, 'buzz', 'fizz', 37, 38, 'fizz', 'buzz', 41, 'fizz', 43, 44, 'fizzbuzz', 46, 47, 'fizz', 49, 'buzz', 'fizz', 52, 53, 'fizz', 'buzz', 56, 'fizz', 58, 59, 'fizzbuzz', 61, 62, 'fizz', 64, 'buzz', 'fizz', 67, 68, 'fizz', 'buzz', 71, 'fizz', 73, 74, 'fizzbuzz', 76, 77, 'fizz', 79, 'buzz', 'fizz', 82, 83, 'fizz', 'buzz', 86, 'fizz', 88, 89, 'fizzbuzz', 91, 92, 'fizz', 94, 'buzz', 'fizz', 97, 98, 'fizz', 'buzz']

def test_fizz_buzz_using_boolean_has_correct_items():

    actual = fizz_buzz_using_boolean()

    assert actual == FIZZBUZZ_EXPECTED_ANSWER

Or my least favourite to use a literal in each test (which will lead to lots of duplicatation)
def test_fizz_buzz_using_boolean_has_correct_items():

    actual = fizz_buzz_using_boolean()

    assert actual == [1, 2, 'fizz', 4, 'buzz', 'fizz', 7, 8, 'fizz', 'buzz', 11, 'fizz', 13, 14, 'fizzbuzz', 16, 17, 'fizz', 19, 'buzz', 'fizz', 22, 23, 'fizz', 'buzz', 26, 'fizz', 28, 29, 'fizzbuzz', 31, 32, 'fizz', 34, 'buzz', 'fizz', 37, 38, 'fizz', 'buzz', 41, 'fizz', 43, 44, 'fizzbuzz', 46, 47, 'fizz', 49, 'buzz', 'fizz', 52, 53, 'fizz', 'buzz', 56, 'fizz', 58, 59, 'fizzbuzz', 61, 62, 'fizz', 64, 'buzz', 'fizz', 67, 68, 'fizz', 'buzz', 71, 'fizz', 73, 74, 'fizzbuzz', 76, 77, 'fizz', 79, 'buzz', 'fizz', 82, 83, 'fizz', 'buzz', 86, 'fizz', 88, 89, 'fizzbuzz', 91, 92, 'fizz', 94, 'buzz', 'fizz', 97, 98, 'fizz', 'buzz']

Which method should I use?

I don't do much python, but I do find your use of a fixture to return the answer a little bizarre.  For me, having the actual string in the test would be the way to go.  As I understand it, what you don't like about doing this is that you end up with multiple tests, all of which are calling different implementations and then verifying that the implementation returns the same thing.  This sounds like something you might want to use a parameterized test for.  Rather than passing in the changing data though (since it's the same), you'd parameterize the method under test:
@pytest.mark.parametrize("fizzbuzzer",
    [fizz_buzz_if_calc, fizz_buzz_using_boolean])
def test_fizz_buzz_has_correct_items(fizzbuzzer):

    actual = fizzbuzzer()

    assert actual == [1, 2, 'fizz', 4, 'buzz', 'fizz', 7, 8, 'fizz', 'buzz', 11, 'fizz', 13, 14, 'fizzbuzz', 16, 17, 'fizz', 19, 'buzz', 'fizz', 22, 23, 'fizz', 'buzz', 26, 'fizz', 28, 29, 'fizzbuzz', 31, 32, 'fizz', 34, 'buzz', 'fizz', 37, 38, 'fizz', 'buzz', 41, 'fizz', 43, 44, 'fizzbuzz', 46, 47, 'fizz', 49, 'buzz', 'fizz', 52, 53, 'fizz', 'buzz', 56, 'fizz', 58, 59, 'fizzbuzz', 61, 62, 'fizz', 64, 'buzz', 'fizz', 67, 68, 'fizz', 'buzz', 71, 'fizz', 73, 74, 'fizzbuzz', 76, 77, 'fizz', 79, 'buzz', 'fizz', 82, 83, 'fizz', 'buzz', 86, 'fizz', 88, 89, 'fizzbuzz', 91, 92, 'fizz', 94, 'buzz', 'fizz', 97, 98, 'fizz', 'buzz']

If the test fails, then you get an indication as to which failed as part of the test name and the follow up information:
________________________________ test_fizz_buzz_has_correct_items[fizz_buzz_if_calc] _________________________________

fizzbuzzer = <function fizz_buzz_if_calc at 0x7f2ff2bbab80>


Stack implementation using a queue I've implemented a stack using a queue.

Time complexity:

\$O(n)\$ - push(), where n is the number of elements in the queue

\$O(1)\$ - pop()

Space complexity:

\$O(n)\$ - n is the number of elements in the queue.

I'm looking for optimizations, code review and best practices.

public class StackUsingQueue<T> {

    private Queue<T> queue1;
    private Queue<T> queue2;

    public StackUsingQueue() {
        queue1 = new LinkedList<T>();
        queue2 = new LinkedList<T>();
    }

    public synchronized void push(T x) {
        queue2.add(x);
        queue2.addAll(queue1);
        // swapping references rather than copying from q2 into q1.
        queue1 = queue2;
        queue2 = new LinkedList<T>();
    }

    public synchronized T pop() {
        return queue1.poll();
    }

    public static void main(String[] args) {
        StackUsingQueue<Integer> isuq = new StackUsingQueue<Integer>();
        isuq.push(10);
        isuq.push(20);
        isuq.push(30);

        System.out.println(isuq.pop());
        System.out.println(isuq.pop());
        System.out.println(isuq.pop());
    }
}

Why? Why use a Queue to implement a stack? A Queue, by definition, (Computer Science Definition, not Java interface) is a FIFO device. A Stack, by definition, is a LIFO device. They are significant opposites of each other. Doing it this way is .... silly.

Also, it is not using the features of a FIFO queue to do the work, it is using the features of a Collection (add, addAll, Iterable). The only methods you should be calling on the queue are add, and remove.

A native array with a size would be a much, much better implementation, and would have \$O(1)\$ add and remove complexity, with occasional \$O(n)\$ if a resize is needed.

Also, your code is buggy, three things:

You 'cheat' and use poll() for the pop, and poll() returns null if the queue is empty, but, you do not 'guard' against the null being used as a push(null) value. Thus, there is no way to test whether the stack is empty reliably.
You do not implement peek()
You leak the monitor. By using synchronized methods, you let other people control your locking system. If people want to lock their code on something, they may use your stack, and then your synchronization is not contained.

I don't like this question of yours. To me it looks like a step backwards compared to your other recent questions.

Edit: About Locking

Consider some user of your class, they want a stack that is thread safe.... and they need to coordinate between some threads.

They need a convenient object to synchronize on. STACK is there... so they use it.

Note, I would never normally write code like the following.... but, the following code will never finish... Even though, at face value, it looks fine? You understand now?

private static final ExecutorService THREAD_POOL = Executors.newFixedThreadPool(10);

private static final StackUsingQueue<File> STACK = new StackUsingQueue<File>();

private static final void myMethodProcessesFilesInParallel(File dir) {

    // this method processes Files in a folder.
    // only allow one folder to be processed at any one time. Synchronize on something:
    synchronized(STACK) {
        List<File> files = dir.listFiles();
        final AtomicInteger counter = files.size();

        for (final File file : files) {
            THREAD_POOL.submit(new Runnable() {
                public void run() {
                    // STACK is thread-safe. pop() is good!
                    try {
                        File file = STACK.pop();
                        System.out.println(file.length());
                    } finally {
                        synchronized(counter) {
                            counter.decrementAndGet();
                            counter.notifyAll();
                        }
                    }
                }
            });
        }

        synchronized (counter) {
            while (counter.get() > 0) {
                counter.wait();
            }
        }
    }
}


Simplifying dictionary of constant values I have the following code:

    public string GetRefStat(string pk) {
        return RefStat[pk.Substring(2, 2)];
    }
    private readonly Dictionary<String, int> RefStat =
    new Dictionary<string, int>
    {
        {"00", REF.MenuType,       } // Menu
        {"01", REF.ReferenceStatus,} // Article
        {"02", REF.ReferenceStatus,} // Favorites List
        {"03", REF.ReferenceStatus,} // Content Block
        {"06", REF.ReferenceStatus } // Topic
    };

GetRefStat and the dictionary are always used together.

Is there a way I could simplify and combine these? I was wondering if I could put the information in a static class and then have a get method that returned the information I needed.

Your code is short, readable and efficient, so I don't think it needs much improvement. And I don't think adding a static class would give you anything (although changing the field and the method to static could make sense).

Another way to write your code would be to use switch, but I'm not sure whether it's more readable (although it could be slightly more efficient):

public string GetRefStat(string pk)
{
    switch (pk.Substring(2, 2))
    {
    case "00":
        return REF.MenuType;
    case "01":
    case "02":
    case "03":
    case "06":
        return REF.ReferenceStatus;
    default:
        throw new InvalidOperationException();
    }
}


Running AWS commands works ok, but the code seems messy and I don't like the time.sleep bit This is what I've come up with for a class that handles parsing AWS Textract output. So far it only gets the AWS Textract output into a .txt file.

import json
import logging as log
import os
import sys
import subprocess
import time

log.basicConfig(
  format='%(asctime)s [%(levelname)s]: %(message)s',
  datefmt='%Y-%m-%d %H:%M:%S',
  level=log.DEBUG,
  handlers=[
    log.StreamHandler(),
    log.FileHandler('{}.log'.format(__name__))
  ]
)

class InitialParser:

  def __init__(self, filename, s3_bucket, version=1):
    self.filename = filename
    self.s3_bucket = s3_bucket
    self.version = version

    self.file = None
    self.filepath = None
    self.data = None
    self.output_filename = self.filename.replace('.pdf', '.txt')

  def run(self):
    log.info('Running initial parser on file {}'.format(self.filename))
    if self.run_textract():
      log.info('Textract process successful')
    else:
      log.info('Textract process failed, exiting')
      sys.exit(0)

  def load_file_data(self):
    self.filepath = os.path.join(os.getcwd(), '../extracted/{}'.format(filename))
    try:
      with open(self.filepath, 'r') as file:
        self.file = file
        self.data = json.load(file)
    except FileNotFoundError as e:
      log.error('Could not find parsing file: {}'.format(e))

  def run_textract(self):
    """
    Runs AWS Textract commands on the given document and sends the output
    to a .txt file.

    @param s3_bucket: the S3 Bucket where the document is located.
    """
    job_id = ''
    # note: adding "Version":<str> to the AWS object below breaks the command
    aws_object = json.dumps({"S3Object":{"Bucket":self.s3_bucket,"Name":self.filename}}).replace(' ', '')  # can't have any spaces lol picky AWS CLI

    start_textract_command = "aws textract start-document-text-detection --document-location '{}'".format(aws_object)
    get_textract_output_command = 'aws textract get-document-text-detection --job-id '

    try:
      job_id = '"{}"'.format(json.loads(subprocess.check_output([start_textract_command], shell=True, stderr=subprocess.STDOUT).decode('utf-8'))['JobId'])
    except subprocess.CalledProcessError as e:
      if 'InvalidS3ObjectException' in e.output.decode('utf-8'):
        log.error('InvalidS3ObjectException (could not fetch object metadata from S3).\n Check the document name, AWS CLI configuration region (run `aws configure list`), permissions, and the S3 Bucket name & region.')
      elif 'ProvisionedThroughputExceededException' in e.output.decode('utf-8'):
        log.error('ProvisionedThroughputExceededException (provisioned rate exceeded). You\'re doing that too much.')
      else:
        log.error('Starting Textract failed. Error: {}'.format(e.output.decode('utf-8')))

    time.sleep(10)  # wait for Textract to do its' thing

    if job_id != '':
      try:
        subprocess.call(['touch {}'.format(self.output_filename)], shell=True)
        subprocess.call(['{} > {}'.format(get_textract_output_command+job_id, self.output_filename)], shell=True, stderr=subprocess.STDOUT)
        return True
      except subprocess.CalledProcessError as e:
        log.error(e.output)
    else:
      return False

if __name__ == '__main__':
  initial_parser = InitialParser(
    filename='test1.pdf',
    s3_bucket='test-bucket',
  )
  initial_parser.run()

I'd like a better way of handling the wait period for Textract to finish other than waiting 10 seconds, and it may take longer than that in some instances.

I do know that the output file will have "JobStatus": "IN PROGRESS" so I could repeatedly search the output .txt file for "SUCCEEDED", but that could be slow if the file has 40,000+ lines when it's actually done (it will)

aws_object = json.dumps({"S3Object":{"Bucket":self.s3_bucket,"Name":self.filename}}).replace(' ', '')  # can't have any spaces lol picky AWS CLI
start_textract_command = "aws textract start-document-text-detection --document-location '{}'".format(aws_object)

This is not AWS which is picky, rather you’re not building the command line properly. If you provide a single string and start your subprocess using shell=True, each space will mark a new argument. Instead, you’d be better off building a list of arguments and let subprocess quote them properly:

start_textract_command = ['aws', 'textract', 'start-document-text-detection', '--document-location', json.dumps(…)]

and then just run subprocess.check_output(start_textract_command, stderr=subprocess.STDOUT).

Speaking of which, instead of using the older high level API, you should switch to subprocess.run:

process = subprocess.run(start_textract_command, capture_output=True, encoding='utf-8')
try:
    process.check_returncode()
except CalledProcessError:
    if 'InvalidS3ObjectException' in process.stderr:
       …
else:
    job_id = json.loads(process.stdout)['JobId']
    with open(self.output_filename, 'wb') as output_file:
       process = subprocess.run(['aws', 'textract', 'get-document-text-detection', '--job-id', str(job_id)], stdout=output_file, stderr=subprocess.PIPE)
    if process.returncode:
        log.error(process.stderr.decode('utf-8'))

Now for the time.sleep part, AWS Textract provides two modes of operations: synchronous and asynchronous. You can start by using the synchronous detect-document-text operation if it fits your need and you won't have to deal with the timing at all.

Otherwise, if you need to stick to start-document-text-detection, the completion of the process is published as a notification. I’m not at all versed in how SNS works but they have a tutorial to get you started on using Amazon SNS so you can create a channel, specify it in your textract job and wait for the completion event.


Plotting the integer values in text format in matplotlib piechart I have done a clustering algorithm and represented the results in a pie chart as shown below.

fig, ax = plt.subplots(figsize=(20, 10), subplot_kw=dict(aspect="equal"))
contents  = []

for k,v  in clusters.items():
    indi= str(len(clusters[k])) + " users " +  "Cluster_"+ str(k)
    contents.append(indi)

#contents = ['23 users Cluster_0', '21 users Cluster_1']

data = [float(x.split()[0]) for x in contents]
Cluster= [x.split()[-1] for x in contents]

def func(pct, allvals):
    absolute = int(pct/100.*np.sum(allvals))
    return "{:.0f}%\n({:d} users)".format(pct, absolute)

wedges, texts, autotexts = ax.pie(data, autopct=lambda pct: func(pct, data),
                                  textprops=dict(color="w"))

ax.legend(wedges, Cluster,
          title="CLuster",
          loc="center left",
          bbox_to_anchor=(1, 0, 0.5, 1))

plt.setp(autotexts, size=10, weight="bold")

ax.set_title("Distribution of users: A pie chart")

Even though the users are 23 and 21 in each cluster, the piechart shows 22 and 20.
This is due to the conversion to int and some float values are cut off in the function def func().

But, to fix this I wrote the below code and it works:

def func(percentage, allvals):
    absolute = int(np.sum(allvals))
    newV = (percentage/100)*absolute
    roundnewV = round(newV)
    intnewV = int(roundnewV)
    return "{:.0f}%\n({:d} users)".format(percentage, intnewV)

Is this a good way to save the original form of integer and not lose out any value?

You got yourself in trouble by using plt.pie, and especially the keyword argument autopct, beyond its intended use.

The basic idea of the pie chart is to have the wedge labels outside the pie and perhaps percentages inside.

You wanted the wedge label and percentage inside the pie and manipulated the autopct keyword with a function to achieve this. This involved cumbersome calculations from percentages to values you already know.

Another solution could be to use the more simple labels keyword argument and change the resulting texts properties to be inside the pie instead outside, see code changes below:

contents = ['23 users Cluster_0', '21 users Cluster_1']

data = [int(x.split()[0]) for x in contents]

def pie_chart_labels(data):
    total = int(np.sum(data))
    percentages = [100.0 * x / total for x in data]
    fmt_str = "{:.0f}%\n({:d} users)"
    return [fmt_str.format(p,i) for p,i in zip(percentages, data)]

wedges, texts,  = ax.pie(data, labels=pie_chart_labels(data))

# shrink label positions to be inside the pie
for t in texts:
    x,y = t.get_position()
    t.set_x(0.5 * x)
    t.set_y(0.5 * y)

plt.setp(texts, size=10, weight="bold", color="w", ha='center')


POJO-like class in Python I simply want to organize some data (class= MarketOnCloseSecurity) to be used in another class MarketOnClosePortfolio. The former class would not have any methods but just a constructor that includes a ticker for a company symbol and two Pandas dataframes bars and signals.

class MarketOnCloseSecurity():
    """Encapsulates the notion of a portfolio of positions based
    on a set of signals as provided by a Strategy.

    Requires:
    symbol - A stock symbol which forms the basis of the portfolio.
    bars - A DataFrame of bars for a symbol set.
    signals - A pandas DataFrame of signals (1, 0, -1) for each symbol.
   """

    def __init__(self, symbol, bars, signals):
        self.symbol = symbol
        self.bars = bars
        self.signals = signals

Now the MarketOnClosePortfolio would have all the methods to calculate a portfolio.

Is creating a class with essentially no methods (besides constructor) in python a good practice?

The idea is to pass MarketOnClosePortfolio a list of MarketOnCloseSecurity objects.

An alternative (non-OOP) solution would do something like list_of_MOCS = [{'symbol_1':(bars_1,signals_1)},...,{'symbol_N':(bars_N,signals_N)}] so basically a list containing a map with the key being the ticker and value being a tuple of the two dataframes.

MarketOnClosePortfolio will identify the positions (buy/sell) and how based on each MarketOnCloseSecurity and form a Portfolio Dataframe. It will then calculate metrics, such as holdings, cash, and returns based on the summation of each MarketOnCloseSecurity. The code is not complete, but I have included it for reference.

class MarketOnClosePortfolio(Portfolio):
    """Encapsulates the notion of a portfolio of positions based
    on a set of signals as provided by a Strategy.

    Requires:
    market_on_close_securities
    initial_capital - The amount in cash at the start of the portfolio."""

    def __init__(self, market_on_close_securities, initial_capital=100000.0):
        self.market_on_close_securities = market_on_close_securities
        self.initial_capital = float(initial_capital)
        self.positions = self.generate_positions()

    def generate_positions(self):
        #loop through each MOCS (market_on_close_security) to calculate positions

        # Initialize index for positions Dataframe. Picking index 0 was arbitary they all have same index length
        positions = pd.DataFrame(index=self.market_on_close_securities[0].signals.index).fillna(0.0)
        for security in self.market_on_close_securities:
            positions[security.symbol] = 100 * security.signals['signal'] # This strategy buys 100 shares

        return positions

    def backtest_portfolio(self):

        portfolio_series = self.positions[self.symbol] * self.bars[0]['close_price'].astype(float)

        pos_diff = self.positions.diff()

        # Will need to use this for multiple columns (Eventually)
        symbol_columns = [self.symbol]

        portfolio = pd.DataFrame(index=self.bars[0].index, columns=symbol_columns)
        portfolio[self.symbol] = portfolio_series.values

        # Sum holdings from each company
        portfolio['holdings'] = portfolio.sum(axis=1)
        portfolio['cash'] = self.initial_capital - (pos_diff[self.symbol] * self.bars[0]['close_price'].astype(float)).cumsum()
        portfolio['cash'][0] = self.initial_capital
        portfolio['total'] = portfolio['cash'] + portfolio['holdings']
        portfolio['returns'] = portfolio['total'].pct_change()

        return portfolio

The backtest_portfolio() will determine the total returns/holdings based on each MarketOnCloseSecurity. This implementation is not complete, and is currently the method used when MarketOnClosePortfolio was adapted for only one security.

When reading

  An alternative (non-OOP) solution would do something like list_of_MOCS = [{'symbol_1':(bars_1,signals_1)},...,{'symbol_N':(bars_N,signals_N)}] so basically a list containing a map with the key being the ticker and value being a tuple of the two dataframes.

I thought, that a better alternative would be:

MOCS = {
    'symbol_1': (bars_1, signals_1),
    'symbol_2': (bars_2, signals_2),
    ...
    'symbol_N': (bars_N, signals_N),
}

Especially given the fact that pd.DataFrame(MOCS) would yield a valid structure.

However, reading at your usage in generate_positions and expected usage in backtest_portfolio; and especially the fact that you will need to explode the data-structure anyway, I would keep the class approach and use collections.namedtuple as largely suggested in the comments. It provides both the advantages of a class (attributes access) and a tuple (immutable + index access).

I would however change generate_positions to create a dictionnary that will hold the structure of the resulting dataframe before building said dataframe. And incorporate that method directly into the constructor as it does not provide much added value on its own:

from collections import namedtuple

MarketOnCloseSecurity = namedtuple('MarketOnCloseSecurity', 'symbol bars signals')

class MarketOnClosePortfolio(Portfolio):
    """Encapsulates the notion of a portfolio of positions based
    on a set of signals as provided by a Strategy.

    Requires:
    market_on_close_securities
    initial_capital - The amount in cash at the start of the portfolio."""

    def __init__(self, market_on_close_securities, initial_capital=100000.0):
        self.market_on_close_securities = market_on_close_securities
        self.initial_capital = float(initial_capital)

        #loop through each MOCS (market_on_close_security) to calculate positions
        securities = {
            security.symbol: 100 * security.signals['signal'] # This strategy buys 100 shares
            for security in market_on_close_securities
        }
        self.positions = pd.DataFrame(securities)

    def backtest_portfolio(self):
        ...

I would also extend the namedtuple to provide extra utilities methods:

class MarketOnCloseSecurity(namedtuple('MarketOnCloseSecurity', 'symbol bars signals')):
    def buy_shares(self, amount):
        return amount * self.signals['signal']

    def close_price(self):
        return self.bars['close_price'].astype(float)

So that you can have more meaningful actions in MarketOnClosePortfolio:

class MarketOnClosePortfolio(Portfolio):
    """Encapsulates the notion of a portfolio of positions based
    on a set of signals as provided by a Strategy.

    Requires:
    market_on_close_securities
    initial_capital - The amount in cash at the start of the portfolio."""

    def __init__(self, market_on_close_securities, initial_capital=100000.0):
        self.market_on_close_securities = market_on_close_securities
        self.initial_capital = float(initial_capital)

        securities = {
            security.symbol: security.buy_shares(100)
            for security in market_on_close_securities
        }
        self.positions = pd.DataFrame(securities)

    def backtest_portfolio(self):
        # use security.close_price() for any security in self.marke_on_close_securities if you need to
        ...


Value store ("optional" type) Create a class template named ValueStore that has a member variable,
to store a value and a member variable, hasValue that holds a true
value if there is data in the value member variable and false
otherwise. Use the class to store three values of different types.

#include <iostream>
using namespace std;

template <typename T>
class ValueStore {
private:
  T value;

public:
  bool hasValue = false;

  ValueStore(T val) {
    value = val;
    hasValue = true;
  }

  ValueStore() {
    value = {};
  }

  T getValue() {
    return value;
  }

};

int main ()
{
  ValueStore<string> v1("hello, world");
  if (v1.hasValue) {
    cout << "The value stored is: " << v1.getValue() << endl;
  }
  else {
    cout << "v1 has no value." << endl;
  }
  ValueStore<int>v2(1000);
  if (v2.hasValue) {
    cout << "The value stored is: " << v2.getValue() << endl;
  }
  else {
    cout << "v2 has no value." << endl;
  }
  ValueStore<double> v3;
  if (v3.hasValue) {
    cout << "The value stored is: " << v2.getValue() << endl;
  }
  else {
    cout << "v3 has no value." << endl;
  }
  return 0;
}

About these assignments
As someone who’s been teaching C++ for 15 holy shit, almost 20 years now, I have to say that these assignments are pretty terrible… especially the “vector” one. (To be clear, I mean the assignments are terrible… not the attempts. The attempts aren’t bad at all.)
I’ve always found the logic of “let’s teach by reimplementing what’s in the standard library” to beginners to be a little idiotic. As far as teaching useful skills goes:

it just wastes the learner’s time learning how to do something they (almost certainly) will never need to do again; and
in the vast majority of cases, it requires such terrifyingly advanced C++ knowledge to properly replicate a standard library facility, that in order to make it doable at all you have to dumb it down to the point where it’s almost broken… which only gives the learner a false sense of confidence thinking they understand how to do the thing when they really don’t.

In this case, it looks like the goal is to reimplement std::optional and std::vector… but only half-assed, broken, functionally-useless variants of both. I see beginners attempting these things all the time, but what they don’t seem to understand is that those are INCREDIBLY difficult tasks, requiring knowledge of C++ that is WAY beyond what any beginner should be expected to have. Even some expert level C++ programmers don’t have the knowledge necessary to do these things. In particular, reimplementing std::vector properly is among the hardest things you can possibly do in C++. Just because it’s easy to use doesn’t mean it’s easy to make.
I wish it would become more commonly understood that reimplementing standard library facilities is a pants-crappingly stupid way of teaching C++. Reimplementing standard library facilities is an excellent project… for highly advanced C++ programmers. But beginners? No. Don’t teach beginners to reimplement the standard library… teach them to fricken’ use it!
Anywho, the problem here isn’t the learner—it’s the teacher who gave these crappy assignments—so I won’t rant any more about that. Instead I’ll just focus on the code.
I’m only going to review the first program here. The second program, I’ll review in a second reply. It looks like the recommendation is to split the two programs into two separate questions; so you should probably do that.
Code review
using namespace std;

Never, ever do this at file scope. This is a terrible practice. In fact, you should probably never do this ever, not even at function scope.
Among the many reasons this is bad: Today your code compiles because there is nothing called ValueStore in the standard library. But if some future version of the standard adds something with that name… boom, your code is now broken. And that’s if you’re lucky. What might also happen is that something new gets added to the standard library that silently changes the behaviour of your code. That’s a nightmare scenario.
Just use std:: where necessary.
  ValueStore(T val) {
    value = val;
    hasValue = true;
  }

The first issue with this constructor is that it’s not explicit. That makes it dangerous, because it will silently convert anything that is implicitly convertible to a T. At the very least, that is likely to be inefficient.
In general, any constructor that takes a single argument should be explicit. (Yes, that should actually be the default, but unfortunately, the default was set to implicit way back when C++ was first standardized, and now it’s too late to change.)
The second issue with this constructor is that it doesn’t use member initializer lists. What’s actually happening above is this:
ValueStore(T val)
:
    value{},        // value is default-constructed
    hasValue{false} // hasValue is initialized to false (because of the member initializer where it is declared)
{
    value = val;        // value is copy-assigned using val
    hasValue = true;    // hasValue is move-assigned using true
}

As you can see, value is set up twice. First it is default constructed, then it is assigned (with the actual contents of val).
What you should do, as a start, is this:
explicit ValueStore(T val)
:
    value{val},     // value is copy-constructed using val
    hasValue{true}  // hasValue is initialized to true
{
    // nothing to do here
}

Here you can see value is only set up once: it is initialized right from the start with val, rather than being initialized “empty” (technically default-constructed) then assigned.
But there’s one more improvement you could make. To understand it, consider the following code:
ValueStore<std::string> v1("hello, world");

What’s happening here is:

Before the ValueStore constructor is called:

A temporary std::string is constructed with the contents “hello, world”.
The temporary string is moved to val. (It is moved, not copied, because it is a temporary. As a practical matter, this whole step is usually elided as an optimization, meaning the temporary string is constructed directly into val.)

The ValueStore constructor is called:

The member initializer list runs:

val is copied into value.
hasValue is set to true.

The constructor body is called:

Nothing happens here, because it’s empty.

So a temporary string is constructed, and then moved into val, and then copied into value. The copy is the problem there, because copying a string is expensive. Copying is generally expensive compared to moving, so you want to move whenever possible.
This is an easy fix; just move val into value:
explicit ValueStore(T val)
:
    value{std::move(val)},  // value is move-constructed using val
    hasValue{true}          // hasValue is initialized to true
{
    // nothing to do here
}

That’s all you need to do. Now a temporary string is constructed, then moved into val, then moved into value. In all likelihood, those moves can be elided. But even if not, moves are (usually) fast, especially for strings, and are usually no-fail.
There are a few other improvements you could make to this constructor:
constexpr explicit ValueStore(T val)
    noexcept(std::is_nothrow_move_constructible_v<T>)
:
    value{std::move(val)},
    hasValue{true}
{}

Making the constructor constexpr means you can construct ValueStore objects at compile-time, which can be enormously useful.
If move-constructing value cannot possibly fail, then the whole constructor cannot possibly fail. Adding the noexcept tells the compiler that, allowing it to make additional optimizations. (If move-constructing value can fail, then std::is_nothrow_move_constructible_v<T> will be false.)
  ValueStore() {
    value = {};
  }

This has the same problem as the previous constructor: you’re setting the value of value twice. What’s really happening is this:
ValueStore()
:
    value{},        // value is default-constructed
    hasValue{false} // hasValue is initialized to false
{
    value = {}; // a temporary T object is default-constructed,
                // then move-assigned into value
}

As you can see, that entire value = {}; is just a complete waste of time. All it does is take the already default-constructed value, default-constructs a whole new object, and then moves that into value. If you’d just left value alone, it would already be default-constructed:
ValueStore()
/*
:
    value{},        // value is default-constructed
    hasValue{false} // hasValue is initialized to false
*/
{
    // nothing to do here
}

But wait, there’s more!
Writing the default constructor like ValueStore() {} is actually a bad idea, because if it were possible to trivially-construct ValueStore objects… you just broke that functionality. Trivial construction means the compiler doesn’t actually have to do anything to construct a ValueStore, which means a lot of potential optimizations. It is better to leave the default constructor alone unless you actually need it to do something specific… which you don’t in this case.
So what you should write is:
ValueStore() = default;

And of course, you can add the extra decorations mentioned for the previous constructor to get even more benefits:
constexpr ValueStore() noexcept(std::is_nothrow_default_constructible_v<T>) = default;

(is_nothrow_default_constructible_v<T> is true only if default-constructing a T cannot fail.)
  T getValue() {
    return value;
  }

Since this function doesn’t change the state of the object, it should be const.
You could also add constexpr and maybe noexcept(std::is_nothrow_copy_constructible_v<T> and std::is_nothrow_copy_assignable_v<T>).
Now, this is really getting into complicated territory… but as the default choice for a new API, doing what you’ve done—returning a copy of value—is probably the right choice. However… this is not optimal, because if you don’t actually want a copy of value—if you just want to see what that value is (for example, just to print it like you’re doing in main()—you’re wasting resources by creating the copy.
What you really want to do is return a reference to value… but you have to be careful what kind of reference you return. If the ValueStore object is const, you don’t want to return a non-const reference to value… that would (theoretically) allow users to change the value of your const object, which is absurd. So you need two functions: one for const objects, and one for non-const objects:
constexpr T const& getValue() const noexcept { return value; }
constexpr T&       getValue()       noexcept { return value; }

Oh, but we’re not done yet, not by a long shot. Because there’s still the problem of temporaries. If you have a temporary ValueStore object, and you call getValue() on it, you’ll get a reference to value… but that reference will be dangling because the temporary is destroyed.
So you need to return rvalue references to value when the ValueStore is an rvalue (temporary, basically), and lvalue references when the ValueStore is an lvalue… and you need to take const into account. So what you really need is:
constexpr T const&  getValue() const&  noexcept { return value; }
constexpr T&        getValue() &       noexcept { return value; }
constexpr T const&& getValue() const&& noexcept { return std::move(value); }
constexpr T&&       getValue() &&      noexcept { return std::move(value); }

(Yes, you have to basically write the same function 2–4 times. That’s an unfortunate situation that some people are currently working to fix.)
((Rant mode again: See, complicated crap like this is why reimplementing std::optional is such a terrible project for beginners. And we haven’t even scratched the surface of just how complicated this really gets! You see, your class actually lies: it says hasValue is false if you do ValueStore<double> v3; But v3 does have a value: it is holding a double whose value is 0.0. Zero is a perfectly valid value for a double. Similarly, if you do ValueStore<std::string> v;, v.hasValue will say false… but v does have a value… it is holding a string whose current contents are just “”, but an empty string is still a string.))
That’s pretty much it for the ValueStore class. The only other comments I have are for the main() function.
First: Don’t use std::endl. Just don’t. Ever. Pretty much every use of std::endl I’ve ever seen is wrong… and yes, every single use in your program is wrong.
In particular, this is absurd:
    cout << "v1 has no value." << endl;

If you want a newline, just use a newline:
    std::cout << "v1 has no value.\n";

I know, every tutorial you’ve ever seen probably uses std::endl. Yes, I’m saying they’re all wrong. There is almost never a right time to use std::endl… and for those very, very rare situations where it has a purpose, you’re probably better off calling std::cout.flush() directly anyway, to make clear that’s what you want.
Finally: there is no need for return 0; in main(). main() automatically returns zero. (This is only true for main(); main() is special in a lot of ways.)
Summary
The problem I have with deciding whether this is a solid attempt at this problem or not is that the problem itself is stupid. Given a stupid problem, if someone’s solution isn’t great, is that because the solution is bad or because the problem is bad? It’s not easy to say.
I suppose this is a decent swing at the problem for a beginner. But really, this is not a beginner problem. Not if you take it seriously, anyway.
So long as you understand that hasValue is a lie, I suppose you can walk away from this having learned something. The truth is that your ValueStore always has a value. It can’t not have a value, so long as it has a T data member. Doing value = {} doesn’t set value to “not a value”; it sets it to the default value. Which is a value. There is literally no way to set value to “not a value”; that is literally impossible in C++.
You could cheat a little and do something like this:
template <typename T>
class ValueStore
{
private:
    std::optional<T> value;

public:
    bool hasValue = false;

    constexpr explicit ValueStore(T val)
    :
        value{std::move(val)},
        hasValue{true}
    {}

    constexpr ValueStore() noexcept = default;

    constexpr T const&  getValue() const&  noexcept { return value.value(); }
    constexpr T&        getValue() &       noexcept { return value.value(); }
    constexpr T const&& getValue() const&& noexcept { return std::move(value).value(); }
    constexpr T&&       getValue() &&      noexcept { return std::move(value).value(); }
};

Now ValueStore can have no value, because it is no longer holding a T, it’s holding a std::optional<T>… but now hasValue is redundant: it’s just duplicating value.has_value(). You could just do:
template <typename T>
class ValueStore
{
private:
    std::optional<T> value;

public:
    constexpr explicit ValueStore(T val) : value{std::move(val)} {}

    constexpr ValueStore() noexcept = default;

    constexpr T const&  getValue() const&  noexcept { return value.value(); }
    constexpr T&        getValue() &       noexcept { return value.value(); }
    constexpr T const&& getValue() const&& noexcept { return std::move(value).value(); }
    constexpr T&&       getValue() &&      noexcept { return std::move(value).value(); }

    constexpr bool hasValue() const noexcept { return value.has_value(); }
};

Anywho, back to your code: As I said, I think it’s a decent attempt at a very, very hard problem. In particular, your code is correct, for the vast majority of use cases… it’s just not efficient. Which is not a bad start! It’s always good to get it right first, and get it fast later, if possible.


Solving the 'Vietnamese school maths problem' Having read Can you do the maths puzzle for Vietnamese eight-year-olds that has stumped parents and teachers?, which presented the problem of filling in numbers 1 to 9 to satisfy

$$ a + 13 \frac{b}{c} + d + 12 e - f - 11 + \frac{g \cdot h}{i} - 10 = 66 $$

… I thought I'd try a Common Lisp implementation as a learning exercise.

(defun solve-puzzle (permutation)
  (destructuring-bind (a b c d e f g h i) permutation
    (let ((result (infpre:!! a + 13 * b / c + d + 12 * e - f - 11 + g * h / i - 10)))
      (when (= result 66) (print permutation)))))

(alexandria:map-permutations #'solve-puzzle '(1 2 3 4 5 6 7 8 9) :length 9)

This works; when executed, it produces a list of 136 valid combinations in around 300ms on my laptop.  I have a couple of questions regarding the code though:

How's my idiom?  My background is C, C#, Ruby so I'm probably carrying over habits from those languages.
Are there any obvious performance gotchas in the code - obvious to experienced Lispers, that is?

I'm not familiar with either the alexandria or infpre packages; I wrote my solution in Common Lisp as well, but using only language primitives. Assuming that the infpre evaluator follows standard arithmetic order-of-evaluation rules, your code looks perfectly fine (and more compact and readable than mine) but of course requires having those packages available. More importantly, your code should produce the same answers as my (different) approach.

I used my approach to make sure that the variables were local and that the compiled code manipulated them efficiently. (I ran mine in Clozure Common Lisp which compiles all expressions to machine code before running them).

;; Transcription of expression from puzzle picture
(defvar *expression* '(+ a (/ (* 13 b) c) d (* 12 e) (* -1 f) -11 (/ (* g h) i) -10))

;; Macro to generate nested loops trying all combinations
(defmacro nested-loops (variables variable-initializer body)
  (let ((internal-variable (gensym)))
    `(let ((answers))
       ,(nested-loops-helper t variables variable-initializer body internal-variable)
       answers)))

;; Helper function for macro to generate loop guts for each level
(defun nested-loops-helper (first-time variables variable-initializer body internal-variable)
  (cond ;; no more variables, generate body
    ((null variables)
     `(,@body))
    ;; otherwise generate a loop setting this variable to all possible values except ones already used
    (t (let* ((this-var (pop variables))
              (extra (when first-time `(for ,internal-variable = nil))))
         `(loop for ,this-var ,@variable-initializer ,@extra
            unless (member ,this-var ,internal-variable)
            do (push ,this-var ,internal-variable)
            (,@(nested-loops-helper nil variables variable-initializer body internal-variable))
            (pop ,internal-variable))))))

;; Invocation of macro to solve puzzle (prints all answers and saves them in solutions variable)
(length (setq solutions
          (nested-loops (a b c d e f g h i) (from 1 to 9)
                (let ((value #.*expression*))
                  (when (eql value 66)
                (print (list a b c d e f g h i))
                (push (list a b c d e f g h i) answers))))))

(format t "~&There are ~d solution~:p saved in the variable solutions.~%" (length solutions))


Passing Database connection I wanted to open Database (Mongo) connection only once when I start application and share the same connection across the application. What am doing as part of app.js is

Creating connection

var dbConnection = mongoose.createConnection(config.database.address, config.database.dbName, config.database.port);

passing dbConnection in every route call

require('./services/MasterServices')(app , dbConnection);

Is it right approach or anything wrong here?

You can use the connect function, which returns the default connection.

Just add mongoose.connect('mongodb://username:password@host:port/database'); whenever you need it.

See http://mongoosejs.com/docs/connections.html


Pick a two different random elements from a given closed range I wrote a function which picks a two different random elements from a closed  range of ints and returns them as a tuple.

I would like to take a your feedback, if the solution is a "Clever", "Best practice" or "non of them"

func pickTwoRandomElementsFromRange(_ range: ClosedRange<Int>) -> (Int, Int)? {
    guard range.count >= 2 else { return nil }

    let firstElement = range.randomElement()
    var secondElement = range.randomElement()

    while firstElement == secondElement  {
        secondElement = range.randomElement()
    }
    if let firstElement = firstElement, let secondElement = secondElement {
        return (firstElement, secondElement)
    }

    return nil
}

Naming

Swift naming conventions are listed in the API Design Guidelines. With respect to “Omit needless words” and “Strive for Fluent Usage” I would suggest to call the function

func randomPair(from range: ClosedRange<Int>) -> (Int, Int)?

Simplify the code

Your method is fine, but can be simplified a bit. First, the return values from randomElement() can safely be forced-unwrapped because it is guaranteed at that point that the range is not empty. That makes the final optional binding obsolete:

func randomPair(from range: ClosedRange<Int>) -> (Int, Int)? {
    guard range.count >= 2 else { return nil }

    let firstElement = range.randomElement()!
    var secondElement = range.randomElement()!

    while firstElement == secondElement {
        secondElement = range.randomElement()!
    }
    return (firstElement, secondElement)
}

Alternatively one can use a repeat-loop, so that the second element is assigned only at one place, that is a matter of personal preference:

func randomPair(from range: ClosedRange<Int>) -> (Int, Int)? {
    guard range.count >= 2 else { return nil }

    let firstElement = range.randomElement()!
    var secondElement: Int
    repeat {
        secondElement = range.randomElement()!
    } while firstElement == secondElement

    return (firstElement, secondElement)
}

But actually the same can be achieved without a loop, if we pick the second element from a reduced range and adjust it if necessary:

func randomPair(from range: ClosedRange<Int>) -> (Int, Int)? {
    guard range.count >= 2 else { return nil }

    let firstElement = range.randomElement()!
    var secondElement = range.dropLast().randomElement()!
    if secondElement >= firstElement {
        secondElement += 1
    }

    return  (firstElement, secondElement)
}

Generalizations

Now we have a function to get a random pair from a closed range. But what about open ranges? What about ranges of other types? In fact it is not difficult to implement the same algorithm for arbitrary (random access) collections. The only difference is that we pick random indices first:

func randomPair<C>(from collection: C) -> (C.Element, C.Element)? where C: RandomAccessCollection {
    guard collection.count >= 2 else { return nil }

    let firstIndex = collection.indices.randomElement()!
    var secondIndex = collection.indices.dropLast().randomElement()!

    if secondIndex >= firstIndex {
        collection.formIndex(after: &secondIndex)
    }

    return (collection[firstIndex], collection[secondIndex])
}

This covers closed ranges, open ranges, arrays etc:

randomPair(from: 0...10)
randomPair(from: 0..<10)
randomPair(from: ["a", "b", "c", "d"])

Alternatively one can implement this as an extension method for RandomAccessCollection so that it closely resembles the existing randomElement() method:

extension RandomAccessCollection {
    func randomPair() -> (Element, Element)? {
        guard count >= 2 else { return nil }

        let firstIndex = indices.randomElement()!
        var secondIndex = indices.dropLast().randomElement()!
        if secondIndex >= firstIndex {
            formIndex(after: &secondIndex)
        }
        return (self[firstIndex], self[secondIndex])
    }
}

(0...10).randomPair()
(0..<10).randomPair()
["a", "b", "c", "d"].randomPair()


Vigenere's Cipher in C I have updated the code and the program now works as it should. If you could critique my code that would be great. I am new to C so I guess I will have made some rookie error :)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <stdbool.h>

// Function to check if alphabet characters.
int alphaCheck(char * argv[]) {
  int length = strlen(argv[1]), n;
  for (n = 0; n < length; n++) {
    if (!isalpha(argv[1][n])) {
      printf("Characters 'A-Z' for Keyword.\n");
      return 2;
    }
  }
}

int main(int argc, char * keyWord[]) {
  int cipher[64], i, j, k;
  char message[128];
  int keyCount = 0;
  int p = strlen(keyWord[1]);

  // Validation - using alphaCheck Function
  if (argc != 2 || alphaCheck(keyWord) == 2) {
    return 0;
  }

  // For loop to convert to upper and set value ie A = 0 B = 1
  for (i = 0, j = strlen(keyWord[1]); i < j; i++) {
    cipher[i] = (toupper(keyWord[1][i]) - 65);
  }

  // Prompt the user for the message to encrypt
  printf("Enter your secret message: ");
  fgets(message, 128, stdin);

  for (i = 0, k = strlen(message); i < k; i++) {
    if (isupper(message[i])) {
      char c = (message[i] - 65 + cipher[keyCount])  % 26 + 65;
      printf("%c", c);
      keyCount++;
    }
    else if (islower(message[i])) {
      char d = (message[i] - 97 + cipher[keyCount]) % 26 + 97;
      printf("%c", d);
      keyCount++;
    }
    else {
      printf("%c\n", message[i]);
    }
    if (keyCount >= p) {
      keyCount = 0;
    }
  }
}

i++ or ++i?

Using i++ is fine here, but also read this discussion:

https://stackoverflow.com/questions/4261708/i-or-i-in-for-loops

fgets(message, 128, stdin);

How about fget(message, sizeof(message), stdin) -- makes it easier to change the size of the message buffer.

int main(int argc, char * keyWord[])

Most people are used to seeing:

int main(int argc, char* argv[])

int alphaCheck(char * argv[])

How about:

int alphaCheck(char *str) {
  for ( ;*str; ++str) {
    if (!isalpha(*str)) {
      ...
    }
  }
  return 0;   // You don't have this return.
}

First of all, this makes alphaCheck usable on any string, not just argv[1]. Also you avoid computing the length of the string. Moreover, this is the idiomatic way to iterate over a null-terminated string in C.

Finally - make sure you always return a value since you've declared this function returns an int.

for (i = 0, j = strlen(keyWord[1]); i < j; i++) {

You are modifying cipher[i] but are you checking that i < 64? What if keyWord[1] has length > 64?

keyWord[1]

You reference this array element many times in the program. Perhaps you should assign a meaningful variable name to it, e.g.:

char* secret = keyWord[1];

It will make you program more readable.

printf("%c\n", message[i]);

Do you want to always print a \n here? I would think you only want to print an extra \n at the end of the message.

the main loop

You main loop has this structure:

for each character c in the message:
  let e = encrypt c using the secret key
  printf("%c", e)
  advance pointers, counters, etc.
printf("\n")

so it would be nice if the code could match this as much as possible.

How about:

int i;
for (char *p = message, i = 0; *p; ++p, ++i) {
  char e = ...encrypt (*p) using secret, strlen(secret) and i...
  printf("%c", e);
}
printf("\n");

Now there is only one place where the encrypted character is printed out - instead of three difference places.

You can implement this via a helper function:

char encrypt(char* secret, int secret_len, char c, int i) {
  // return how c is encrypted when at position i in the message.
  if (isupper(c)) {
    return ...
  } else if (islower(c)) {
    return ...
  } else {
    return c;
  }
}

Hint: The expression i % secret_len might come in handy here.


Determining if an entity exists in a database via a stored procedure Here is a short and simple Ajax method that returns "True" or "False" if an entity exists in a database via a stored procedure that returns just "Y" or "N" (the details of this entity and database are not relevant to my question though).

This is the first time I've used the C# using() statement, and was wondering if anyone would be kind enough to review this and give me feedback.

[WebMethod]
public string ValidateEntity(string EntityType, string EntityName)
{
    string connstr = (from c in Companys where c.Name.Equals(company, StringComparison.OrdinalIgnoreCase) select c.ConnectionString).FirstOrDefault();
    if (connstr == null) { return "False"; }
    using (SqlConnection conn = new SqlConnection(connstr))
    {
        using (SqlDataAdapter da = new SqlDataAdapter())
        {
            using (da.SelectCommand = new SqlCommand("ValidateEntity", conn))
            {
                da.SelectCommand.CommandType = CommandType.StoredProcedure;
                da.SelectCommand.Parameters.AddWithValue("@EntityType", EntityType);
                da.SelectCommand.Parameters.AddWithValue("@EntityName", EntityName);
                using(DataSet ds = new DataSet())
                {
                    da.Fill(ds, "result_name");
                    DataTable dt = ds.Tables["result_name"];
                    if ( dt.Rows.Count > 0){
                        if (dt.Rows[0]["Valid"].ToString()=="Y") { return "True"; }
                    }
                }
            }
        }
    }
    return "False";
}

I may be pointing out the obvious here, but the ValidateEntity Method returns a string. I would expect it to return a Boolean.
There are a number of one and two letter variable names. We've not been restricted in the size of names for many years now. It would be good to give them more descriptive names and we can take advantage of C#'s case sensitivity.

da = adapter
ds = dataSet (but perhaps something even more meaningful)
dt = table or resultNames

If you return a bool instead of a string, you could directly return here instead of using an if statement.

if (dt.Rows[0]["Valid"].ToString()=="Y") { return "True"; }

Becomes

return (dt.Rows[0]["Valid"].ToString() == "Y");


Simple MVC - Displaying a Customer I would like to ask about possible improvement of this code. I have created very simple code using MVC pattern. Is there any logic or anything to change? I tried to keep MVC idea.

This is model, class Customer:

package customereditor;

public class Customer {

    private int ID;
    private String name;
    private String surname;

    public int getID() {
        return ID;
    }

    public void setID(int ID) {
        this.ID = ID;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public String getSurname() {
        return surname;
    }

    public void setSurname(String surname) {
        this.surname = surname;
    }

}

This is view, class CustomerView:

package customereditor;

public class CustomerView {

    public void displayCustomer(int customerID, String customerName, String customerSurname) {

        System.out.println("ID: " + customerID);
        System.out.println("Name: " + customerName);
        System.out.println("Surname: " + customerSurname);
    }

}

This is controller, class CustomerController:

package customereditor;

public class CustomerController {

    Customer model;
    CustomerView view;

    public CustomerController(Customer model, CustomerView view) {
        this.model = model;
        this.view = view;
    }

    public void setCustomerID(int customerID) {
       model.setID(customerID);
    }

    public int getCustomerID() {
        return model.getID();
    }

    public void setCustomerName(String customerName) {
       model.setName(customerName);
    }

    public String getCustomerName() {
        return model.getName();
    }

    public void setCustomerSurname(String customerSurname) {
       model.setSurname(customerSurname);
    }

    public String getCustomerSurname() {
        return model.getSurname();
    }

    public void displayCustomer() {
        view.displayCustomer(model.getID(),model.getName(),model.getSurname());
    }

}

And this is class with main method, class MainCustomer:

package customereditor;

public class MainCustomer {

    public static void main(String[] args) {

        Customer model = new Customer();
        CustomerView view = new CustomerView();
        CustomerController controller = new CustomerController(model, view);

        controller.setCustomerID(1);
        controller.setCustomerName("John");
        controller.setCustomerSurname("Doe");
        controller.displayCustomer();
    }
}

What you have here is not MVC. It's DataVC, this results in some minor and major design problems.
Unwrapping data in the Controller
That's the first really bad idea here. You're trying to hide the way Data is structured from the View, based on the assumption that the Model is the Data.
That's not the case. Just pass your Customer-instance to the View for display. The view knows what it needs from the Customer to display it properly.
The Controller doesn't.
Model != POJO
The Model in an MVC application is not the "Domain Model". It's instead the Layer to provide instances of classes modelling that Domain Model (in your case Customers).
Instead of having the Controller receiving a Customer from the calling code (which is unsustainable in larger applications), you should make the Controller ask the Model for customers depending on user-input.
More nitpicks

Your package-declaration should hint at a domain you own.
Making the Customer's field accessible through getters on the Controller is a dumb idea. The Controller doesn't actually have to know about the internals of Customers and shouldn't be asked about them