Read a text file and print pre-defined html when specific keywords are found How can I code this in a better way, in one file? Output must be more than one (if there is more that one status_update with the given word "online or performancedegradation"), like:

#online
System is operating at peak performance.

#directonly
Traffic is bypassing Smart CDN system, and going directly to websites.

<?php
$filename = "export_file_system_status_output";
$status_update = "online";
$handle = fopen($filename, "r");
$file = fread($handle,filesize($filename));
if (preg_match('#' . $status_update . '#', $file, $match)) {
echo "

#online

System is operating at peak performance.

";
}
?>

<?php
$filename = "export_file_system_status_output";
$status_update = "performancedegradation";
$handle = fopen($filename, "r");
$file = fread($handle,filesize($filename));
if (preg_match('#' . $status_update . '#', $file, $match)) {
echo "

#performancedegradation

Performance is slower than normal.

";
}
?>

<?php
$filename = "export_file_system_status_output";
$status_update = "directonly";
$handle = fopen($filename, "r");
$file = fread($handle,filesize($filename));
if (preg_match('#' . $status_update . '#', $file, $match)) {
echo "

#directonly

Traffic is bypassing Smart CDN system, and going directly to websites.

";
}
?>

It must sort by first status_update found:

online
#online

System is operating at peak performance.

then performancedegradation
#directonly

Traffic is bypassing Smart CDN system, and going directly to websites.

I'll give you a hint. Try using a foreach loop:
http://www.php.net/manual/en/control-structures.foreach.php

You can store all your files and associated information in an associative array, such as:

$files = array(
  0 => array(
    'filename' => "export_file_system_status_output";
    'status_update' => "online";
  ),
  1 => array(
    'filename' => "another_file";
    'status_update' => "offline";
  )
);

You should really try this on your own :) If you don't get it, I'll give you a more complete answer :)

On a side note, your HTML is not valid. Use classes instead of ids when possible, and use quotes:

#performancedegradation

Performance is slower than normal.

Hope this helps :)

EDIT:

Here's what your final code will look like, with the array I posted above:

foreach($files as $file)
{
  $handle = fopen($file['filename'], "r");
  $file = fread($handle,filesize($file['filename']));
  if (preg_match('#' . $file['status_update'] . '#', $file, $match))
  {
    echo '

#performancedegradation

Performance is slower than normal.

';
  }
}


SPOJ "It's a murder!" challenge I wrote a solution for SPOJ "It's a murder!". I'm exceeding the time limit even after n log(n) solution and fast I/O.

Given a list of numbers, I need to calculate the sum of the sum of the previously encountered numbers that are smaller than the current number.  For example, given 1 5 3 6 4, the answer is

(0) + (1) + (1) + (1 + 5 + 3) + (1 + 3) = 15

My code has complexity n log(n) and is pretty much similar to how we calculate the number of inversions. The function merge in the code below merges two sorted arrays and the function merge_sort is the basic call to merge sort procedure. The array sum1 stores the cumulative sum in array1 of elements whose index is strictly less less that current index . The variable ans stores the final answer. How can I make my code more efficient ?

#include <bits/stdc++.h>
using namespace std ;

//Declaration of global variables
int array[100000] , array1[100000] , array2[100000]   ;
long long int sum1[100000] ;
long long int ans ;

void merge_sort(int left , int right) ;
void merge(int left , int mid , int right) ;

int main()
{
    int t,counter,n,i ;

    // t is the number of testcases
    scanf("%d",&t) ;

    for(counter=0;counter<t;counter++)
    {
        // n is the number of elements in the array
        scanf("%d",&n) ;

        for(i=0;i<n;i++)
        {
            scanf("%d",&array[i]) ;

        }

        // ans hold the final answer and so it is initialized to 0 for every test case
        ans =0 ;
        merge_sort(0 , n-1) ;

        printf("%lld\n",ans );

    }

}

void merge(int left , int mid , int right)
{
    int  index , index1 , index2 ;

     // array1 is used to store the elements from left to mid
     // array2 is used to store the elemetns from mid+1 to right
     // sum1 holds the sum of elements whose index is less than current index in array1 so that sum1[0] is always 0 .

     // sum1 is initialised to 0
     memset(sum1,0,sizeof(sum1)) ;

    // copying into array1 from left to mid
    index1 = 0 ;
    for(index=left;index<mid+1;index++)
    {
        if(index1!=0)
        {
            sum1[index1] = sum1[index1-1] + array1[index1-1] ;
        }

        array1[index1] = array[index] ;
        index1++ ;
    }

    // copying into array2 from mid+1 to right
    index2 = 0;
    for(index=mid+1;index<right+1;index++)
    {

        array2[index2] = array[index] ;
        index2++ ;
    }

    //merging the two arrays array1 and array2 and adding to the variable  ans array[index1] if array1[index1] < array2[index2]
    index1 = 0 ;
    index2 = 0 ;
    index = left ;
    while((index1<mid-left+1)&&(index2<right-mid))
    {
        if(array1[index1]<array2[index2])
        {

            array[index] = array1[index1] ;
            index++ ;
            index1++ ;
        }
        else if(array1[index1]>=array2[index2])
        {
            ans = ans + sum1[index1];
            array[index] = array2[index2] ;
            index++ ;
            index2++ ;
        }

    }

    if(index1<mid-left+1)
    {
        while(index1<mid-left+1)
        {
            array[index] = array1[index1] ;
            index++ ;
            index1++ ;
        }
    }
    else if(index2<right-mid)
    {
        while(index2<right-mid)
        {
            ans = ans + sum1[index1-1] + array1[index1-1];
            array[index] = array2[index2] ;
            index++ ;
            index2++ ;
        }
    }
}

void merge_sort(int left , int right)
{
    // Typical merge sort procedure
    if(left==right)
    {

    }
    else
    {
        int mid = (left+right)/2 ;
        merge_sort(left , mid) ;
        merge_sort(mid+1 , right) ;
        merge(left , mid , right) ;
    }
}

Your main issue is this line:

 memset(sum1,0,sizeof(sum1)) ;

The whole array is being initialized to zero, even if you are only dealing with a range of one element.  You really only need to initialize the first element:

sum1[0] = 0; // initialize here

// copying into array1 from left to mid
index1 = 0 ;
for(index=left;index<mid+1;index++)
{
    if(index1!=0)
    {
        sum1[index1] = sum1[index1-1] + array1[index1-1] ;
    }

The rest is already assigned proper values.


Abstract Factory Pattern in Java I reproduced my version of Abstract Factory Pattern in Java. I know it's redundant but can I get critical comments about my code. Is it really the classic implementation of the pattern or not?

/*
* The demonstration of Abstract Factory Pattern
*
* Here the 'DocumentFactory' creates documents of types
* 'Resume' or 'Letter' which are concrete classes responsible to
* create things like 'FancyResume' or 'CoverLetter'
*
* The example is inspired by Wikipedia documentation of Abstract
* Factory.
*/

interface letter{
    void print();
}

class CoverLetter implements letter{
    @Override
    public void print(){
        System.out.println("Cover letter printed");
    }
}

class FancyLetter implements letter{
    @Override
    public void print(){
        System.out.println("Fancy letter printed");
    }
}

interface resume{
    void process();
}

class CorporateResume implements resume{
    @Override
    public void process(){
        System.out.println("Corporate resume processed");
    }
}

class FancyResume implements resume{
    @Override
    public void process(){
        System.out.println("Fancy resume processed");
    }
}

//Create 'Concrete' factories
class LetterFactory{
    public letter getLetter(String type){
        switch(type){
            case "Fancy":
                return new FancyLetter();
            case "Cover":
                return new CoverLetter();
        }
        return null;
    }
}

class ResumeFactory{
    public resume getResume(String type){
        switch(type){
            case "Fancy":
                return new FancyResume();
            case "Corporate":
                return new CorporateResume();
        }
        return null;
    }
}

// create abstract factory
abstract class DocumentFactory{
    abstract LetterFactory letterFactory();
    abstract ResumeFactory resumeFactory();
}

class DocumentCreator extends DocumentFactory{
    @Override
    LetterFactory letterFactory(){
        return new LetterFactory();
    }

    @Override
    ResumeFactory resumeFactory(){
        return new ResumeFactory();
    }
}

class abstractfactory{
    public static void main(String args[]){
        DocumentCreator creator = new DocumentCreator();

        // create a fancy letter
        letter myLetter = creator.letterFactory().getLetter("Fancy");
        myLetter.print();

        // create a corporate resume
        resume myResume = creator.resumeFactory().getResume("Corporate");
        myResume.process();
    }
}

First, use standard Java conventions for interfaces names: not resume or letter but Letter and Resume

Second, I would define e.g. StyleDocumentsFactory API like this

interface StyleDocumentsFactory {
    Letter createLetter();
    Resume createResume();
}

and then implement FuncyStyleDocumentsFactory and CorporateStyleDocumentsFactory instances of this API.

So you will have 2 abstract factories (for creation group of object with common theme: funcy and corporate) with set of factory methods.

Instantiate appropriate StyleDocumentsFactory impl in your main method at the program beginning.


Minimal Haskell Unit Testing Framework I'm trying to come up with a unit testing framework for Haskell that is

small and self-contained
produces TAP-compatible output
exits abnormally on failure (instead of relying on the TAP consumer to validate all the output).
has a simple API with easy to understand compile-time errors. (That's the motivation for committing to concrete types in the interface).

With that in mind, this is what I came up with:

I'm mainly looking for things that

are not idiomatic Haskell
would hamper usability in very small to small projects in The Real World.

module TestTrivial
    ( tests
    ) where

import System.Exit (exitSuccess, exitFailure)

testsImpl :: [(Bool, String)] -> Int -> Bool -> IO ()
testsImpl [] nextTest status =
    putStrLn ("1.." ++ show (nextTest - 1)) <> doExit where
        doExit = if status then exitSuccess else exitFailure
testsImpl ((cond, msg):xs) nextTest success =
    putStrLn msg' <> rest where
        ok = if cond then "ok" else "not ok"
        num = show nextTest
        f [] = unwords [ok, num]
        f m  = unwords [ok, num, "-", msg]
        msg' = f msg
        rest = testsImpl xs (nextTest + 1) (success && cond)

tests :: [(Bool, String)] -> IO ()
tests xs = testsImpl xs 1 True

And here's an example test suite using this library.

module TestAdd where

import TestTrivial

main = tests
    [ (1 + 4 == 5, "1 + 4 == 5")
    , (5 + 6 /= 7, "5 + 6 /= 7")
    ]

And what it produces. ... Despite the - sign separating the test number from the message and how strange that looks here, the output is formatted correctly.

ok 1 - 1 + 4 == 5
ok 2 - 5 + 6 /= 7
1..2

Replace explicit recursion with library combinators. Unduplicate and inline as much as possible.

testImpl :: Int -> (Bool, String) -> String
testImpl i (cond, msg) = unwords $
  [ if cond then "ok" else "not ok"
  , show i
  ] ++ case msg of [] -> []; m -> ["-", m]

tests :: [(Bool, String)] -> IO ()
tests xs = do
  putStrLn $ unlines $ zipWith testImpl [1..] xs
  putStrLn $ "1.." ++ show (length xs)
  if all fst xs then exitSuccess else exitFailure


Business rule check inside DAL service At now I have such code, creating or updating some entity by dto items.

public void Upsert(List<Object> items)
{
    var existings = _db.Get().Where().ToList();
    foreach (var item in items)
    {
        var existing = existings.FirstOrDefault(x => x.Id == item.SomeId);
        if (existing == null)
        {
            if (item.Property != 645)
            {
                throw new EntityNotFoundException();
            }

            // create new
        }
        else
        {
            // update existing
        }
    }
    _db.Save();
}

I want to move if (item.Property logic out of this layer. I cannot add some method to Object, it is simply dto. I also cannot subclass UpsertService.

What I can do is to pass Action<Object> checkWhenNotFound to Upsert method. Disadvantage - I need to write all those mocking It.IsAny<Action<Object, NotOnlyObjectInReal, AndMaybeMore>> in tests.

Other way is to inject checker inside Upsert caller ctor:

interface IUpsertService {
    void Upsert(List<Object> items);
    void SetChecker(Action<Object>);
}
public Owner(IUpsertService service)
{
    _service = service;
    service.SetChecker(CheckWhenNotFound)
}
private CheckWhenNotFound(Object item)
{
    if (item.Property != 645)
    {
        throw new EntityNotFoundException();
    }
}

Is there commmon solution for such problem?

Can't add a comment, so I'm going to try to answer the question.

If I'm understanding the question correctly, you don't want the responsibility of validation to be in the Upsert method. If that is the case then you can invert the control to another class by passing a 'Validator'. Which I think is what you are alluding to in the second part of the question but I would recommend the following implementation.

In a nutshell what you do is create a class which is responsible for validation which is injected into the main class. This makes this testable as well.

private readonly IValidator _validator;

public Constructor(IValidator validator)
{
  _validator = validator;
}

public void Upsert(List<Object> items)
{
    var existings = _db.Get().Where().ToList();
    foreach (var item in items)
    {
        var existing = existings.FirstOrDefault(x => x.Id == item.SomeId);
        if (existing == null)
        {
            _validator.ValidateEntity(existing);
        }
        else
        {
            // update existing
        }
    }
    _db.Save();
}

Example Implementation of the IValidator interface would be

public Validator : IValidator
{
  void ValidateEntity(object existing)
  {
    if (item.Property != 645)
    {
      throw new EntityNotFoundException();
    }
  }
}

You could take this further and use generics and allow the Validator class to decide what the object is and what needs validating.


Simplify definition of a dictionary with default return value I need a constant (dictionary with values and default return value if there are no defined value for a key). Searching for any ways to simplify the code.

Without a default value I can simply define a dictionary like this:

REDUCE_VALUE = {
    "honda": 0.95,
    "suzuki": 0.85,
    "yamaha": 0.87
}

But with a default value it looks not so good:

from collections import defaultdict
REDUCE_VALUE = defaultdict(lambda: "0.9")
REDUCE_VALUE["honda"] = 0.95
REDUCE_VALUE["suzuki"] = 0.85
REDUCE_VALUE["yamaha"] = 0.87

The same way that you would with dict.
From the documentation for collections.defaultdict it says:

  The first argument provides the initial value for the default_factory attribute; it defaults to None. All remaining arguments are treated the same as if they were passed to the dict constructor, including keyword arguments.

This means we can use all, bar b, of: (from the dict docs.)

>>> a = dict(one=1, two=2, three=3)
>>> b = {'one': 1, 'two': 2, 'three': 3}
>>> c = dict(zip(['one', 'two', 'three'], [1, 2, 3]))
>>> d = dict([('two', 2), ('one', 1), ('three', 3)])
>>> e = dict({'three': 3, 'one': 1, 'two': 2})
>>> a == b == c == d == e
True

Since you're overwriting suzuki, I'd remove the first assignment,
as dicts are unordered, and so it's up to chance which it uses.
And as Alex points out, you should keep the value data type consistent.

REDUCE_VALUE = defaultdict(lambda: 0.9, {"honda": 0.95, "suzuki": 0.87})


Any field change in form, disable button, and activate another I have JavaScript code that activates a button and deactivates another if a select is changed.

HTML

<select name="list" id="list">
     <option value="">choose</option>
     <option value="1">1</option>
     <option value="1">2</option>
     <option value="3">3</option>
</select>
<input name="refresh" type="button" value="refresh" disabled="" />
<input name="add" type="button" value="add"  />

JavaScript

$('select').change(function () {
    var op = $(this).val();
    if (op != '') {
        $('input[name="refresh"]').prop('disabled', false);
    } else {
        $('input[name="refresh"]').prop('disabled', true);
    }
});
$('select').change(function () {
    var op = $(this).val();
    if (op != '') {
        $('input[name="add"]').prop('disabled', true);
    } else {
        $('input[name="add"]').prop('disabled', false);
    }
});

I need to expand the function to all fields in the form with minimal code. I could use the same code for the rest of inputs, but the code will be very long. I'm looking for a short version.

Targetting elements

$('select') is generic selector and will select all <select> elements on the page. There might be some <select> elements which are not needed to target for this functionality.

You can target only required elements using

Unique parent selector + descendant selector

In this, there will be a main container element and using this all the descendant <select> elements can be targeted.

Example:

HTML:

    <select name="list" id="list">
        <option value="">...</option>
    </select>
    <input name="refresh" />
    <input name="add" type="button" value="add" />

    <select name="list1" id="list1">
        <option value="">...</option>
    </select>
    <input name="refresh1" />
    <input name="add1" type="button" value="add1" />

    ...

JavaScript:

$('#container select').change(function() {

Using common class
Sometimes, adding all the elements under a container is not possible. In such cases, adding a common class to all the elements will allow to select elements using class selector.

$('.someCommonClass').change(function() {

Common attributes can also be added and selected using Attribute Equals Selector [name=”value”]. However, this can be slower than class selectors.

Similarly, another common + unique class should be applied to buttons.

<input name="refresh" class="anotherUnique" />
<input name="add" class="anotherUnique" />

Then, to disable/enable these buttons, they can be referenced.

HTML5 data-* attributes

HTML5's custom data-* attribute can be used to store custom data on HTML elements.

<select name="list" id="list" data-key="value">

Getting data-* attribute value

jQuery provides data(name) method to get the value of the data-name attribute value. Core JavaScript has dataset property that allows access to data attributes.

$('elemSelector').data('key')

Using prop() with value

prop() accepts second parameter which will be converted to Boolean internally and depending on the value, the action will be taken. So, the if and else statements in the question can be combined as

$('elemSelector').prop('disabled', booleanValueHere);

If second parameter is truthy, element will be disabled. If falsy then element will be enabled.

Putting it all together

// Bind change event on all select elements
// having unique class
$('select.unique').change(function () {
    // Cache this
    var $this = $(this);

    // Get the data-* attribute values
    var button1 = $this.data('button1'),
        button2 = $this.data('button2');

    // Get the value of this select box
    // Use this as second parameter to prop
    $('input[name="' + button1 + '"]').prop('disabled', this.value);
    $('input[name="' + button2 + '"]').prop('disabled', !this.value);
});
/* For Demo Purpose */
input {
  color: green;
  font-weight: bold;
}
input:disabled {
  color: gray;
  opacity: .5;
}

<select name="list" id="list" class="unique" data-button1="refresh" data-button2="add">
    <option value="">choose</option>
    <option value="1">1</option>
    <option value="2">2</option>
    <option value="3">3</option>
</select>
<input name="refresh" type="button" class="anotherUnique" value="Refresh" />
<input name="add" type="button" class="anotherUnique" value="Add" disabled />

<select name="list1" id="list1" class="unique" data-button1="refresh1" data-button2="add1">
    <option value="">choose</option>
    <option value="10">10</option>
    <option value="20">20</option>
    <option value="30">30</option>
</select>
<input name="refresh1" type="button" class="anotherUnique1" value="Refresh 1" />
<input name="add1" type="button" class="anotherUnique1" value="Add 1" disabled />

Conclusion

Now, the JavaScript code don't have to be written again for each new select element. Just adding the markup with required attributes will automatically work. The relation between HTML element-JavaScript handler is reduced from many to many to many to one.


Thread safety of this queue backed by ConcurrentHashMap import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Future;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class DedupingQueue<E> implements QueueWrapper<E> {
    private static final Logger LOGGER = LoggerFactory.getLogger(DedupingQueue.class);

private final Map<E, Future<E>> itemsBeingWorkedOn = new ConcurrentHashMap<>();
private final AsyncWorker<E> asyncWorker;

public DedupingQueue(AsyncWorker<E> asyncWorker) {
    this.asyncWorker = asyncWorker;
}

@Override
public Future<E> submit(E e) {
    if (!itemsBeingWorkedOn.containsKey(e)) {
        itemsBeingWorkedOn.put(e, asyncWorker.executeWorkAsync(e, this));
    } else {
        LOGGER.debug("Rejected [{}] as it's already being worked on", e);
    }
    return itemsBeingWorkedOn.get(e);
}

@Override
public void complete(E e) {
    LOGGER.debug("Completed [{}]", e);
    itemsBeingWorkedOn.remove(e);
}

@Override
public void rejectAndRetry(E e) {
    itemsBeingWorkedOn.putIfAbsent(e, asyncWorker.executeWorkAsync(e, this));
}

}

I am having some difficulty reasoning the thread safeness of the above code.

I reckon complete and rejectAndretry are completely thread-safe as the map is thread safe. But what about submit? Also, how can I make it thread safe in the most efficient way?

What if you have 2 threads that submit the same E. The first one runs untill the switch here in the code snippet below. Then the second thread runs up to that point. Both pass the check because it's not in the map yet, but then both will put in a new Future in the map.

@Override
public Future<E> submit(E e) {
    if (!itemsBeingWorkedOn.containsKey(e)) {
        //***switch here***
        itemsBeingWorkedOn.put(e, asyncWorker.executeWorkAsync(e, this));
    } else {
        LOGGER.debug("Rejected [{}] as it's already being worked on", e);
    }
    return itemsBeingWorkedOn.get(e);
}

Easiest way to make this thread safe would be to just make the entire method synchronised. If you're expecting to skip a lot that are already in you might consider this idea based on double checked locking:

@Override
public Future<E> submit(E e) {
    if (!itemsBeingWorkedOn.containsKey(e)) {
        synchronised(this){
            if (!itemsBeingWorkedOn.containsKey(e)) {
                itemsBeingWorkedOn.put(e, asyncWorker.executeWorkAsync(e, this));
           }
       }
    } else {
        LOGGER.debug("Rejected [{}] as it's already being worked on", e);
    }
    return itemsBeingWorkedOn.get(e);
}

This might have some really sublte unexpected results when one finishes (and is taken out of the map) just when another is submitted (this can return null). Depending on how you intend to use the class this may not be a problem though.


Are there any memory issues with this Eigen QR wrapper? I am writing a wrapper to Eigen QR for my personal use and I am wondering if there are any memory leaks or undocumented behavior in my implementation, especially in the function get_QR().

The answer is as expected. This is related to my previous question here.

using std::cout;
using std::endl;
using namespace Eigen;

/*!
 Obtains the QR decomposition as A=QR, where all the matrices are in Eigen MatrixXd format.
 */
void get_QR(MatrixXd A, MatrixXd& Q, MatrixXd& R) {

        int m           =       A.rows();
        int n           =       A.cols();
        int minmn       =       min(m,n);

        //      A_E     =       Q_E*R_E.
        HouseholderQR<MatrixXd> qr(A);
        Q  =   qr.householderQ()*(MatrixXd::Identity(m, minmn));
        R  =   qr.matrixQR().block(0, 0, minmn, n).triangularView<Upper>();
}

/*!
 Obtains the QR decomposition as A=QR, where all the matrices are in double format.
 */
void get_QR(double* A, int m, int n, double*& Q, double*& R) {
        MatrixXd Q_E, R_E;
        int minmn       =       min(m,n);

        //      Maps the double to MatrixXd.
        Map<MatrixXd> A_E(A, m, n);

        get_QR(A_E, Q_E, R_E);

        Q       =       (double*)realloc(Q_E.data(), m*minmn*sizeof(double));
        R       =       (double*)realloc(R_E.data(), minmn*n*sizeof(double));
}

int main(int argc, char* argv[]) {
        srand(time(NULL));

        int m   =       atoi(argv[1]);
        int n   =       atoi(argv[2]);

//      Check the double version.
        int minmn       =       min(m,n);
        double* A       =       (double*)malloc(m*n*sizeof(double));
        double* Q       =       (double*)malloc(m*minmn*sizeof(double));
        double* R       =       (double*)malloc(minmn*n*sizeof(double));

        double RANDMAX  =       double(RAND_MAX);
        //      Initialize A as a random matrix.
        for (int index=0; index<m*n; ++index) {
                A[index]        =       rand()/RANDMAX;
        }

        get_QR(A, m, n, Q, R);
        std::cout << Q[0] << std::endl;

//      Check the MatrixXd version.
        Map<MatrixXd> A_E(A, m, n);
        MatrixXd Q_E, R_E;
        get_QR(A_E, Q_E, R_E);

        cout << Q[0] << endl;
        cout << Q_E(0,0) << endl;

        free(A);
        free(Q);
        free(R);
}

For instance, I get the output as

-0.360995
-0.360995
-0.360995

First check that there are the enough command line parameters

// If there are not enough parameters
//
// If there are no parameters passed then
// Then argv[1] is NULL
//      argv[2] is random
    int m   =       atoi(argv[1]);
    int n   =       atoi(argv[2]);

If not enough parameters are passed then this can get you funny number in m and n which will affect how much memory you allocate.

Don't use malloc in C++ code.
You can not mix malloc/free with new/delete. So by only using one system of memory management you will not mix up the two different types of memory.

    double* A       =       (double*)malloc(m*n*sizeof(double));

   // Also the C++ interface is simpler.

    double* A       =       new double[m*n];

Secondly don't use pointer.
In modern C++ it is very rare to see RAW pointers (as they have no concept of ownership). And they are prone to leaking. you should be using either smart pointers or containers to hold your dynamically allocated memory.

In this case a container would have been better.

   std::vector<double> A(m*n);

All memory management now handled correctly.
This can then be used in your C code with

   double* Ap = &A[0];  // get the address of the first element.

This is not going to go well:

    Q       =       (double*)realloc(Q_E.data(), m*minmn*sizeof(double));

  // Q_E.data() better not only return you a pointer allocated with malloc.
  //            But it better set the internal pointers to NULL or some
  //            other safe value. As soon as the realloc() finishes the
  //            old pointer is no longer valid.

Also this is not the correct way to use realloc.

  size_t    size = /* Calc Size */;
  double*   data = (double*)malloc(size);
  ...
  size_t    newSiz = /* New Space needed */;
  double*   resize = (double*)realloc(data, newSize);
  if (resize != NULL)
  {
      data = resize;  // if realloc() returns NULL the original array
      size = newSiz;  // is unaffected. You should check this after
  }                   // the call and only reassign the original
                      // value if the resize worked correctly.

Major Points:

I don't know what is happening inside MatrixXd but it looks like it is exposing its memory management outside the class. That is not a good idea. An object should know how to handle its memory in all situation (and do its own resizing as appropriate).

Two don't use RAW pointers (use C++ ownership symantics).

Learn the Rule of Three/Five so that objects manage their own memory.

Don't write C code and definately don't write C memory management code.


Concurrency-safe task scheduler I'm trying to do some task scheduler and worker. Tasks are added to the queue from which worker retrieves and processes them. Tasks from one queue must be executed sequentially. Tasks from different queues are executed in parallel. When the worker has completed all the tasks in the queue, he waits for a while and finishes his work. The queue must be removed from the schedule

I did something, but I think it isn't the best solution. I think there may be problems when worker shuts down and closes the channel.

Is my solution concurrency-safe?

// Schedule struct
type Schedule struct {
    sync.RWMutex
    queues map[int]chan Task
    idle   byte
}

// Scheduler pushes task to the queue
func (s *Schedule) Scheduler(t Task, i int) {
    var queue chan Task
    var ok bool

    s.RLock()
    if queue, ok = s.queues[i]; !ok {
        s.RUnlock()
        s.Lock()
        if queue, ok = s.queues[i]; !ok {
            queue = make(chan Task)
            s.queues[i] = queue
            go s.worker(queue, i)
        }
        s.Unlock()
    } else {
        s.RUnlock()
    }

    queue <- t
}

// Worker retrieves task from the queue and process
func (s *Schedule) worker(c chan Task, i int) {
    timeout := time.After(s.idle * time.Second)
    done := false
    for !done {
        select {
        case task := <-c:
            task.Execute()
            timeout = time.After(s.idle * time.Second)

        case <-timeout:
            s.Lock()
            close(c)
            delete(s.queues, i)
            s.Unlock()
            done = true

        default:
            time.Sleep(10 * time.Millisecond)
        }
    }
}

There is no need for the default clause in Schedule.worker.
Memory side, it is better to create a time.Timer object and call Timer.Reset & Timer.Stop on it.
No need for the done variable, we can just return when we are done.

func (s *Schedule) worker(c chan Task, i int) {
    t := time.NewTimer(s.idle * time.Second)
    for {
        select {
        case task := <-c:
            task.Execute()
            if !t.Stop() {
                <-t.C
            }
            t.Reset(s.idle * time.Second)
        case <-t.C:
            s.Lock()
            close(c)
            delete(s.queues, i)
            s.Unlock()
            return
         }
    }
}

It is not advisable to close a channel from a receiver. Better to close the channel when you know that there are no more tasks and have the worker wait for that.

func (s *Schedule) worker(c chan Task, i int) {
    for task := range c {
        task.Execute()
    }
}


Security and efficiency for profile picture upload I am trying to allow users to upload profile pics on my site. Basically I am following this code: Allow users to upload a pic for their profile.

I don't know if the code is secure enough. I am worried that users may upload malicious files.

Also, I am concerned that if there is many users, I will have many pictures in a single folder and it may slow down the retrieving process. I wonder what is the best practice in storing the profile pics. How many photos maximum should there be in a folder for better efficiency?

if(is_uploaded_file($_FILES['image']['tmp_name']))
{
    uploadImage($_FILES['image']['tmp_name'], 100, 100, "image/users/test2.jpeg");
}

and

function uploadImage($source, $max_width, $max_height, $destination) {

list($width, $height) = getimagesize($source);
if ($width > 150 || $height > 150) {
    $ratioh = $max_height / $height;
    $ratiow = $max_width / $width;
    $ratio = max($ratioh, $ratiow);
    // New dimensions
    $newwidth = intval($ratio * $width);
    $newheight = intval($ratio * $height);

    $newImage = imagecreatetruecolor($newwidth, $newheight);

    $ext = trim(strtolower($_FILES['image']['type']));

    $sourceImage = null;

    // Generate source image depending on file type
    switch ($ext) {
    case "image/jpg":
    case "image/jpeg":
        $sourceImage = imagecreatefromjpeg($source);
        break;
    case "image/gif":
        $sourceImage = imagecreatefromgif($source);
        break;
    case "image/png":
        $sourceImage = imagecreatefrompng($source);
        break;
    }

    imagecopyresampled($newImage, $sourceImage, 0, 0, 0, 0, $newwidth, $newheight, $width, $height);

    // Output file depending on type
    switch ($ext) {
    case "image/jpg":
    case "image/jpeg":
        imagejpeg($newImage, $destination);
        break;
    case "image/gif":
        imagegif($newImage, $destination);
        break;
    case "image/png":
        imagepng($newImage, $destination);
        break;
    }

    // Destroy resources
    imagedestroy($newImage);
    imagedestroy($sourceImage);

}
}

what happens if I upload a .exe file? or even a .txt file? Your application will give errors. Always have a fallback for if some hack0r uploads a non-picture to your application. Use the default: statement in your switch cases for that and hand back an error when they try to upload a non-image or a image you don't support (e.g. svg).

You talk about security and number of files in one directory, why is there a correlation between these two? The only restriction you have on the number of files per directory depends on the Filestructure format.

I would however create a directory per user instead of storing all the images in the same directory.


Flexbox layout, columns, rows, multiple heights I wanted everything to just snap into place, but found I had to add spacing to some of the items in order to line them up. It's still neater than trying to float things, and Grid isn't ready yet. But any advice/feedback will be appreciated.

div {padding:0px;margin:0;}
.container {
  display:flex;
  width:1070px;
  min-height:500px;
  margin:0 auto;
  flex-flow:row wrap;
  justify-content:space-between;
  align-content:space-between;
  align-items:flex-start;
  box-sizing:border-box;
}
.item > .item:first-child {padding-bottom:5px;}
.full-width {padding:5px 0;}

https://codepen.io/wagonlips/pen/zZzLqG

Use a different selector: div.subcontainer

Flexbox can work in either column or row. Because the code has a column and row you need to add another div (div.sub-container) with flexbox and change the height to the height of div.sub-container.


Send message and wait for receive while using async/await and promises the proper way I have this working code, but sendAndReceive function looks ugly/smelly to me. Eslint complains about using await inside a loop, and using true as conditional in while loop. Would there be a more elegant way to achieve this? I mean, "blocking" until a response appears in inbox before returning a response. This is necessary because I need to get each response before sending the next message, otherwise the device firmware complains.
const SerialPort = require('serialport');
const ReadLine = require('@serialport/parser-readline');

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

const inbox = [];

// IS THIS FUNCTION OK?
const sendAndReceive = async (message, port) => {
  port.write(message);
  while (true) {
    await sleep(1);
    if (inbox.length > 0) {
      const response = inbox.pop();
      return response;
    }
  }
};

SerialPort.list()
  .then(async portInfos => {
    portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
      .forEach(async portInfo => {
        const port = new SerialPort(portInfo.path).setEncoding('ascii');

        const parser = port.pipe(new ReadLine({
          delimiter: '\r\n',
          encoding: 'ascii',
        }));
        parser.on('data', data => {
          inbox.push(data);
        });

        port.open();

        const serialMessage = api.createReadMessage(SERIAL);
        const batteryMessage = api.createReadMessage(BATTERY);

        for await (const m of [serialMessage, batteryMessage]) {
          const response = await sendAndReceive(m, port);
          console.log(m.trim());
          console.log(response);
          console.log();
        }
      });
  });

The while (true) { await sleep(1); ... is potentially very computationally expensive. Yes, it'd be good to refactor it out if possible.
Response bug Your current implementation looks like it has a bug, or what could very easily become a bug. The inbox array is global, but each portInfo array item will push separate, undistinguished items to the inbox array. That is, each forEach iteration will create a port and open it immediately, and then whichever iteration whose sendAndReceive happens to run its timeout first after the data callback has pushed an item to the array will get the response. Here:
const response = await sendAndReceive(m, port);

Unless you have only a single portInfo, the response received is likely to correspond to a different iteration's m and port.
The easy way to fix this would be to declare a separate inbox array inside each iteration - but the whole approach there deserves some reworking, as you noticed.
Inside the innermost loop that calls sendAndReceive, you might construct Promises for each message being iterated over, and push their resolve function to an array. When the parser responds, if the array has a resolver function, shift it out and call it with the data:
portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
    .forEach(portInfo => {
        const port = new SerialPort(portInfo.path).setEncoding('ascii');
        const parser = port.pipe(new ReadLine({
            delimiter: '\r\n',
            encoding: 'ascii',
        }));

        const resolves = [];
        parser.on('data', data => {
            if (resolves.length) {
                resolves.shift()(data);
            }
        });

        port.open();

        const serialMessage = api.createReadMessage(SERIAL);
        const batteryMessage = api.createReadMessage(BATTERY);
        for (const message of [serialMessage, batteryMessage]) {
            new Promise((resolve) => {
                port.write(message);
                resolves.push(resolve)
            })
                .then((response) => {
                    console.log(m.trim());
                    console.log(response);
                });
        }
    });

The promises are currently dangling, which is usually weird, but in this case, the promises will never reject. If parser might not be able to get a data for a given message, and it exposes an API for that (an error event, maybe?), it would be good to listen for that, so errors can be properly handled instead of ignored.
Note that the above approach calls port.write twice at once in a given iteration *once for each method), instead of waiting for the prior message Promise to resolve first - this will speed up your program a bit, since you're now waiting in parallel, not in serial. If you wanted to do something when both messages are received, you'd use Promise.all and pass into it an array of the Promises instead of using for..of. (If you have to wait in serial, you can await the resolution of each Promise inside the loop)

Other things I noticed:
for..await is only for async iterators While you're technically allowed to do
for await (const m of [serialMessage, batteryMessage]) {

it doesn't make sense to use for await, since the array there is just a plain array - it's not something that implements the async iterator interface. It'd be more appropriate to use just for (const m ...
Variable names m isn't so great as a variable name. message is significantly more informative.
Only use async when you need to return a Promise You do:
SerialPort.list()
  .then(async portInfos => {
    portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
      .forEach(async portInfo => {

The .then's callback does not use await directly inside of it, nor does it need to return a Promise, so there's no need for the async keyword - it's just potentially confusing noise.


How to refactor this code to get a source from property files? I want to get the source of these files, but I don't know if I'm efficient.

I get the source of the file, put the source in a list, so I put this in a hashmap with the key is the version of file and the values are the list.

private Map<String, List<String>> loadXmlNodes() {

    Map<String, List<String>> mapNodes = new HashMap<>();
    List<String> nodes = XmlUtil.loadNodes1_10();

    mapNodes.put("1.10", nodes);
    nodes = XmlUtil.loadNodes2_00();
    mapNodes.put("2.00", nodes);

    return mapNodes;
}

public static List<String> loadNodes1_10() {

    List<String> node = new ArrayList<>();

    try {

        InputStream inputStream = XmlUtil.class.getClassLoader().getResourceAsStream("nodes1.10.properties");
        Properties properties = new Properties();
        properties.load(inputStream);

        for (Object value : properties.keySet()) {
            node.add((String) value);
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return node;
}

public static List<String> loadNodes2_00() {

    List<String> node = new ArrayList<>();

    try {

        InputStream inputStream = XmlUtil.class.getClassLoader().getResourceAsStream("nodes2.00.properties");
        Properties properties = new Properties();
        properties.load(inputStream);

        for (Object value : properties.keySet()) {
            node.add((String) value);
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return node;
}

There are a couple of inefficient things you do, and there's some better-practice items too.

First, let's take one of the methods:

public static List<String> loadNodes2_00() {

    List<String> node = new ArrayList<>();

    try {

        InputStream inputStream = XmlUtil.class.getClassLoader().getResourceAsStream("nodes2.00.properties");
        Properties properties = new Properties();
        properties.load(inputStream);

        for (Object value : properties.keySet()) {
            node.add((String) value);
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return node;
}

node is not a great name for something that is a list. It is typical in computer programs for node to point to just a particular element in a list, stack, or whatever, not the whole thing.

Something like result, or names would be better.

Then, you are not closing the InputStream. This is bad practice, and is a bad habit to be in. Java7 allows the try-with-resources statement, which is ideal for this code.

Now, there is the less-commonly-used method stringPropertyNames() available on Properties. It is useful here, instead of iterating on the keyValues().

Why are the loadNodes1_10() and loadNodes2_00() methods public? Do they need to be? They should be private to the class if they are not used.

Constant values embedded inside code are often called 'Magic values'. In this case, the values are Strings. You should declare a constant for your resource names....

Putting it all together, how about this method:

private static final String RESOURCE_NODES_2_00 = "nodes2.00.properties";
private static List<String> loadNodes2_00() {

    List<String> names = new ArrayList<>();
    try (InputStream inputStream = XMLUtil.class.getClassLoader()
                .getResourceAsStream(RESOURCE_NODES_2_00)) {

        Properties properties = new Properties();
        properties.load(inputStream);
        names.addAll(properties.stringPropertyNames());

    } catch (IOException e) {
        e.printStackTrace();
    }

    return names;
}

This is neater, and more efficient, and it closes all the streams correctly.... but... apart from the method name, and the file name, your two methods are identical. What you should do is generalize the method:

private static List<String> loadNodes(String resourcename) {

    List<String> names = new ArrayList<>();
    try (InputStream inputStream = XMLUtil.class.getClassLoader()
                .getResourceAsStream(resourcename)) {

        Properties properties = new Properties();
        properties.load(inputStream);
        names.addAll(properties.stringPropertyNames());

    } catch (IOException e) {
        e.printStackTrace();
    }

    return names;
}

Now, in your calling method, you can change your code to:

private Map<String, List<String>> loadXmlNodes() {

    Map<String, List<String>> mapNodes = new HashMap<>();
    mapNodes.put("1.10", loadNodes(RESOURCE_NODES_1_10);
    mapNodes.put("2.00", loadNodes(RESOURCE_NODES_2_00);

    return mapNodes;
}

This should simplify the code a whole lot.

If you need the two methods loadNodes1_10() and loadNodes2_00() to be public, I would make them simply:

public static List<String> loadNodes1_10() {
    return loadNodes(RESOURCE_NODES_1_10);
}

All the best, and enjoy.

EDIT:

If you want to return a Map<String,Map<String,String>> instead, it will look something like:

private static Map<String,String> loadNodes(String resourcename) {

    Map<String,String> names = new HashMap<>();
    try (InputStream inputStream = XMLUtil.class.getClassLoader()
                .getResourceAsStream(resourcename)) {

        Properties properties = new Properties();
        properties.load(inputStream);
        for (String key : properties.stringPropertyNames()) {
            names.put(key, properties.getProperty(key));
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return names;
}

and then you would have to adjust the other methods that call this respectively.


Dictionary Key look up with variable length inputs I am decoding GS1 barcodes and need to match up application identifiers with relevant data about them.

00    SSCC (Serial Shipping Container Code)   N2+N18  FALSE   SSCC
01    Global Trade Item Number (GTIN)         N2+N14  FALSE   GTIN
02    GTIN of Contained Trade Items           N2+N14  FALSE   CONTENT
10    Batch or Lot Number                     N2+X.20 TRUE    BATCH/LOT
11    Production Date (YYMMDD)                N2+N6   FALSE   PROD DATE
12    Due Date (YYMMDD)                       N2+N6   FALSE   DUE DATE
13    Packaging Date (YYMMDD)                 N2+N6   FALSE   PACK DATE
15    Best Before Date (YYMMDD)               N2+N6   FALSE   BEST BEFORE or BEST BY
16    Sell By Date (YYMMDD)                   N2+N6   FALSE   SELL BY
17    Expiration Date (YYMMDD)                N2+N6   FALSE   USE BY OR EXPIRY
20    Variant Number                          N2+N2   FALSE   VARIANT
21    Serial Number                           N2+X.20 TRUE    SERIAL
240   Additional Item Identification          N3+X.30 TRUE    ADDITIONAL ID
241   Customer Part Number                    N3+X.30 TRUE    CUST. PART NO.
242   Made-to-Order Variation Number          N3+N.6  TRUE    MTO VARIANT
243   Packaging Component Number              N3+X.20 TRUE    PCN
250   Secondary Serial Number                 N3+X.30 TRUE    SECONDARY SERIAL
251   Reference to Source Entity              N3+X.30 TRUE    REF. TO SOURCE
253   Global Document Type Identifier (GDTI)  N3+N13+XTRUE    GDTI
254   GLN Extension Component                 N3+X.20 TRUE    GLN EXTENSION COMPONENT
255   Global Coupon Number (GCN)              N3+N13+NTRUE    GCN
30    Count of Items (Variable Measu ...      N2+N..8 TRUE    VAR. COUNT
3100  Net weight  kilograms (Var... )     N4+N6   FALSE   NET WEIGHT (kg)
...

Full list here.

This is how I identify them:

public Dictionary<int, Tuple<string, string, bool, string>> gs1AiDict { get; private set; }

var aiDataOut = new Tuple<string, string, bool, string>("","",false,"");

//Get first 4 digits (max identifier length)
//the string input pi might look like this: ~10000123
int applicationIdentifier = Convert.ToInt16(pi.Substring(1,4));
//Try to find an identifier with 2 digits.
if (gs1AiDict.TryGetValue(applicationIdentifier / 100, out aiDataOut))
{
    //assign found data to tuple
    id = aiDataOut.Item1;
    format = aiDataOut.Item2;
    func = aiDataOut.Item3;
    abrv = aiDataOut.Item4;
}
//look up the first 3 digits
else
{
    if (gs1AiDict.TryGetValue(applicationIdentifier / 10, out aiDataOut))
    {
        id = aiDataOut.Item1;
        format = aiDataOut.Item2;
        func = aiDataOut.Item3;
        abrv = aiDataOut.Item4;
    }
    //look up first 4 digits
    else
    {
        if (gs1AiDict.TryGetValue(applicationIdentifier, out aiDataOut))
        {
            id = aiDataOut.Item1;
            format = aiDataOut.Item2;
            func = aiDataOut.Item3;
            abrv = aiDataOut.Item4;
        }
        //no identifiers are 5 digits
        else
        {
         console.log("No Application Identifier found!");
        }
    }
}

My tests have worked wonderfully, but my issue is the readability and efficiency of this solution. Are 3 dictionary key look ups for about 140 keys too much?

Code duplication is a sin

You could use shorter and more readable code using a simple for loop:

int applicationIdentifier = Convert.ToInt16(pi.Substring(1,4));
for (int i = 100; i >= 1; i /= 10)
{
    int maybeId = applicationIdentifier / i;
    if (gs1AiDict.TryGetValue(maybeId, out aiDataOut))
    {
        id = aiDataOut.Item1;
        format = aiDataOut.Item2;
        func = aiDataOut.Item3;
        abrv = aiDataOut.Item4;
        return;
    }
}
Console.WriteLine("No Application Identifier found!");

You should also consider making your data object something along the lines of a Dicitionary<int, ApplicationInfo>, that 4-member tuple doesn't help readibility. Then you could also return an ApplicationInfo object instead of copying the fields to local variables.


Create 1% Sample Using Multiprocessing in Python I'm trying to process a large dataset (300GB, myfile.txt in the script) line by line using multiprocessing. I want to define a 1% random sample based one variable (contained in unique_ids_final.txt). My first step is to define the sample and then I want to read the data file using multiprocessing. I would like to improve the efficiency of the code in two ways:

First, I'd like to pass the pct1 object from the parent to the child so that it only needs to be defined once. I've seen a description of this on thelaziestprogrammer.com: Pass Data to Workers w/o Globals, but I'm relatively new to python and don't understand how to implement it in my code.

Second, I'd like to define the chunks without reading in the data. In chunkify() I get the start of each chunk and the number of bytes that need to be read by looking for the end of the line after reading in 1MB of data. I was hoping to use seek to move forward by 1MB and then find the end of the line, but this creates problems because later I need to read in the chunks and read treats '\n' as one byte, while seek treats it as two.

Any other suggestions to increase efficiency would also be much appreciated!

#define sample
uid = list(line.strip() for line in open('Subsets/unique_ids_final.txt'))
pct1 = round(len(uid)/100)
random.seed(1)
id_pct1 = set(random.sample(uid, k=pct1))
id_pct1.add(vname)

#read original file and write 1% sample using multiprocessing
def worker(chunkStart, chunkSize, q):
    with open('myfile.txt') as f:
        tlines = []
        f.seek(chunkStart)
        lines = f.read(chunkSize).splitlines()
        for line in lines:
            data = line.split('*')
            if data[30] in id_pct1: tlines.append(line)
        q.put(tlines)
        return tlines

def chunkify(fname,size=1024*1024):
    fileEnd = os.path.getsize(fname)
    with open(fname, 'r') as f:
        chunkEnd2 = 0
        while True:
            chunkStart = chunkEnd2
            f.seek(chunkStart)
            f.read(size)
            chunkEnd1 = f.tell()
            f.readline()
            chunkEnd2 = f.tell()
            chunkSz = 1024*1024 + chunkEnd2 - chunkEnd1 - 1
            yield chunkStart, chunkSz
            if chunkEnd2 >= fileEnd:
                break

def listener(q):
    with open('myfile1pct.txt', 'w') as out_f1:
        while True:
            m = q.get()
            if m == 'kill': break
            else:
                for line in m:
                    out_f1.write(line+'\n')
                    out_f1.flush()

def main():

    manager = mp.Manager()
    q = manager.Queue()
    pool = mp.Pool()

    watcher = pool.apply_async(listener, (q,))

    jobs = []
    for chunkStart, chunkSize in chunkify('myfile.txt'):
        jobs.append(pool.apply_async(worker,(chunkStart,chunkSize,q)))

    for job in jobs:
        job.get()

    q.put('kill')
    pool.close()
    pool.join()

if __name__ == '__main__':
    main()

Disclaimer: I have never worked with multiprocessing, so I can't comment on that implementation.

Handling Files

I noticed at the top of the file you open a file, but never close it. (For the argument that an anonymous file is closed right after the statement is executed, see this post). Leaving files open is not a good idea. You should always close your files; leaving files open can slow down your program. I'd change that line to the code below:

with open('Subsets/unique_ids_final.txt') as file:
    uid = list(line.strip() for line in file.readlines())

While it is more typing, with automatically closes the file after the inner code is run.

List Comprehension

A couple of you for loops can be reduced to one line statements. You can create a list directly with a for loop. Take a look:

tlines = [line for line in lines if line.split('*')[30] in id_pct1]

Same with jobs:

jobs = [
    pool.apply_async(worker, (chunk_start, chunk_size, q))
    for chunk_start, chunk_size in chunkify('myfile.txt')
]

One line if statements

This

if m == 'kill': break

should be this

if m == 'kill':
    break

Even though it's one line, one word, you should still indent.

Docstrings

You should include docstrings when you write functions, methods, and classes. They are used to provide more description. Take a look:

def worker(chunk_start, chunk_size, q) -> List[str]:
    """
    Read original file and write 1% sample using multiprocessing

    :param <type> chunk_start: <description>
    :param <type> chunk_size: <description>
    :param <type> q: <description>

    :return: List[str]
    """

I had trouble following your code and understanding what variables were what types. Essentially, when labeling parameters in your docstrings, the format I used goes as follows:

:param <type of parameter> <name of parameter>: <description about parameter>

And returns are as follows:

:return: <type to return>

If you want docbuilders, consider using sphinx.

Type Hints

These help portray what types are accepted and returned by a function/method. Take a look:

def add(x: int, y: int) -> int:
    return x + y

While this is a very straightforward example, the idea is still there.

Variable/Parameter Names

These should be in snake_case, not mixedCase.

chunkStart -> chunk_start
chunkSize -> chunk_size


Indented if/else approach I have a flow where if a certain condition is true then I should take another approach. That condition can be triggered by different scenarios. But each condition test must be done only if the last one failed. Once a condition is true there is no further logical tests to do. My question is how to do that in an elegant way, the way I'm doing it right now requires an indentation for each condition.

    StatusCode statusCode = null;
    boolean invokeService = true;
    CardPort cardPort = findCardPort(card, port);
    if (cardPort == null) {
        statusCode = StatusCode.CARD_PORT_NOT_FOUND;
    } else {
        Sim sim = cardPort.getSim();
        if (sim == null) {
            statusCode = StatusCode.CARD_WITHOUT_SIM;
        } else {
            User user = sim.getUser();
            if (user == null) {
                statusCode = StatusCode.SIM_WITHOUT_USER;
            }
        }
    }

    if (statusCode != null) {
        invokeService = false;
    }

Personally, I would separate this into multiple functions:

public StatusCode GetStatus( ... )
{
    CardPort cardPort = findCardPort(card, port);
    if (cardPort == null) {
        return StatusCode.CARD_PORT_NOT_FOUND;
    }

    Sim sim = cardPort.getSim();
    if (sim == null) {
        return StatusCode.CARD_WITHOUT_SIM;
    }

    User user = sim.getUser();
    if (user == null) {
        return StatusCode.SIM_WITHOUT_USER;
    }

    return null; /* IMO, should be StatusCode.OK instead of null */
}

public bool ShouldInvokeService( ... )
{
    return GetStatus( ... ) == null;
}

Now you don't need the nested indentations to track the status state -- just check conditions until you either know everything is okay, or you run into a problem.


Build string by generating random characters step by step I'm trying to create a program that will try to build a target string character by character by generating random characters. I have come up with the following Nodejs program with a little bit of Google help:

const args = process.argv.slice(2)

const targetWord = args[0]
let builtWord = ''

function getRandomChar() {
    return String.fromCharCode(Math.floor(Math.random() * (126 - 32 + 1)) + 32)
}

function updateWord() {
    const {length} = builtWord
    const nextChar = getRandomChar()
    process.stdout.write('\r')
    process.stdout.write(builtWord)
    process.stdout.write(nextChar)
    if (targetWord[length] === nextChar) {
        builtWord += nextChar
    }
}

const sleep = ms => new Promise(resolve => setTimeout(resolve, ms))

async function main() {
    while (true) {
        updateWord()
        if (builtWord === targetWord) {
            break
        }
        await sleep(1000 / 16)
    }
}

main()

What are your thoughts on it, anything I can improve or do differently? Thanks !

Extract magic numbers into constants

For instance, what do 126 and 32 represent in the following line?

return String.fromCharCode(Math.floor(Math.random() * (126 - 32 + 1)) + 32)

It is much better to extract magic numbers into constants, i.e. to annotate them with a semantically meaningful name:

const HIGHEST_ASCII_PRINTABLE_CHARACTER = 126;
const LOWEST_ASCII_PRINTABLE_CHARACTER = 32;

But even when you replace the numbers by their constant's name, it isn't immediately clear what you're doing with -32, +32, Math.random() and Math.floor() there. Even if you are accustomed to random number generation, a dedicated function call is more readable with less redundancy and in fact is easier to reason about:

return String.fromCharCode(getRandomInt(LOWEST_ASCII_PRINTABLE_CHARACTER, HIGHEST_ASCII_PRINTABLE_CHARACTER));

Especially, with your old code, I had to look up Math.random() on MDN to make really sure that is produces a uniform distribution. Now you can just add a docblock comment to getRandomInt indicating that property.
await sleep(1000 / 16) also features magic numbers.

Miscellaneous remarks

Using async-await for waiting after each loop iteration is a good use case and really makes the code cleaner!
Extracting only a single property with the object destructuring syntax (const {length} = builtWord) seems a bit unidiomatic to me as opposed to const length = builtWord.length.
Apart from that, you are using length only once anyway, so I would drop that variable all together.
Try to avoid global variables and interdependencies.

For example, builtWord as well as targetWord are shared (and the former even modified!) by main and updateWord. I would suggest making updateWord accepting both as arguments and returning the possibly newly built word:

function tryUpdateWord(currentBuiltWord, targetWord) {
    // This is a good moment to introduce a sanity check as well
    if (currentBuildWord.length >= targetWord.length) {
      throw new Error('<todo>');
    }

    const nextChar = getRandomChar()
    process.stdout.write('\r')
    process.stdout.write(currentBuiltWord)
    process.stdout.write(nextChar)
    if (targetWord[currentBuildWord.length] === nextChar) {
        return currentBuiltWord + nextChar
    }
    else {
        return currentBuiltWord;
    }
}

Since the interdependencies have now been eliminiated, I would also move the part reading the process arguments into main():

async function main() {
    const targetWord = process.args[2]
    let builtWord = ''

    const SLEEP_PER_ITERATION = 1000 / 16;
    while (true) {
        builtWord = tryUpdateWord(builtWord, targetWord)
        if (builtWord === targetWord) {
            break
        }
        await sleep(SLEEP_PER_ITERATION)
    }
}


Basic playing card generation and shuffle This is my first attempt at Java so I appreciate any criticism or pointers.  The program should generate a full set of given decks and then shuffle them.

import java.util.Scanner;
import java.util.Arrays;
import java.util.Random;

public class shuffle {
    //Make it easier to find array items
    static int value = 0;
    static int suit = 1;
    static int remaining = 2;

    public static void main(String Args[]) {
            int[][] deck;

            String clabel;
            //number of decks to generate
            int deckcount = 1;
            int decksize = deckcount * 52;

            deck = new int[decksize][2];
            //loop through deck building until deck reaches card count
            for (int c = 0; c < decksize;) {
                for (int s = 0; s < 4; s++) {
                    for (int v = 1; v <= 13; v++) {
                        deck[c][value] = v;
                        deck[c][suit] = s;
                        System.out.println(getCard(deck[c]));
                        c++;
                    }
                }
                //test parameters for debugging
                //System.out.println(""+c);
                //pressAnyKeyToContinue();
            }

            System.out.println("\nSHUFFLING\n");

            int order[];
            order = new int[decksize];
            order = shuffle(decksize);

            for(int i = 0;i < decksize;i++) {
                System.out.println(getCard(deck[order[i]]));
            }
        }
    //method to get cards correct label
    public static String getCard(int[] card) {
            String c_label, s_name = "", v_name = "";
            //get suit name
            switch (card[suit]) {
                case 0:
                    s_name = "Clubs";
                    break;
                case 1:
                    s_name = "Diamonds";
                    break;
                case 2:
                    s_name = "Hearts";
                    break;
                case 3:
                    s_name = "Spades";
                    break;
            }
            //get card label
            if (card[value] > 10 || card[value] == 1) {
                switch (card[value]) {
                    case 1:
                        v_name = "ACE";
                        break;
                    case 11:
                        v_name = "Jack";
                        break;
                    case 12:
                        v_name = "Queen";
                        break;
                    case 13:
                        v_name = "King";
                        break;
                }
            } else {
                v_name = "" + card[value];
            }
            c_label = v_name + " of " + s_name;

            return c_label;

        }
        //shuffle deck
    public static int[] shuffle(int size) {
        Random rgen = new Random(); // Random number generator
        int[] order;
        order = new int[size];

        for(int i = 0; i < order.length; i++) {
            order[i] = i;
        }

        for (int i = 0; i < order.length; i++) {
            int randomPosition = rgen.nextInt(order.length);
            int temp = order[i];
            order[i] = order[randomPosition];
            order[randomPosition] = temp;
        }

        return order;
    }
    private static void pressAnyKeyToContinue() {
        System.out.println("Press any key to continue...");
        try {
            System.in.read();
        } catch (Exception e) {}
    }
}

First off I want to say that your decision to make an array that represents the ordering permutation is a really good idea. Many people would have passed in the array to be shuffled and had it mutated. Returning a permutation and applying that to an ordered array is a conceptually much nicer way to go.

Your shuffle algorithm has an interesting and common bug. Your shuffle algorithm is "generate the vector {0, 1, 2, 3, 4, ..., 51}. For each position 0 through 51, swap the current contents with a randomly-generated position".

Let's do a little math. Suppose we have only three cards, to make the math easier. How many decks are there? Six:

0, 1, 2
0, 2, 1
1, 0, 2
1, 2, 0
2, 0, 1
2, 1, 0

Each deck should be as likely as any other.

How many paths through the algorithm are there? Twenty-seven, each equally likely:

Swap 0 with 0, swap 1 with 0, swap 2 with 0
Swap 0 with 1, swap 1 with 0, swap 2 with 0
Swap 0 with 2, swap 1 with 0, swap 2 with 0
Swap 0 with 0, swap 1 with 1, swap 2 with 0
Swap 0 with 1, swap 1 with 1, swap 2 with 0
Swap 0 with 2, swap 1 with 1, swap 2 with 0
Swap 0 with 0, swap 1 with 2, swap 2 with 0
Swap 0 with 1, swap 1 with 2, swap 2 with 0
Swap 0 with 2, swap 1 with 2, swap 2 with 0

... I won't list all of them.

Now we have 6 possible outcomes and 27 paths through the algorithm, so plainly some of the outcomes must be produced more often than the others. There is no way to divide 27 paths into 6 sets such that each set has the same number of elements.

The same is true with your shuffle; there are \$52!\$ possible decks and \$52^{52}\$ paths through the code, and those don't divide evenly into each other.

What you've done is the most common incorrect implementation of the Knuth Shuffle.  Now go do some research and figure out how to fix your bug; it's an easy fix.

Once you've done that, you've still got a problem. There are 52! possible decks but only \$2^{32}\$ possible seeds to Random, so you are still not sampling the space of all possible decks fairly.  If you care about this, you should probably use a crypto-strength randomness generator.

You might be interested in my articles on these subjects.

http://ericlippert.com/2013/04/15/producing-permutations-part-one/
http://ericlippert.com/2014/10/13/producing-combinations-part-one/

Other problems with your code:

int[][] deck;

What the heck?  A deck of cards is an array of arrays of integers?  That's craziness.  I want to see

Deck deck = new Deck();

Programming computers is the art of creating meaningful abstractions.  A deck is not an array of arrays of integers.  A deck is a deck. Make a class that represents the concept of deck that abstracts away this mechanism.

String clabel;

We pronounce that "klabel" of course.

Seriously, typing out the remaining letters of a word will not cause you to die younger.

Later...

  String c_label, s_name = "", v_name = "";

Oh, it's c_label now when it was clabel before. Now would be a good time to get into good habits. I don't care what your naming strategy is, so long as it is (1) comprehensible, and (2) consistent.  Your strategy is neither.  My preference as a C# programmer is

cardLabel

but

card_label

is idiomatic in Java.  Better entirely though would be to make a Card type that has a label getter.

        order = new int[decksize];
        order = shuffle(decksize);

If you saw

x = 1;
x = 2;

you would immediately note that the first line is pointless. But your code is equally pointless for exactly the same reason.

Something made you think that you needed to initialize order to a new array immediately before throwing it away, just like the contents of x are thrown away. What incorrect process of thought led you to this conclusion?  Can you examine that process?

These are not rhetorical questions. I analyze these sorts of programming errors for a living and I am very interested in the beginner-mind psychology that leads to them. Any insights would be appreciated.

WHY IS "ACE" IN ALL CAPS? It seems weird.


Audit Updating Macro In my code, I'm trying to manipulate data for an audit between 3 sheets in a workbook.The first block of my code is to paste the data of items I need to find for the audit from the original sheet onto the 3rd sheet by setting each row equal to the data of the original row in the first sheet. The second block is used to re-paste the data of found objects in the Audit to only have values rather then formulas.Then the code will iterate through the Audit list to check for the same values and delete those values on the list in the 3rd sheet. The 2nd sheet will have the list of found audit items being pasted in at the same time. The end result is 3 sheets, 1st being just the main list where all the data is collected, the 2nd being a list of found audit items, and the 3rd being left over items that need to be found at a later date. The code works and has a few kinks in it where the screen would be buzzing because of all of the activate lines so I was wondering if there were better ways to manipulate data between different sheets in a workbook.

Sub Update_Audit()
    Dim j As Integer
    Dim i As Integer
    Dim k As Integer
    Dim Aud_Tot As Integer
    i = 2
    Aud_Tot = Application.InputBox("How big is your audit", , , , , , , 1)
    k = 2
    Worksheets(1).Activate
    Do While Cells(k, 24) <> ""
        Tab_Data = Range(Cells(k, 24), Cells(k, 44)).Value
        Worksheets(3).Activate
        Range(Cells(k, 1), Cells(k, 21)).Value = Tab_Data
        Worksheets(1).Activate
        k = k + 1
    Loop
    Do While Cells(i, 1).Value <> "" And Not IsError(Cells(i, 2).Value)
        Dataset = Range(Cells(i, 1), Cells(i, 22)).Value
        Range(Cells(i, 1), Cells(i, 22)).Value = Dataset
        Worksheets(2).Activate
        Range(Cells(i, 1), Cells(i, 22)).Value = Dataset
        Worksheets(1).Activate
        For j = 2 To Aud_Tot
            If CStr(Cells(j, 24).Value) = CStr(Cells(i, 2).Value) Then
                Worksheets(3).Activate
                Range(Cells(j, 1), (Cells(j, 22))).Delete Shift:=xlShiftUp
                Worksheets(1).Activate
                Exit For
            End If
        Next j
        i = i + 1
    Loop
End Sub

Option Explicit
That should be at the top of every VBA module you ever create. Go to Tools -> Options -> Require Variable Declaration to have it inserted automatically. It's important because it forces you to declare every variable you use, and so automatically gets you to declare types and catches any typos that creep in. Those 2 alone will prevent all sorts of problems down the line.

Very Low Hanging Performance Fruit
VBA has 3 of these:
Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual

Doing the following will vastly increase the speed of your code:
Public Sub DoThing()

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlCalculationManual

    ...

    Code

    ...

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlCalculationAutomatic

End Sub

In this case, since you're relying on certain formulas to throw errors, you should probably keep Application.Calculation on xlCalculationAutomatic.

Use the Object Model
The great power of VBA comes from its' tight integration with the Office Object Model (from were Intellisense gains its' power).
Worksheet objects, Workbook objects, Range objects, Array objects, Err (error) objects etc.
Rather than constantly activating different worksheets, put them in objects and then refer to them instead:
    Dim sourceDataSheet As Worksheet
    Set sourceDataSheet = Worksheets(1)

    Dim foundItemsSheet As Worksheet
    Set foundItemsSheet = Worksheets(2)

    Dim remainingItemsSheet As Worksheet
    Set remainingItemsSheet = Worksheets(3)

    ...

    Do While sourceDataSheet.Cells(k, 24) <> ""

        Tab_Data = sourceDataSheet.Range(sourceDataSheet.Cells(k, 24), sourceDataSheet.Cells(k, 44)).Value

        remainingItemsSheet.Range(remainingItemsSheet.Cells(k, 1), remainingItemsSheet.Cells(k, 21)).Value = Tab_Data

        k = k + 1

    Loop

This also lets you do really awesome things like hold object references using With statements:
    Do While sourceDataSheet.Cells(k, 24) <> ""

        With sourceDataSheet
            Tab_Data = .Range(.Cells(k, 24), .Cells(k, 44)).Value
        End With

        With remainingItemsSheet
            .Range(.Cells(k, 1), .Cells(k, 21)).Value = Tab_Data
        End With

        k = k + 1

    Loop

And now you can forget about having to keep using Activate ever again.
It also lets you re-use references, so this:
    Do While Cells(i, 1).Value <> "" And Not IsError(Cells(i, 2).Value)
        Dataset = Range(Cells(i, 1), Cells(i, 22)).Value
        Range(Cells(i, 1), Cells(i, 22)).Value = Dataset
        Worksheets(2).Activate
        Range(Cells(i, 1), Cells(i, 22)).Value = Dataset
        Worksheets(1).Activate
        For j = 2 To Aud_Tot
            If CStr(Cells(j, 24).Value) = CStr(Cells(i, 2).Value) Then
                Worksheets(3).Activate
                Range(Cells(j, 1), (Cells(j, 22))).Delete Shift:=xlShiftUp
                Worksheets(1).Activate
                Exit For
            End If
        Next j
        i = i + 1
    Loop

becomes this:
    Dim startCell As Range
    Dim errCheckCell As Range

    Const START_COLUMN As Long = 1
    Const ERR_CHECK_COLUMN As Long = 2
    Const END_COLUMN As Long = 22

    Dim sourceDataRange As Range
    Dim pasteDataRange As Range

    Set startCell = sourceDataSheet.Cells(i, START_COLUMN)
    Set errCheckCell = sourceDataSheet.Cells(i, ERR_CHECK_COLUMN)

    Do While startCell.Value <> "" And Not IsError(errCheckCell.Value)

        With sourceDataSheet
            Set sourceDataRange = .Range(.Cells(i, START_COLUMN), .Cells(i, END_COLUMN))
        End With

        With foundItemsSheet
            Set pasteDataRange = .Range(.Cells(i, START_COLUMN), .Cells(i, END_COLUMN))
        End With

        Dataset = sourceDataRange

        sourceDataRange = Dataset
        pasteDataRange = Dataset

        For j = 2 To Aud_Tot

            If CStr(sourceDataSheet.Cells(j, 24).Value) = CStr(errCheckCell.Value) Then

                With remainingItemsSheet
                    .Range(.Cells(j, 1), (.Cells(j, 22))).Delete Shift:=xlShiftUp
                End With

                Exit For

            End If

        Next j

        i = i + 1

    Loop

which looks a little bigger (right now, we'll get to cleaning it up later) but is much, much clearer about what's going on and where, and lets you change just one reference if and when things get moved/changed in the future.
For instance, what happens if your order of worksheets gets changed? Now, you only have to change that once, right at the start, and the rest takes care of itself.

Tips and Tricks

finalRow - Want to find the last used row in a column?
    Dim finalRow As Long

    With sheetObject
        finalRow = .Cells(.Rows.Count, targetColumn).End(xlUp).Row
    End With

And you can then use
    For k = 2 To finalRow

        ...

    Next k

instead of that unwieldy Do While cellReference(k).Value <> ""

constants - if you're going to hard-code values (e.g. Column 1, Column 22, Worksheets(1)) then actually hard-code them. Once. In one place. So you can change it in a single go rather than having to track down every occurence of the thing (and invariably missing some and causing errors).
The proper variable for a constant value is, unsurprisingly a Constant. Standard VBA Naming Conventions use SHOUTY_SNAKE_CASE for constants. Created like so:
Option Explicit

Public Const GLOBAL_CONSTANT As Boolean = True
Private Const MODULE_CONSTANT As Long = 42

Public Sub DoThing()

    Const PROCEDURE_CONSTANT As Long = 1

    ...

End Sub

Codenames - Every Worksheet has a codename (name) property. If you got to the properties window in the Editor, select a worksheet and type, e.g. sheetCodename in the (name) property, then you can write a procedure like so:
Public Sub DoThingWithSheet()

    sheetCodename.Cells(1, 1).Value = 1

without having to declare the sheet, or assume anything about its' name, or its' position in your workbook, or anything else. The variable is just there, constant and unchanging.

Your Code, Better
(assuming we've already given your sheets the following codenames: dataSheet, foundItemsSheet, remainingItemsSheet)
Public Sub UpdateAudit()

    Const TAB_START_COLUMN As Long = 24
    Const TAB_END_COLUMN As Long = 44

    Const TAB_PASTE_START_COLUMN As Long = 1
    Const TAB_PASTE_END_COLUMN As Long = 1
    Const START_ROW As Long = 2 '/ +1 for headers

    Dim numItemsToAudit As Long
    numItemsToAudit = Application.InputBox("How big is your audit", Type:=1)

    Dim finalRow As Long
    With dataSheet
        finalRow = .Cells(.Rows.Count, TAB_START_COLUMN).End(xlUp).Row
    End With

    '/ Copy All Raw Data to "remainingItemsToFind" sheet
    Dim tabData As Variant
    Dim iRow As Long
    For iRow = START_ROW To finalRow

        With dataSheet
            tabData = .Range(.Cells(iRow, TAB_START_COLUMN), Cells(iRow, TAB_END_COLUMN))
        End With

        With remainingItemsSheet
            .Range(.Cells(iRow, TAB_PASTE_START_COLUMN), .Cells(iRow, TAB_PASTE_END_COLUMN)) = tabData
        End With

    Next iRow

    '/ For each row in "rawData" sheet, check for error.
    '/ If not error, copy to "foundItems" sheet and delete from "remainingItems" sheet
    Const FOUND_START_COLUMN As Long = 1
    Const FOUND_END_COLUMN As Long = 22

    Const FOUND_ERR_CHECK_COLUMN As Long = 2
    Const REMAINING_ERR_CHECK_COLUMN As Long = 24

    With dataSheet
        finalRow = .Cells(.Rows.Count, FOUND_START_COLUMN).End(xlUp).Row
    End With

    Dim dataArray As Variant
    Dim dataRange As Range
    Dim pasteRange As Range

    Dim foundErrCheckCell As Range
    Dim remainingErrCheckCell As Range

    Dim errCheckRow As Long

    For iRow = START_ROW To finalRow

        Set foundErrCheckCell = dataSheet.Cells(iRow, FOUND_ERR_CHECK_COLUMN)

        If Not IsError(foundErrCheckCell) Then

            '/ Get Source Data
            With dataSheet
                Set dataRange = .Range(.Cells(iRow, FOUND_START_COLUMN), .Cells(iRow, FOUND_END_COLUMN))
            End With

            dataArray = dataRange

            With foundItemsSheet
                Set pasteRange = .Range(.Cells(iRow, FOUND_START_COLUMN), .Cells(iRow, FOUND_END_COLUMN))
            End With

            '/ Copy Data
            dataRange = dataArray
            pasteRange = dataArray

            '/ Find and Delete from "remainging items" sheet
            For errCheckRow = 2 To numItemsToAudit

                Set remainingErrCheckCell = dataSheet.Cells(errCheckRow, REMAINING_ERR_CHECK_COLUMN)

                If remainingErrCheckCell.Text = foundErrCheckCell.Text Then
                    remainingItemsSheet.Rows(errCheckRow).Delete shift:=xlShiftUp
                    Exit For
                End If

            Next errCheckRow

    Next iRow

End Sub

You'll notice the better code looks longer. This is entirely down to having more variable declarations and adding some whitespace for readability. When measuring code, the metric that matters is not how much you can cram into a small space, but how quickly you can read and understand the code and how to change it.


Checking for a win in Tic Tac Toe I'm working on Tic Tac Toe and I want do it for any ROW x COL board with any WIN_SIZE (e.g. 4x4 board requires 3 to win). I've done the hasWon method already but I'm not sure is it done right. Can you check what can I do better here?

public boolean hasWon(Seed theSeed) {
    int count = 0;

    for (int col = 0; col < COLS; ++col) {               //check columns
        if (cells[currentRow][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
    }
    count = 0;

    for (int row = 0; row < ROWS; ++row) {              //check rows
        if (cells[row][currentCol].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
    }
    count = 0;

    int row = currentRow;

    for (int col = currentCol; col >= 0; --col) {      //check forward diagonal
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
        row--;
        if (row < 0)
            break;
    }
    row = currentRow;
    count--;
    for (int col = currentCol; col < COLS; ++col) {
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
        row++;
        if (row == ROWS)
            break;
    }

    count = 0;
    row = currentRow;

    for (int col = currentCol; col >= 0; --col) {  //check backward diagonal
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
        row++;
        if (row == ROWS)
            break;
    }

    row = currentRow;
    count--;
    for (int col = currentCol; col < COLS; ++col) {
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
        row--;
        if (row < 0)
            break;
    }
    return false;
}

public boolean hasWon(Seed theSeed) {

What is a Seed?  The way that you are using it, I'd expect a Player object.

    for (int col = 0; col < COLS; ++col) {               //check columns
        if (cells[currentRow][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
    }

A more common format would be

    //check squares in currentRow
    for (int col = 0; col < COLS; ++col) {
        if (cells[currentRow][col].content == theSeed) {
            count++;
        } else {
            count = 0;
        }

        if (count == WIN_SIZE) {
            return true;
        }
    }

I added the else because I would expect a match to need to be consecutive.  As is, you would accept any row containing WIN_SIZE total matches.  Maybe that's what you want, but it's not what I would expect.

You iterate over columns when you are checking a row.  The original comment was a bit confusing on that aspect.

While there is a single statement form of control structures (if), it is often recommended not to use it.  It's less clear about what goes with what and it's easier to edit in a way that causes problems but is syntactically correct.

    count = 0;

    int row = currentRow;

    for (int col = currentCol; col >= 0; --col) {      //check forward diagonal
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
        row--;
        if (row < 0)
            break;
    }
    row = currentRow;
    count--;
    for (int col = currentCol; col < COLS; ++col) {
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
        row++;
        if (row == ROWS)
            break;
    }

This looks more complicated than it needs to be.  Consider

    // check forward diagonal
    for (int col = currentCol, row = currentRow; col >= 0 && row >= 0; --col, --row) {
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
    }

    for (int col = currentCol + 1, row = currentRow + 1; col < COLS && row < ROWS; ++col, ++row) {
        if (cells[row][col].content == theSeed)
            count++;
        if (count == WIN_SIZE)
            return true;
    }

Yes, you can use two variables in one for loop.

Rather than decrementing the count, this skips the current square in the second loop.

Or do the whole thing in one loop.

    // check forward diagonal
    count = 0;
    int col = 0;
    int row = 0;
    if (currentCol >= currentRow) {
        col = currentCol - currentRow;
    } else {
        row = currentRow - currentCol;
    }

    for (; col < COLS && row < ROWS; ++col, ++row) {
        if (cells[row][col].content == theSeed) {
            count++;
        }

        if (count == WIN_SIZE) {
            return true;
        }
    }

More work up front to figure out the first square in the diagonal, but only a single loop.

I left off the else case, as I'm not sure whether the current behavior is intentional or not.  The single loop version works with the else as before.  The double loop versions don't.


Bitmask type to perform binary operations Without any specific implementation details, how would you create a bitmask type that wraps binary arithmetic into something easy to read? Ideally, this would be used with an enum, which is why there are some generic methods.

Some Questions

Is the type a struct or class?
Is the type generic or are certain methods generic?
Is it okay to constrain the generics to struct, IConvertible?
Is a type like this preferred when working with bitmasks?

Starting Example: Here's a fiddle.

//Example Use
var days = new Bitmask((ulong)(Day.Monday | Day.Tuesday));
days.Contains((ulong)Day.Friday); //false
days.Contains((ulong)Day.Monday); //true
//Example Use END

public enum Day { Monday = 1, Tuesday = 2, Wednesday = 4, Thursday = 8, Friday = 16, Saturday = 32, Sunday = 64 }

public struct Bitmask
{
    //Bitmask Value
    private ulong _value;
    public ulong Value
    {
        get { return _value; }
        set { _value = value; }
    }

    //Constructor
    public Bitmask(ulong value) { _value = value; }

    //Methods
    public bool Any() { return _value != 0; }

    public void Clear() { _value = 0; }

    public bool Contains(ulong value) { return (_value & value) == value; }

    public bool Equals(ulong value) { return _value == value; }

    public List<TEnum> ToList<TEnum>() where TEnum : struct, IConvertible
    {
        var local = this;
        var result = ((TEnum[])Enum.GetValues(typeof(TEnum))).Where(m => local.Contains<TEnum>(m)).ToList();
        return result;
    }
}

It looks like you are misusing an enum as just a more cumbersome way to write a ulong.

If the days of the week are meant to be treated as a set rather than a single value (i.e., more like checkboxes than radio buttons), then you should be using the [Flags] attribute.  In fact, this is almost exactly the example given in the MSDN documentation:

[Flags]
enum Days2
{
    None = 0x0,
    Sunday = 0x1,
    Monday = 0x2,
    Tuesday = 0x4,
    Wednesday = 0x8,
    Thursday = 0x10,
    Friday = 0x20,
    Saturday = 0x40
}

class MyClass
{
    Days2 meetingDays = Days2.Tuesday | Days2.Thursday;
}

Personally, I'd write the constants using left-shift operators:

[Flags]
enum Days2
{
    None = 0,
    Sunday = 1 << 0,
    Monday = 1 << 1,
    Tuesday = 1 << 2,
    Wednesday = 1 << 3,
    Thursday = 1 << 4,
    Friday = 1 << 5,
    Saturday = 1 << 6
}


Counting average protein length from a proteome fasta file this code is counting the average protein length (number of amino acids) across the proteome of some microbes.

As it stands the code takes a very very long time to run, I think I'm being very inefficient somewhere but not 100% sure where.

The way I run the code is using a bash-loop (using find for each example of particular file type, like *.faa) like this:

for FAA in $(find . -name "*.faa")
do
    python proteinlengthgen.py $FAA
done

My code:

from Bio import SeqIO
import sys
from collections import Counter

def species_name_function(infile):
    for record in SeqIO.parse(infile, "fasta"):
        speciesname = record.description.split('[', 1)[1].split(']', 1)[0]
    return speciesname

if __name__ == '__main__':
    frequency = []
    infile = sys.argv[1]
    avgproteinlength = 0
    totalproteinlength = 0
    fastacounter = 0

    for fasta in SeqIO.parse(open(infile), "fasta"):
        dna = str(fasta.seq)
        freq = Counter(dna)
        fastacounter = fastacounter + 1
        frequency.append(freq)
        species_name = species_name_function(infile)

        genomesize = freq['G'] + freq['A'] + freq['L'] + freq['M'] + freq['F'] + freq['W'] + freq['K'] + freq['Q'] + freq['E'] + freq['S'] + freq['P'] + freq['V'] + freq['I'] + freq['C'] + freq['Y'] + freq['H'] + freq['R'] + freq['N'] + freq['D'] + freq['T']

        listofbases = ['G', 'A', 'L', 'M', 'F', 'W', 'K', 'Q', 'E', 'S', 'P', 'V', 'I', 'C', 'Y', 'H', 'R', 'N', 'D', 'T']

        totalproteinlength = genomesize + totalproteinlength

    avgproteinlength = totalproteinlength / fastacounter

    towrite = str(avgproteinlength) + '\t' + species_name + '\t' + '\n'

    with open("proteinlength.csv", "a") as myfile:
        myfile.write(towrite)

Example of a subset of input file (I cannot use a whole one as they are very big):

>WP_013179448.1 DNA-directed RNA polymerase [Methanococcus voltae]
MYKILTIEDTIRIPPKMFGNPLKDNVQKVLMEKYEGILDKDLGFILAIEDIDQISEGDIIYGDGAAYHDTTFNILTYEPE
VHEMIEGEIVDIVEFGAFIRLGPLDGLIHISQVMDDYVAFDPQREAIIGKETGKVLEKGDKVRARIVAVSLKEDRKRGSK
IALTMRQPALGKLEWLEDEKLETMENAEF
>WP_013179449.1 DNA-directed RNA polymerase subunit E'' [Methanococcus voltae]
MARKGLKACTKCNYITHDDFCPICQHETSENIRGLLIILDPVNSEVAKIAQKDIKGKYALSVK
>WP_013179451.1 30S ribosomal protein S24e [Methanococcus voltae]
MDIKVVSEKNNPLLGRKEVKFALKYEGATPAVKDVKMKLVAILNANKELLVIDELAQEFGKMEANGYAKIYESEEAMNSI
EKKSIIEKNKIVEEAEEAQE
>WP_013179452.1 30S ribosomal protein S27ae [Methanococcus voltae]
MAQKTKKSDYYKIDGDKVERLKKSCPKCGEGVFMAEHLNRFACGKCGYMEYKKNEKAEKEE
>WP_013179453.1 hypothetical protein [Methanococcus voltae]
MNELNELKNPDKIDGKNNNTKNNNNNNNKDSNTENSITEIIKADNETQDNLSDLCVLEDIKTLKSKYKVYKTSKYLTKKD
INNIIEKDYDEIIMPQSIYKLLNEKNKSSMEKLRLCGIIVKTTDNVGRPKKITKYDKDKIKELLVDGKSVRKTAEIMDMK
KTTVWENIKDCMNEIKIEKFRKMIYEYKELLIMQERYGSYVESLFLELDIYINNEDMENALEILNKIIIYVKSEDKKD
>WP_013179454.1 integrase [Methanococcus voltae]
MKNKRINNNQKSKWETMRTDVINTQRNQNINSKNKQYRVKKHYCKEWLTKEELKVFIETIEYSEHKLFFKMLYGMALRVS
ELLKIKVQDLQLKEGVCKLWDTKTDYFQVCLIPDWLINDIKEYIALKSLDSSQELFKFNNRKYVWELAKKYSKMAELDKD
ISTHTFRRSRALHLLNDGVPLEKVSKYLRHKSIGTTMSYIRITVVDLKQELDKIDDWYEL
>WP_013179455.1 hypothetical protein [Methanococcus voltae]
MNTQNAIKKTLKTSKVNKNISNVIIGYSAILLDTYSNNKNLLLVKYDKLFKGFLNSSSITEKQYNKLYDTVLNSLF
>WP_013179456.1 hypothetical protein [Methanococcus voltae]
MVVKLVKISNGGYVSSLELKRINDIILSQLTNEFTIKDIVNMYSNKYDDCNNNAIAQKTRRLLNNHIESGVFTVRNALKN
KKIYKFKDVFVPASAGDTNTSLLFYSTSMKNSNHIEKQKKNNNKYNTNVNKPTITPDQIRVMAGIVNNPQIKSLKKERFK
SILHLNCKHMLNEEDRTELLENFKEYIIKASSQNLVLERTRYHKNKPKYITFPYLTRFTNSKQLKRQLAQYNCIFEQKAI
KYNRGVHLTLTTDPKLFRNIYEANKHFSKAFNRFMSFLSKRNKDVNGKSRRPVYLAAYEYTKSGLCHAHILIFGKKYLLD
QRVITQEWSRCGQGQIAHIYSIRRDGINWIYGRARPEEIQTGEYKNANEYLKKYLVKSLYSELDGSMYWAMNKRFFTFSK
SLKPAMMKRPKPEKYYKFVGIWTDEEFTDEINQAFKTGIPYDEIKKIHWKNLNNGLSCG
>WP_013179457.1 hypothetical protein [Methanococcus voltae]
MVRGRYPVFSGFKKFNKINLGKEKRNEGVYKYYNQDKTLLYVGVSNEVKLRLLSAYYGRSDYAVLENKKKLRQNIAYYKV
KYCGLDQARKIEHRIKKQCKYNLN

Output of code:

302.7408088235294   Methanococcus voltae

The first thing to do to accelerate processing is to read the file only once. species_name_function reads the entire file, and is called multiple times. Extract the line which parses it and inline into the main loop:

    for fasta in SeqIO.parse(open(infile), "fasta"):
        species_name = fasta.description.split('[', 1)[1].split(']', 1)[0]

Now,

        dna = str(fasta.seq)
        freq = Counter(dna)

does unnecessary work. There's no need to convert to a string before counting.

listofbases is unused, and genomesize is calculated by a large explicit sum. My best guess is that you were experimenting, trying to come up with

        genomesize = sum(freq[base] for base in "GALMFWKQESPVICYHRNDT")

Using those changes and applying some further simplifications which I don't think need individual explanation, I get:

from Bio import SeqIO
import sys
from collections import Counter

if __name__ == '__main__':
    infile = sys.argv[1]
    genomesizes = []

    for fasta in SeqIO.parse(open(infile), "fasta"):
        species_name = fasta.description.split('[', 1)[1].split(']', 1)[0]
        freq = Counter(fasta.seq)
        genomesizes.append(sum(freq[base] for base in "GALMFWKQESPVICYHRNDT"))

    avgproteinlength = sum(genomesizes) / len(genomesizes)
    towrite = str(avgproteinlength) + '\t' + species_name + '\t' + '\n'

    with open("proteinlength.csv", "a") as myfile:
        myfile.write(towrite)

However, I'm not very convinced by the explicit output filename. Also, I would be inclined to simplify the usage by supporting multiple files:

from Bio import SeqIO
import sys
from collections import Counter

if __name__ == '__main__':
    for infile in sys.argv[1:]:
        genomesizes = []

        for fasta in SeqIO.parse(open(infile), "fasta"):
            species_name = fasta.description.split('[', 1)[1].split(']', 1)[0]
            freq = Counter(fasta.seq)
            genomesizes.append(sum(freq[base] for base in "GALMFWKQESPVICYHRNDT"))

        avgproteinlength = sum(genomesizes) / len(genomesizes)
        print(str(avgproteinlength) + '\t' + species_name)

called as

find . -name "*.faa" | xargs python proteinlengthgen.py >>proteinlength.csv


Checking if a student ID is present in the database What I need to do is fetch the XML link using curl then get a student ID and check if the ID is present in the database. If yes, then I need to get other details from another database table.

My code works fine but it's kinda slow. I later need to add more functions in this which will make the loading speed slower. Is there any way I can improve this code?

It currently takes around 5-6 second to display result when XML has 10-12 ID elements.

<?php

$parseXml = "example.com/xml.xml";
$result = curl_json($parseXml);
if ($result['httpcode'] === 200) {
    $output = simplexml_load_string($result['result']) or die('err');
    if ($output->result) {
        echo 'Results: ';
        foreach ($output->result as $resultItem) {
            echo checkID($resultItem->title, $resultItem->dataID);
        }
        echo '';
    } else {
        echo 'no result';
    }
}

function checkID($title, $id) {
    $con = mysql_connect(HOST, DBUSER, DBPASS);
    if ($con) {
        mysql_select_db(DBNAME);
        $sql = "SELECT * FROM idlist WHERE id='$id'";
        $retval = mysql_query($sql);
        if (!$retval) {
            die();
        }
        $num_rows = mysql_num_rows($retval);
        if ($row = mysql_fetch_array($retval, MYSQL_ASSOC)) {
            $out['val'] = 'yes';
            $out['data'] = getInfo($id);
            return $out;
        }
    }
}

function getInfo($id) {
    $con = mysql_connect(HOST, DBUSER, DBPASS);
    if ($con) {
        mysql_select_db(DBNAME);
        $sql = "SELECT * FROM infolist WHERE id='$id'";
        $retval = mysql_query($sql);
        if (!$retval) {
            die();
        }
        if ($row = mysql_fetch_array($retval, MYSQL_ASSOC)) {
            $arr = array();
            $arr['name'] = $row['name'];
            $arr['class'] = $row['class'];
            return $arr;
        }
    }
}

function curl_json($url) {
    $ch = curl_init();
    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
    curl_setopt($ch, CURLOPT_URL, $url);
    $result = curl_exec($ch);
    $httpcode = curl_getinfo($ch, CURLINFO_HTTP_CODE);
    curl_close($ch);

    $out['result'] = $result;
    $out['httpcode'] = $httpcode;

    return($out);
}

mysql_ is deprecated, and it has been for quite a while. It will be removed in PHP 7, so you should not use it anymore.
you are open to SQL injection. Never put variables directly into SQL queries, always use prepared statements.
if you use prepared statements, you could prepare your queries only once, and then execute them with multiple different values, which might increase your performance.
use one connection across your code instead of creating new ones for every query (this can increase your performance).
profile your code to find out what actually causes performance problems. There is no need to "improve" code that is not causing problems. My guess would be that the curl request is the bottleneck, but without profiling, it's hard to say.
return early to reduce your level of nesting. Eg if (!$cond) return false (or throw an exception).
don't just silently fail, as it will be extremely difficult to debug your code this way. You silently fail if the httpcode of your curl request isn't 200, you silently fail if you can't create a database connection, etc.
don't die in functions, it makes it impossible for the calling code to recover. Throw an exception instead. If you do die, at the very least add a message stating the reason for it, as it will otherwise be really hard to debug your code.