Java job responsible for processing file statements I am looking for any refactoring tips to improve code readability. Does it feel like procedural programming? If so, how can I improve it?

public class StatementJob implements Job {

    private static final Logger LOGGER = LoggerFactory.getLogger(StatementJob.class);
    private static final String JOB_IS_DISABLED = "Uploads statement job is disabled and will not run";
    private static final String UNABLE_TO_CONTINUE = "Unable to continue";

    private final Configuration jobConfiguration;
    private final Log log;

    public StatementJob(JobConfiguration jobConfiguration) {
        this.jobConfiguration = jobConfiguration;
        this.log = new JobLog();
    }

    @Override
    public Log execute() {

        LOGGER.info("Stating uploading statements");
        final JobProperties properties;
        try {
            properties = jobConfiguration.loadProperties();
        } catch (BadConfigurationException e) {
            return log.addError(e.getMessage());
        }

        if (!properties.isRunJob()) {
            return log.addSuccess(JOB_IS_DISABLED);
        }

        final Employee statementUploader = Employee.load(properties.getStatementUploader());
        final FileStatements fileStatements = new Statements(properties.getDirectories());

        fileStatements
                .getInboundStatements()
                .forEach(inboundStatement -> {

                    try {
                        fileStatements.moveToWipLocation(inboundStatement);
                        fileStatements
                                .getWipStatements()
                                .forEach(wipFileStatement -> {

                                    try {
                                        new UploadableDocument(statementUploader, wipFileStatement).upload();
                                        fileStatements.moveToArchiveLocation(wipFileStatement);
                                    } catch (FailedToUploadDocumentException e) {
                                        log.addError(e.getMessage());
                                        try {
                                            fileStatements.moveToErrorLocation(wipFileStatement);
                                        } catch (UnableToMoveFileToDirectoryException ex) {
                                            LOGGER.error(UNABLE_TO_CONTINUE, e);
                                            throw new FatalJobException(UNABLE_TO_CONTINUE, e);
                                        }
                                    } catch (UnableToReadFileException | UnableToMoveFileToDirectoryException e) {
                                        LOGGER.error(UNABLE_TO_CONTINUE, e);
                                        throw new FatalJobException("Unable to continue", e);
                                    }
                                });
                    } catch (UnableToMoveFileToDirectoryException e) {
                        LOGGER.error(UNABLE_TO_CONTINUE, e);
                        throw new FatalJobException("Unable to continue", e);
                    }
                });

        return log;
    }

}

What does the code do?

There is FileStatements which represents files, called Statement, stored in the file system in the inbound directory.
When StatementJob is executed it moves Statements from the inbound to work in progress directory.
Then Statements from the work in progress directory are uploded to some storage.
When the upload is succesfull, the Statements are moved to the archive statement folder. Statements which failed to upload are moved to the error statement folder.
Log is used to collect information about the job that will be emailed to whomever needs to know the status. This is very primitive at this point.

Here's the best I think we can get it without actually knowing what your code is for.

public class StatementJob {
    public static final Logger LOGGER = LoggerFactory.getLogger(StatementJob.class);

    public static final String JOB_IS_DISABLED = "Uploads statement job is disabled and will not run";
    public static final String UNABLE_TO_CONTINUE = "Unable to continue";

    private final Configuration jobConfiguration;
    private final Log log;

    public StatementJob(JobConfiguration jobConfiguration) {
        this.jobConfiguration = jobConfiguration;
        this.log = new JobLog();
    }

    public Log execute() {
        LOGGER.debug("Stating uploading statements");

        final JobProperties properties;
        try {
            properties = jobConfiguration.loadProperties();
        } catch (BadConfigurationException e) {
            return log.addError(e.getMessage());
        }

        if (!properties.isRunJob()) {
            return log.addSuccess(JOB_IS_DISABLED);
        }

        Employee employee = Employee.load(properties.getStatementUploader());
        FileStatements fileStatements = new Statements(properties.getDirectories());

        FatalConsumer<WipStatement> wipConsumer = wip -> {
            try {
                new UploadableDocument(employee, wip).upload();
                fileStatements.moveToArchiveLocation(wip);
            } catch (FailedToUploadDocumentException e) {
                log.addError(e.getMessage());
                fileStatements.moveToErrorLocation(wip);
            }
        };
        FatalConsumer<InboundStatement> inboundConsumer = inbound -> {
            fileStatements.moveToWipLocation(inbound);
            fileStatements.getWipStatements().forEach(wipConsumer);
        };

        fileStatements.getInboundStatements().forEach(inboundConsumer);

        return log;
    }

    @FunctionalInterface
    public interface FatalConsumer<T> extends Consumer<T> {
        void fatalAccept(T t) throws UnableToMoveFileToDirectoryException, UnableToReadFileException;

        @Override
        default void accept(T t) {
            try {
                fatalAccept(t);
            } catch (UnableToMoveFileToDirectoryException | UnableToReadFileException e) {
                LOGGER.error(UNABLE_TO_CONTINUE, e);
            }
        }
    }
}

Changelog:

Separate constants to be grouped by category. The logger typically is flush with the class declaration while other constants are grouped below it.
No need to have the extra space after the start of the execute method
Use more accurate log levels. Specifically, you overuse "info". Refer to this article about different things to avoid when logging effectively.

"DEBUG is intended for messages that could be useful in debugging an issue (ex: method execution started)"

Use line breaks in fluent calls only when it becomes too long, and then only when you have created your stream/data-structure. Specifically, there's no need to have a line break after fileStatements
You have a lot of excess indentation and repeated code. Specifically, all of times you catch an exception, log it, and then immediately rethrow it. My changes above resolve that.
If you are going to statically define the document uploader to be an Employee, naming the variable documentUploader instead of employee is kind of pointless.

I would highly recommend moving FatalConsumer and each of the non-logging constants you have to their own files. Besides that, this is about as good as you're going to get imo. Hope this helps!


MySQL database connection in the constructor I'm an absolute beginner in PHP OOP in search of the "Holy Grail" of connecting to MySQL database once and reusing this connection for the whole site.

classes/db.php

<?php
define('SERVER', 'localhost');
define('USERNAME', 'root');
define('PASSWORD', 'password');
define('DATABASE', 'cms');

class DB {
    function __construct(){
        $connection = @mysql_connect(SERVER, USERNAME, PASSWORD) or die('Connection error -> ' . mysql_error());
        mysql_select_db(DATABASE, $connection) or die('Database error -> ' . mysql_error());
    }
}
?>

classes/users.php

<?php
class Users {
    function __construct(){
        $db = new DB();
    }

    public function read(){
        $query = mysql_query("select use_id, use_name, use_email FROM users");
        while ($row = mysql_fetch_array($query)){
            $data[] = $row;
        }
        return $data;
    }
}
?>

users.php

<?php require_once('classes/db.php'); ?>
<?php require_once('classes/users.php'); ?>
<?php
$Users = new Users();
$users = $Users->read();
?>
<?php
if ($users) {
    echo '
';
    foreach($users as $user){
        echo '
' . $user['use_name'] . '
';
    }
    echo '
';
}
?>

My doubts are mostly on the Users class part:

function __construct(){
    $db = new DB();
}

It seems to be an easy way to have the connection available, but I read somewhere that instantiate the db connection in the constructor is a bad idea.  Can you explain why, and if there's a better way to have the db connection easily available in every class that needs it?

I read a similar question here, but I can't understand the abstraction/holding reference/singleton suggestion from the accepted answer, and the lack of a full practical example doesn't help me.

A better practice is passing a created DB instance to the Users class. Imagine that you have a Products class, an Orders class etc. Will all of their constructors create a separate MySQL connection? It doesn't look a good idea since all of them could use the same database connection.

class Users {
    private $db;

    public function __construct($db) {
        $this->db = $db;
    }

    ....
}

class Products {
    private $db;

    public function __construct($db) {
        $this->db = $db;
    }

    ....
}

$db = new DB();
$users = new Users($db);
$products = new Products($db);


Remove specific consecutive element in array I'm trying to reduce consecutive elements of array to one, but not for all values like:

{3,0,0,0,3,3,3,0,0,0} => {3,0,3,0}

but for specific one, in my example 0:

{3,0,0,0,3,3,3,0,0,0} => {3,0,3,3,3,0}

so only zeros (the threes are intact) are reduced.

I have Java String working code I wrote:

public static String removeConsecutive(String str, char remove) {
    char[] chars = str.toCharArray();

    int current = 0;
    int result  = current;
    while (current < chars.length) {
        if (chars[current] == remove) {
            // keep the first occurrence
            chars[result++] = chars[current++];

            // ignore the others
            while (current < chars.length && chars[current] == remove) {
                ++current;
            }
        } else {
            chars[result++] = chars[current++];
        }
    }

    return new String(chars, 0, result);
}

and it does the trick:

public static void main(String[] args) {
    System.out.println(removeConsecutive("000300300030303330000", '0'));
}

outputs: 0303030303330

Can anyone suggest any improvements, since it think the code is not perfect. It's doesn't have to be String in use, but with any other array.

You can have the same output with regex , using Arrays.toString(char[]):

System.out.println("000300300030303330000".replaceAll("[0]+", "0"));

or, with a parameter :

char charToRemove = '0' ;
System.out.println("000300300030303330000".replaceAll("[" + charToRemove + "]{1,}", charToRemove + ""));

or change charToRemove  + "" in  Character.toString(charToRemove) more academic.


Print multiplication tables Write a function that given a max argument will print a nicely aligned multiplication tables, for example, if max = 8

  1   2   3   4   5   6   7   8
  2   4   6   8  10  12  14  16
  3   6   9  12  15  18  21  24
  4   8  12  16  20  24  28  32
  5  10  15  20  25  30  35  40
  6  12  18  24  30  36  42  48
  7  14  21  28  35  42  49  56
  8  16  24  32  40  48  56  64

The code is pretty straightforward, but I am interested in any possible improvement.

def print_multiplication_table(max)
  pad = (1 + (max*max).to_s.length)
  puts (1..max)
    .to_a
    .product((1..max).to_a)
    .map{|a, b| a * b}
    .each_slice(max)
    .map{ |x| x.map(&:to_s).map{ |x| " " * (pad - x.length) + x}.join(' ')}
    .join("\n")
end

print_multiplication_table(8)

Spike gave a nice answer already. I just wanted to point out one thing: This is a good use case for rjust, aka "right-justify". With that, you can skip the string-multiplication when you pad the numbers.

def print_multiplication_table(max)
  padding = (max * max).to_s.length + 1
  sequence = (1..max).to_a
  puts sequence.product(sequence)
    .map { |a, b| (a * b).to_s.rjust(padding) }
    .each_slice(max)
    .map(&:join)
    .join("\n")
end

This one only uses 1 space between columns, but 2 spaces can be had by replacing .map(&:join) with .map { |row| row.join(" ") }.


Evaluation of a solution for Run-length encoding algorithm I wrote an answer for the following question and wondering if there is any better approach to it.

  Using the Java language, have the function RunLength(str) take the str
  parameter being passed and return a compressed version of the string
  using the Run-length encoding algorithm. This algorithm works by
  taking the occurrence of each repeating character and outputting that
  number along with a single character of the repeating sequence.

  For example: "wwwggopp" would return 3w2g1o2p. The string will not
  contain any numbers, punctuation, or symbols.

Code

 public class App {
    void runLength(String str) {
        HashMap<Character, Integer> hash = new HashMap<Character, Integer>();
        Character c;
        for (int i = 0; i < str.length(); i++) {
            c = str.toLowerCase().charAt(i);
            if (hash.containsKey(c)) {
                int value = hash.get(c);
                value++;
                hash.put(c, value);
            } else {
                hash.put(c, 1);
            }
        }
        int value = 0;

        StringBuffer buf = new StringBuffer();
        String temp;
        for (int j = 0; j < str.length(); j++) {
            c = str.toLowerCase().charAt(j);
            value = hash.get(c);
            temp = str.substring(j, j + 1);
            if (buf.indexOf(c.toString()) == -1) {
                buf.append(value);
                buf.append(temp);
            }

        }
        System.err.println(buf.toString());

    }

    public static void main(String[] args) {
        new App().runLength("wwwggopp");
    }
}

Output

3w2g1o2p

Avoid printing in methods, unless the purpose of the method is speicifically to print. The main purpose of the runLength method is certainly not to print. It should return the compressed value instead.

The description doesn't say that you should treat upper and lowercase characters equal. It's not correct to do that. And if you really wanted to do that,
it would have been simpler to lowercase the entire input string once.
This is a very wasteful operation:

for (int i = 0; i < str.length(); i++) {
    c = str.toLowerCase().charAt(i);

This would have been much better:

String lowered = str.toLowerCase();
for (int i = 0; i < lowered.length(); i++) {
    c = lowered.charAt(i);

Declare variables with the interface type, when possible.
Instead of:

HashMap<Character, Integer> hash = new HashMap<Character, Integer>();

This would have been better:

Map<Character, Integer> hash = new HashMap<Character, Integer>();

Btw are you still on Java6? You should definitely migrate to at least Java7,
where the above declaration becomes simply:

Map<Character, Integer> hash = new HashMap<>();

Note that in most use cases, StringBuilder is preferred over StringBuffer.
StringBuffer is synchronized, StringBuilder is not, which makes it faster.
In this program you don't need synchronization when building the compressed string.

Suggested implementation

This is simpler, without a hashmap:

public String compress(String str) {
    if (str.isEmpty()) {
        return "";
    }

    char[] chars = str.toCharArray();
    StringBuilder builder = new StringBuilder();

    int count = 1;
    char prev = chars[0];
    for (int i = 1; i < chars.length; i++) {
        char current = chars[i];
        if (current == prev) {
            count++;
        } else {
            builder.append(count).append(prev);
            count = 1;
        }
        prev = current;
    }
    return builder.append(count).append(prev).toString();
}

Unit testing

It's always good to have unit tests to verify correctness:

@Test
public void test_aabcccccaaa() {
    assertEquals("2a1b5c3a", compress("aabcccccaaa"));
}

@Test
public void test_a5() {
    assertEquals("5a", compress("aaaaa"));
}

@Test
public void test_empty() {
    assertEquals("", compress(""));
}

@Test
public void test_a() {
    assertEquals("1a", compress("a"));
}

@Test
public void test_a3b4() {
    assertEquals("3a4b", compress("aaabbbb"));
}

@Test
public void test_abc() {
    assertEquals("1a1b1c", compress("abc"));
}

@Test
public void test_wwwggopp() {
    assertEquals("3w2g1o2p", compress("wwwggopp"));
}


Logging to a File vs UDP I'm trying to understand where my ruby implementation of this fails and hoping for an expert to shed some light on this situation.  I've tried to make this a very simple example with the idea that setup is minimal.

The Problem:

I need to be able to log very quickly, ideally over UDP.  Alternatively I can log to a file but I'd prefer not to for a variety of reasons but mainly logfile rotations, another dependency like a "forwarding" app, etc.

With the code below I am able to benchmark 100_000 [<= 16byte] log messages:

                            Logfile  UDP
 Time elapsed (in seconds)  1.36    24.48

Specifics on that code include:  ruby 2.0.0-p353 MRI

The Logfile Setup

require 'logger'
logger = Logger.new("logfile.log")

start_time = Time.now
100_000.times do |i|
  logger.info "#{i}"
end

puts "Elapsed time was #{Time.now - start_time}"

UDP

The server (run this first):

require 'socket'
s = UDPSocket.new
s.bind(nil, 1234)
loop do
  text, sender = s.recvfrom_nonblock(16)
  # puts text
end

The client:

require 'socket'
s = UDPSocket.new
before_time = Time.now
100_000.times do |i|
  s.send("#{i}", 0, 'localhost', 1234)
end
puts "Time elapsed was #{Time.now - before_time}"

Why UDP?  Well I was under the impression that UDP was a "fire and forget" protocol where the client would not wait for the server to say they received it.  Fire and forget is fine for me in this scenario, I care more about performance.

Issue 1

As outlined in "The Problem" area - why is UDP so much slower than writing to a file?

Issue 2

You'll notice I have a commented out puts statement in the server.  If you uncomment that, the client's benchmark approaches 40+ seconds which really makes me think the client is waiting for the server or the server is communicating with the client.  Either way I'm probably not understanding UDP correctly.

Well I learned a lot today, specifically that writing to UDP is on average faster than writing to a file on my local machine and on my servers.  My implementation on the UDP client was off.

If you look at this code:

require 'socket'
s = UDPSocket.new
before_time = Time.now
100_000.times do |i|
  s.send("#{i}", 0, 'localhost', 1234)
end
puts "Time elapsed was #{Time.now - before_time}"

There's something sneaky in there that I didn't catch with this line:

s.send("#{i}", 0, 'localhost', 1234)

When you pass in host and port into the send method it DNS resolves before each send, which has major overhead.  If instead I change the code to connect ahead of time like so:

require 'socket'
s = UDPSocket.new
before_time = Time.now
s.connect('127.0.0.1', 1234)
100_000.times do |i|
  s.send("#{i}", 0)
end
puts "Time elapsed was #{Time.now - before_time}"

I get benchmarks that beat File IO.  Boom.


Recursive function with Promise and setTimeout to ping DOM I use this function in browser automation. It pings the document in the browser to check if an element we are looking for is available (useful when there is JavaScript generated async content). If it's not available, it will retry after a given interval or times out eventually.

Example usage:

function clickContinue() {
  return $(":contains('Continue')");
}

elementFinder(clickContinue).then(el => el.click());

Function:

// accepts both document.querySelector and jQuery() selector
function elementFinder(selectorFn) {
  const interval = 500;
  const timeout = 8000;
  const maxTries = timeout / interval;
  function checkerFn(resolve, reject, tryCount=0) {
    if(tryCount > maxTries) {
      reject(`Timeout ${timeout}ms reached when searching for element`);
      return;
    }
    setTimeout(() => {
      const el = selectorFn();
      // null check is for document.querySelector, zero check is for jQuery()
      if(el !== null && el.length !== 0) {
        resolve(el);
        return;
      }
      tryCount += 1;
      checkerFn(resolve, reject, tryCount)
    }, tryCount === 0 ? 0 : interval);
  }
  return new Promise(function(resolve, reject) {
    checkerFn(resolve, reject)
  });
}

It seems weird to me that I define a function within a function, wonder if this can be maybe done with one function or if there are some other issues.

This function will certainly do the job. Couple of nitpicks:

You might consider replacing el !== null && el.length !== 0 with el && "nodeType" in el
Every time the elementFinder is called, you are defining a new function in memory called checkerFn, instead you can reuse it, but probably you'll have to pass interval and maxTries as a parameter to it:

const finder = function(){
   function elementFinder (..){
      ...
   }
   function checkerFn (..) {
      ...
   }
   return elementFinder;
}();

Consider switching to requestAnimationFrame (rAF); for tasks/animations that does not have to be executed with sub-millisecond precision can be handled by rAF. The good thing with rAF is that if the tab is out of focus, it won't run.
If you are simultaneously searching for several elements at the same time, multiple setTimeout/rAF calls will have to be executed. For a couple of elements/tasks this is not an overhead, but if you have many, it makes sense to execute 1 setTimeout/rAF per 'tick' and process the tasks in there. For this you will need to modify your code to store tasks in some sort of ledger.
I see that you are using Promises, which is a great idea, perhaps to return a thenable from your function which you can execute other tasks once the element is available. The problem with Promises is that you cannot 'break' out of it, you'll have to add some sort of breaking mechanism to your code. Also if you want to seamlessly support evergreen and older browsers, you'll need a Promise polyfill.
Although Promises bring a lot of convenience, they certainly bring an overhead (see perf differences between Native Promises and Bluebird's version). If the sole purpose is to return a thenable for constructing chains, this can certainly be implemented using only setTimeout/rAF. This alleviates the need for polyfill for olderbrowsers.


Parse and write data from/to a xml file I recently started to learn Python for the purpose of making my life as a network engineer easier when performing repetitive tasks.

The script below does parse a huge inventory.xml file I get from a reporting tool to find the ID of ports I want it to and then writes back to the XML file a structure representing a cable between the two network ports. I do convey the name of the ports I want to link in a basic CVS file with 2 columns.

<link>
<portid1>ca1cfd39-cf8f-4d69-a52b-d06357da66b1</portid1>
<portid2>79d8c32a-8891-4bd4-a0a1-1a6e1b333a4b</portid2>
</link>

This is the code I tried to comment to the best of my ability:

#links.csv file shoud be made as follows :
#port1,port2
#port1,port2
#   .
#   .
#   .
from lxml import etree
import csv
import fileinput

#preparing variables
result=[]
trigger=False
tree=etree.parse("inventory.xml")

#parsing the csv file containing the port names to link together
with open('links.csv', 'rb') as csvfile:
    fichier=csv.reader(csvfile)
    for row in fichier:
        name1=row[0] #name of the first port
        name2=row[1]
        portid1=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
        portid2=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))
        portid1=portid1[2:-2] #output was surrounded by [""] so i cut the first two and last two
        portid2=portid2[2:-2]
        result.append(("<link>\n<portid1>{}</portid1>\n<portid2>{}</portid2>\n</link>\n").format(portid1, portid2)) #store data

result="".join(result)
#search the file until we find the <links> line, then we print the result
for line in fileinput.input('inventory.xml', inplace=1):
    if line.startswith('    <links>'):
        trigger=True
    else:
        if trigger:
            print result
        trigger=False
    if line.rstrip():
        print line, #wannabe pretty print

The goal of posting here is mainly to make my code as readable to others and as standard as possible, because I know the work I do will probably be read of tweaked by others in the future.

I don't know anything about phyton, but I will throw my 5 ct in anyway.

Adding horizontal spaces between assignments to variables/properties and vertical spacing between related parts will make your code more readable like so

for row in fichier:
        name1 = row[0] #name of the first port
        name2 = row[1]

        portid1 = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
        portid2 = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))

        portid1 = portid1[2:-2] #output was surrounded by [""] so i cut the first two and last two
        portid2 = portid2[2:-2]

        result.append(("<link>\n<portid1>{}</portid1>\n<portid2>{}</portid2>\n</link>\n").format(portid1, portid2)) #store data

If you need a comment about a variable, then this variable is poorly named. So better name the variable in a meaningful name and remove the comment.

name1=row[0] #name of the first port
name2=row[1]

vs.

first_port_name = row[0]
second_port_name = row[1]

The same approach I would use for

portid1=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
portid2=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))

like so

first_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
second_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))

So the mentioned points together would look like so

for row in fichier:
        first_port_name = row[0]
        second_port_name = row[1]

        first_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
        second_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))

        first_port_id = first_port_id[2:-2] #output was surrounded by [""] so i cut the first two and last two
        second_port_id = second_port_id[2:-2]

        result.append(("<link>\n<portid1>{}</portid1>\n<portid2>{}</portid2>\n</link>\n").format(first_port_id , second_port_id )) #store data


LINQ queries to generate a report on accrued work time on projects The following code fetches data from a MySql database to generate a simple report on accrued work time on projects.

I've been away from Entity Framework for a while and looking at this code you can see that the creation of ListProject is basically repeated 3 times.

Is there any way to refactor this so I can reuse the duplicate parts?

I've thought about a factory method, but that won't work because it can't be translated to SQL by the expression interpreter.

    var project = entities.Tickets
        .Where(p => p.ID == id)
        .Select(p => new ListProject
        {
            Id = p.ID,
            Name = p.ueberschrift,
            AssignedEmployee = new ListEmployee
            {
                Id = p.AssignedEmployee.ID,
                Name = p.AssignedEmployee.name
            },
            WorkUnits = p.WorkUnits
                .Select(wu => new ListProjectWorkUnit
                {
                    Duration = wu.dauer,
                    PauseDuration = wu.pauseDauer,
                    Employee = new ListEmployee
                    {
                        Id = wu.AssignedEmployee.ID,
                        Name = wu.AssignedEmployee.name
                    }
                })
                .ToList()
            })
            .Single();

    project.Tickets = entities.Tickets
        .Where(p => p.vonProjekt == id)
        .Where(p => p.phaseID == 0)
        .OrderBy(p => p.ueberschrift)
        .Select(p => new ListProject
        {
            Id = p.ID,
            Name = p.ueberschrift,
            AssignedEmployee = new ListEmployee
            {
                Id = p.AssignedEmployee.ID,
                Name = p.AssignedEmployee.name
            },
            WorkUnits = p.WorkUnits
                .Select(wu => new ListProjectWorkUnit
                {
                    Duration = wu.dauer,
                    PauseDuration = wu.pauseDauer,
                    Employee = new ListEmployee
                    {
                        Id = wu.AssignedEmployee.ID,
                        Name = wu.AssignedEmployee.name
                    }
                })
                .ToList()
        })
        .ToList();

    project.Phases = entities.projekt_phase
        .Where(p => p.projektID == id)
        .OrderBy(p => p.rang)
        .Select(p => new ListProjectPhase
        {
            Id = p.ID,
            Name = p.name,
            Tickets = p.Tickets
                .OrderBy(t => t.ueberschrift)
                .Select(t => new ListProject
                {
                    Name = t.ueberschrift,
                    Id = t.ID,
                    AssignedEmployee = new ListEmployee
                    {
                        Id = t.AssignedEmployee.ID,
                        Name = t.AssignedEmployee.name
                    },
                    WorkUnits = t.WorkUnits
                        .Select(wu => new ListProjectWorkUnit
                        {
                            Duration = wu.dauer,
                            PauseDuration = wu.pauseDauer,
                            Employee = new ListEmployee
                            {
                                Id = wu.AssignedEmployee.ID,
                                Name = wu.AssignedEmployee.name
                            }
                        })
                        .ToList()
                })
                .ToList()
        })
        .ToList();

I've thought about a factory method, but that won't work because it
  can't be translated to SQL by the expression interpreter.

Why don't you call (as suggested by t3chb0t) AsEnumerable() right before your factory method? You would perform the factory method in memory.

 var project = entities.Tickets
        .Where(p => p.ID == id)
        .AsEnumerable()   // <-- loads the tickets into memory
        .Select(p => myFactoryMethod(p))
        .Single();


XML DTD for downloadable stories I want to make sure that I am writing the code correctly and fully understand what is needed. My biggest concern is what tags to use, (? * | etc.)

  The element stories
  5.1.  The element stories contains 0 or more story elements.
  The element story
  6.1.  The element story has an enumerated attribute updated that can only have the values true, false, or unknown. The default is true.
  Represents whether or not the story was updated since the last time
  the feed was downloaded.
  6.2.  The element story’s first element is the url element. Must appear exactly once. A link to the story.
  6.3.  The element story’s second element is the title element. Must appear exactly once.
  6.4.  The element story’s third element is the preview element. Must appear exactly once. A short introduction to the story. Usually, about
  1 sentence.
  6.5.  The element story’s element content is optional, but if present, appears no more than once. The story. Some sites allow the story text
  in feeds, some don’t. That is why it is optional
  6.6.  The element story’s element video is optional, but if present, appears no more than once. A link to an optional video.
  6.6.1.    The order of the elements content and video does not matter. If both are present, content can be first or video can be first.
  6.7.  The element story’s last element publication-date is optional, but if present, appears no more than once and must be the last
  element. The date the story was published.
  The element title
  7.1.  The element title can contain text

My code:

<!ELEMENT stories (stories*)>
<!ELEMENT story (update true|false, url?, title?, preview?, content?, video?, publication-date?>
<!ELEMENT title >

There's an unmatched ( in the declaration of the contents of story.  I recommend that you actually test the DTD using a validating parser such as OpenSP before posting code for review.

6.1 is not satisfied: you have declared content for story but no attributes.

6.2-6.4 are not satisfied: the url, title and preview elements must each appear exactly once.

6.6.1 is not satisfied: you only allow content before video and not video before content.

7.1 is not satisfied, because title has been declared as an empty element.


Ltree inverse crossing function using high order functions I was solving some Haskell exercises as a way to refresh some of the language concepts.

I encounter the following exercise:

data LTree a = Leaf a | Fork (LTree a) (LTree a)

crossing :: LTree a -> [(a,Int)]
crossing (Leaf x) = [(x,0)]
crossing (Fork e d) = map(\(x,y) -> (x,y+1)) (crossing e ++ crossing d)

The above function basically takes a Ltree and turns into a list of pars (Lists the tree leaves, along with their depth).

The exercise is to make a function build :: [(a,Int)] -> LTree a, that does the inverse of the crossing function, such as build (crossing a) = a for any 'a' tree.

What I have done so far:

build :: [(a,Int)] -> LTree a
build l = fst (multConvert (map (\(x,n) -> (Leaf x, n)) l))

multConvert :: [(LTree a,Int)] -> (LTree a,Int)
multConvert [x] = x
multConvert l = multConvert (convert l)

convert :: [(LTree a,Int)] -> [(LTree a,Int)]
convert [] = []
convert [x] = [x]
convert ((a,b):(c,d):xs) | b == d = ((Fork a c),(b-1)):xs
                         | otherwise = (a,b):convert ((c,d):xs)

Basically, I turn the list produce by the crossing into an list of (LTree,a) , for example [(3,3),(4,3)..] becomes [(Leaf 3, 3), (Leaf 4, 3)...]. The convert function will take this  list of (LTree,a) and turn consecutive elements with the same depth into a Fork with both elements. For example [(Leaf 3, 3), (Leaf 4, 3), (Leaf 5, 2)...] becomes [(Fork (Leaf 3) (Leaf 4),2), (Leaf 5, 2)...]. Note, that the new produced element have a depth-1.

multConvert function will apply the above idea on the list, over and over, until this list have only one element. This element will represent the original tree. In other words, my algorithm builds the Ltree using a bottom up approach.

I would like to know if there is a more robust way to solve this problem. For example using high order functions, instead of explicitly recursive (possibly using foldr).

I think this is a continuation-passing solution (simple parser) for writing build:

build :: [(a,Int)] -> LTree a
build xs = parseAt 0 xs fst

-- The first Int is the depth of the "root" of parseAt
parseAt :: Int -> [(a,Int)] -> ( (LTree a, [(a,Int)]) -> r ) -> r
parseAt r [] k = error "input too short"
parseAt r xs@((a,n):rest) k =
  case compare r n of
    GT -> error "depth too small"
    EQ -> k (Leaf a,rest)
    LT -> parseAt (succ r) xs (\ (left,rest1) ->
            parseAt (succ r) rest1 (\ (right,rest2) ->
              k (Fork left right, rest2)))


Very simple in-memory database I've implemented a very simple in-memory "database" just as an exercise. I wanted to see if there's anything obvious I should be doing that fits more of the Haskell way of doing things.

import qualified Data.Map as M

type History = [String]

data Result = Result (Maybe String) Bool DB

data Command = Command { name  :: String
                       , key   :: Maybe String
                       , value :: Maybe String
                       }

data DB = DB (M.Map String String)

cmdKey c   = let Just s = key   c in s
cmdValue c = let Just s = value c in s

runSetCmd :: Command -> DB -> IO (Maybe String, DB)
runSetCmd c db@(DB map) = do
    let newMap = M.insert (cmdKey c) (cmdValue c) map
    return $ (Nothing, DB newMap)

runGetCmd :: Command -> DB -> IO (Maybe String, DB)
runGetCmd c db@(DB map) = return $ (M.lookup (cmdKey c) map, db)

execCmd :: DB -> Command -> IO Result
execCmd db c@(Command name key value) = do
    (output,newDB) <- case name of
        "set" -> runSetCmd c db
        "get" -> runGetCmd c db
        _     -> return (Nothing, db)
    return $ Result output end newDB

  where
    end = case name of
              "end" -> True
              _     -> False

getCmd = getLine >>= return . parseCmd

parseCmd :: String -> Command
parseCmd s =
    case words s of
        (name:key:value:_) -> Command name (Just key) (Just value)
        (name:key:[])      -> Command name (Just key) Nothing
        (name:[])          -> Command name Nothing Nothing

displayResult :: Result -> IO Result
displayResult r@(Result (Just s) _ _) = putStrLn s >> return r
displayResult r                       = return r

continue :: Result -> IO ()
continue (Result _ end db) = if end then return () else repl db

repl state = getCmd >>= execCmd state >>= displayResult >>= continue

main = repl (DB M.empty)

First of all, as Karl said, you should minimize the code in IO monad and make sure most of your code is in pure functions

Second your Command type is not properly designed

data Command = Command { name  :: String
                         , key   :: Maybe String
                         , value :: Maybe String
                       }

Having the name as String is not cool. You could have a type something like

data CommandType = Get | Set

...but still your command type has too many invalid combinations which the type system cannot help you with (for example: name="get", key="xyz", value="asdfasdf"). Your type should be designed such that you avoid these combinations and you make the type system work for you.

Given this I would define the Command type like this:

data Command = Invalid | End | Get String | Set String String

if your type is like this then execCmd will become:

exec (Get key) = ....
exec (Set key value) = ...
exec Exit = ...
exec Invalid = ...

If you only address these two issues you will see a lot of simplification in your code. Other things would be:

Using the State monad will help you with the passing of the current db state between the functions operating on it. If you do this then passing the state will not be your concern and your result could be either a tuple or a record:

data Result = Result { output: String ; end:Bool}

Then your displayResult will look like this:

UPDATE

If you don't want the a command to output anything then the output field in Result could have the type "Maybe String" instead of String. \

data Result = Result { output: Maybe String ; end:Bool}

Then you will have to choose only the Just outputs before you display the results.


Is this the correct approach to animation in Java? I was fiddling with animation in Java and wrote this:

import java.awt.Graphics;
import java.awt.Image;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.ArrayList;

import javax.swing.Timer;
import javax.swing.ImageIcon;
import javax.swing.JFrame;

class Main extends JFrame implements ActionListener {
    private Timer timer = new Timer(500, this);

    private Image mario1 = new ImageIcon("C:/Users/User/workspace/Game Development/src/mario.jpg").getImage();
    private Image mario2 = new ImageIcon("C:/Users/User/workspace/Game Development/src/mario2.jpg").getImage();
    private int counter = 0;
    private ArrayList<Image> scenes = new ArrayList<Image>();

    public Main() {
        setSize(500, 500);
        setVisible(true);

        addScene(mario1);
        addScene(mario2);
        timer.start();
    }

    public void actionPerformed(ActionEvent e) {
        nextScene();
    }

    public void addScene(Image image) {
        scenes.add(image);
    }

    public void nextScene() {
        counter++;
        repaint();
    }

    @Override
    public void paint(Graphics g) {
        super.paint(g);
        if (!(counter < 0 || counter >= scenes.size())) g.drawImage(scenes.get(counter), 100, 100, null);
    }

    public static void main(String[] args) {
        new Main();
    }
}

Is this the correct approach or is this just bad practice?

Two recommendations:

It is better that you don't "do any action" in the constructor; create, say, a .start() method and do new Main().start() in main();
rather than testing on count in animation, use this, where nrImages is the number of images:

count = (count + 1) % nrImages;

Also, declare your list as a List, not an ArrayList: use interfaces when available.


Picking a pattern based on employee type or department I have an if else statement like this deciding the pattern to be followed based on type or department of employee.

The if else block goes like this. I am planning to simplify the if else block to a better format.

 public static Pattern decidePattern(Employee employee) {
        try {
            if ("individual".equalsIgnoreCase(employee.getType()) {
                return new IndivualPattern();
            } else if ("multiple".equalsIgnoreCase(employee.getType()) {
                return new MultiplePattern();
            } else if ("combined".equalsIgnoreCase(employee.getType()) {
                return new CombinedPattern();
            }else if ("financial".equalsIgnoreCase(employee.getDepartment()) {
                return new FinancialPattern();
            } else if ("hr".equalsIgnoreCase(employee.getDepartment()) {
                return new HrPattern();
            } else if ("facilities".equalsIgnoreCase(employee.getDepartment()) {
                return new FacilitiesPattern();
            } else {
                return new DefaultPattern();
            }
        } catch (Exception e) {
            return new DefaultPattern();
        }
    }

All patterns are inherited from common interface. Can I break this into a simpler one?

You can't really do much to simplify this. There are 6 cases plus one default which means 7 different results, which means 7 lines to return and 6 conditions.

Unless you don't need to return a new thing every time. Assuming Pattern is from java.util.regex, this is the case. Then you can use something like

import com.google.common.collect.ImmutableMap;

private static final Map<String, Pattern> PATTERNS_FOR_TYPE =
    ImmutableMap.of(
    "individual", new IndivualPattern(),
    "multiple", new MultiplePattern(),
    "combined", new CombinedPattern());
private static final Map<String, Pattern> PATTERNS_FOR_DEPARTMENT = ...;

public static Pattern decidePattern(Employee employee) {
    Pattern result = PATTERNS_FOR_TYPE
            .get(employee.getType().toLowerCase(Locale.ENGLISH));
    if (result!=null) {
        return result;
    }
    Pattern result = PATTERNS_FOR_DEPARTMENT
        .get(employee.getDepartment().toLowerCase(Locale.ENGLISH));
    if (result!=null) {
        return result;
    }
    return new DefaultPattern();
}

But I wouldn't call it a big improvement. What you should do instead or in addition to it, is to get rid of the Strings, that's what enums are for.

In case you insist on Strings, you should normalize them ASAP. No equalsIgnoreCase, no toLowerCase anywhere but when when you encounter the inputs the first time.

Concerning catch Exception, no method can really throw an all possible Exceptions and you should be as specific as possible.

You can (and most probably should) use enums. The annotations come from project Lombok and do exactly what they say (you can do it manually, too).

@RequiredArgsConstructor @Getter
enum Type {
    INDIVIDUAL(new IndivualPattern()),
    MULTIPLE(new MultiplePattern()),
    COMBINED(new CombinedPattern()),
    ANOTHER(null),
    YET_ANOTHER(null);

    private final Pattern pattern;
}

... enum Department ...

public static Pattern decidePattern(Employee employee) {
    Pattern result = employee.getType().getPattern();
    if (result!=null) {
        return result;
    }
    ...
}

Not always it is a good idea to define the pattern in the enum itself. Then you can use a switch

public static Pattern decidePattern(Employee employee) {
    switch (employee.getType()) {
        case INDIVIDUAL: return new new IndivualPattern();
        ...
        default: // (just to prevent "missing label" warning)
    }
    switch (employee.getType()) {
        ...
    }
    ...
}


Sorting an array in alphabetical order I'm looking to get some feedback on this program I wrote - ways I could make it simpler or best practices methods.
Chapter 7:

Let's write a program which asks us to type in as many words as we
want (one word per line, continuing until we just press Enter on an
empty line), and which then repeats the words back to us in
alphabetical order. OK?
...
Assignment: Write the program we talked about at the very beginning of
this chapter. Hint: There's a lovely array method which will give you
a sorted version of an array: sort. Use it!

Here's the program I wrote:
puts "This program will take your words and sort them alphabetically"
puts ""
puts "Type in a word and press enter. When you are done, press enter on an empty line to launch program."

inputArray = []
while (inputWord = gets.chomp) != ""
    inputArray.push inputWord
    puts "Current List is: " + inputArray.join(', ')
end
print "Your list in alphabetical order is: " + inputArray.sort.join(', ') + "."

Flambino has already pointed out the issues in your code, so I'll just show how to tackle the problem with a different approach. A declarative and functional code (as opposed to imperative) favours immutability and known abstractions over manual control flow and in-place updates. Uusing lazy from Ruby 2 and String#present? from active_support (so we can write the more declarative code we can) it would look like this:

require 'active_support/core_ext/string'
sorted_words = $stdin.lines.lazy.map(&:strip).take_while(&:present?).sort
puts("Your list in alphabetical order is: #{sorted_words.join(', ')}.")


Blurring a given image using moving average in Python 3 I've tried solving the following HackerRank question:

  Write a function blur_image() that takes as a parameter an image in the form of a nested list A and blurs it by averaging each pixel value with four of its neighboring pixels (Top, Bottom, Right, and Left).  Note: not all of the neighbors are available in boundary cases.  You have to write suitable conditions accordingly.

My Solution:

import ast
A = input()
A = ast.literal_eval(A)

def blur_image(a):
    result = []
    for i in range(len(a)):
        row = []
        for j in range(len(a[i])):
            total, count = a[i][j], 1
            if i + 1 < len(a): total, count = total + a[i+1][j], count + 1
            if j + 1 < len(a[i]): total, count = total + a[i][j+1], count + 1
            if i - 1 > -1: total, count = total + a[i-1][j], count + 1
            if j - 1 > -1: total, count = total + a[i][j-1], count + 1
            row.append(round(total/count, 2))
        result.append(row)
    return result

print(blur_image(A))

I would appreciate any suggestions and advice you can give me to improve this solution. Please note that my focus is to solve this without using any modules.

First, I think this code arguably abuses tuple destructuring to declare and reassign variables. Declaring multiple variables on a line generally hurts readability, just to save a line/some keystrokes. I would write everything out fully, even if that comes with the cost of verbosity. I'd also space it out a bit:

def my_blur_image1(a):
    result = []
    for i in range(len(a)):
        row = []
        for j in range(len(a[i])):
            total = a[i][j]
            count = 1
            if i + 1 < len(a):
                total += a[i+1][j]
                count += 1

            if j + 1 < len(a[i]):
                total += a[i][j+1]
                count += 1

            if i - 1 > -1:
                total += a[i-1][j]
                count += 1

            if j - 1 > -1:
                total += a[i][j-1]
                count += 1

            row.append(round(total/count, 2))

        result.append(row)

    return result

Twice, you have something along the lines of

lst = [] # Create a list
for i in range(len(a)):
    res = # Calculate some result
    lst.append(res)

I think the logic could be broken up, and could make use of some list comprehensions. This is basically the scenario that list comprehensions (and map) are intended for. Iterating over a list to produce a new list is a very common operation.

I'm not necessarily recommending this way, but it does show an alternative, more functional way of approaching the problem. I'll say that my way ended up a fair bit slower than yours. On my machine, yours takes roughly 14 seconds for a 2000x2000 matrix, while my version takes 25 seconds unfortunately. You didn't tag performance though :D

# Returns whether or not i,j is inbound for a given a matrix
def inbounds(i, j, a):
    return 0 <= i < len(a) and \
           0 <= j < len(a[i])

# Returns the inbound pixel values on and surrounding the given point
def inbound_pixels_around(i, j, a):
    # I figured it was best to hard-code the indices instead of using several "if"s like you had done
    # That way, I can make use of looping to reduce some duplication
    # If diagonal pixels were included, this could be generated by another comprehension instead
    indices = [(i, j), (i - 1, j), (i + 1, j), (i, j - 1), (i, j + 1)]

    # Iterate over the indices.
    # Remove the ones that are  out of bounds, and use the inbound ones to index the list
    return [a[i][j] for (i, j) in indices if inbounds(i, j, a)]

def my_blur_image2(a):
    # A 3D-array. Each pixel has been replaced by a list of inbound neighbor values
    inbound_neigh_rows = [[inbound_pixels_around(i, j, a) for j in range(len(a[i]))]
                          for i in range(len(a))]

    # Then iterate ever each set of neighbors, and divide the sum of the neighbors by their length
    # This does away with needing an explicit "count" variable
    return [[sum(neighs) / len(neighs) for neighs in row]
            for row in inbound_neigh_rows]

I'm making fairly extensive use of list comprehensions here. I'm using them to filter out non-inbound cells in inbound_pixels_around using inbounds, and to generate neighbors and their average values in my_blur_image2.

test_data = [[1, 2, 3],
             [4, 5, 6],
             [7, 8 ,9]]

print(blur_image(test_data))
print(my_blur_image1(test_data))
print(my_blur_image2(test_data))

[[2.33, 2.75, 3.67], [4.25, 5.0, 5.75], [6.33, 7.25, 7.67]]
[[2.33, 2.75, 3.67], [4.25, 5.0, 5.75], [6.33, 7.25, 7.67]]
[[2.3333333333333335, 2.75, 3.6666666666666665], [4.25, 5.0, 5.75], [6.333333333333333, 7.25, 7.666666666666667]]

Updated to make use of comparison chaining. Thanks @Mathias.


Searching files in a directory for a string I have the following static class that enumerates directories in a folder, then searches each file in the folder (it seems to only work with text files even thought I don't explicitly specify that) for a given string and returns an IEnumerable that holds the results.

It takes about 15 seconds to go through 40 text files that are about 250kb in size and I think it could be faster. Could I use a better algorithm, or is there a better method of achieving this?

public static class LogFileReader
{
    public static IEnumerable<string> GetLines(string path, string searchterm)
    {

        var dirs = Directory.EnumerateDirectories(path);
        List<string> thelines = new List<string>();

        foreach (var dir in dirs)
        {

            var files = Directory.EnumerateFiles(dir);

            foreach (var file in files)
            {
                using (StreamReader sr = new StreamReader(file))
                {
                    string line = string.Empty;
                    while ((line = sr.ReadLine()) != null)
                    {
                        if (line.IndexOf(searchterm, StringComparison.CurrentCultureIgnoreCase) >= 0)
                        {
                            thelines.Add(line);
                        }
                    }

                }
            }

        }

        return thelines;
    }
}

Currently your method does two things. It searches the directory structure and anylizes the files at the same time. This should be separated so that you can maintain each feature separately without affecting the other. For example should you want to search the directories recursively you now only need to change the GetFileNames method without thinking about reading the files.

public static IEnumerable<string> FindLines(this IEnumerable<string> fileNames, Func<string, bool> predicate)
{
    return fileNames.Select(fileName =>
    {
        using (var sr = new StreamReader(fileName))
        {
            var line = string.Empty;
            while ((line = sr.ReadLine()) != null)
            {
                if (predicate(line))
                {
                    return line;
                }
            }
        }
        return null;
    })
    .Where(line => !string.IsNullOrEmpty(line));
}

public static IEnumerable<string> GetFileNames(this string path)
{
    return
        Directory.EnumerateDirectories(path)
        .SelectMany(Directory.EnumerateFiles);
}

If you want you can make it parallel later with:

var results =
    @"c:\foo".GetFileNames()
    .FindLines(line => line.IndexOf("bar", StringComparison.CurrentCultureIgnoreCase) >= 0)
    .AsParallel()
    .ToList();


Wrath Cookie Switch for Auto Golden Cookie Clicker Sometimes you feel like a wrath cookie and sometimes you don't.

Here is a function that I created that will run on an interval, so all I have to do is set a boolean when I want to switch between Wrath + Gold cookies and just Gold cookies.

// allows me to turn on and off the clicking of the Wrath Cookies
var wrathCookieSwitchOn = false;

var SpecialCookieClicker =
    setInterval(function() {
        if (wrathCookieSwitchOn){
            Game.shimmers.forEach(function(shimmer) {
                if (shimmer.type == "golden") { shimmer.pop() }
            })
        } else {
            Game.shimmers.forEach(function(shimmer) {
                if(shimmer.type == "golden" && shimmer.wrath == 0)
                {
                    shimmer.pop()
                }
            })
        }
    }, 500);

All I have to do is change the variable value in the Developer Console in order to affect this code.

How could I do this a little better?

How could I do this a little better?

Initially my eyes are directed at the two branches of the conditional and ways to eliminate the redundancy. Would it be too long of a line to move that conditional check of wrathCookieSwitchOn inside the if inside the forEach?
Game.shimmers.forEach(function(shimmer) {
    if (shimmer.type == "golden" && (wrathCookieSwitchOn || shimmer.wrath == 0) {
        shimmer.pop()
    }
})

It must be asked whether the type of shimmer.wrath needs to be an integer. If any falsely value will suffice, then shimmer.wrath == 0 could be simplified to !shimmer.wrath, otherwise, use strict equality: shimmer.wrath === 0
Beyond that, the only idea I have possibly removing the closure by some technique like currying or just defining a function... or if ES-2015 is an option, simplify it into an arrow function.
Update
In response to your answer, I shall reveal another option I considered, which some argue might be less readable, but yes, like you typed: “if we are going to one-line it, let's one-line it!”:
Game.shimmers.forEach(function(shimmer) {
    shimmer.type == "golden" && (wrathCookieSwitchOn || shimmer.wrath == 0) && shimmer.pop();
});

Which would lend itself well to an arrow function (if permissible).


Monkey-patching Jasmine's it() function to log errors The introduction of async native support in Jasmine doesn't log the errors (i.e., specific line number where the error occurs) to the console. So, to get around this behavior and to make the errors appears in the console, I came up with this following solution to patch Jasmine.

I have the following function which I implemented to monkey patch the Jasmine. And I would like to reuse the same function for other functions like beforeEach, beforeAll and so on..

     function patchJasmineForAsyncError(method: string) {
        let oldFn = window[method];
        if (method === 'it' || method === 'fit') {
            window[method] = function (desc: string, fn: Function) {
                    oldFn(description, (done) => {
                    let p = fn();
                    if (p instanceof Promise) {
                        p.then(() => done()).catch(e => {
                            console.error(e);
                            done.fail(e);
                        });
                    }
                    });
            }
        } else {
            window[method] = function (axn: Function) {
                oldFn((done) => {
                    let p = axn();
                    if (p instanceof Promise) {
                        p.then(() => done()).catch(e => {
                            console.error(e);
                            done.fail(e);
                        });
                    }
                });
            }
        }
}

As you can see the second part of the function (the else block) has quite a lot of duplication. I was wondering if there is an elegant way to minimize this and implement the function.

Note: the it and fit functions have two arguments desc and fn.. but the beforeAll, beforeEach.. etc have only one argument axn.

Just factor it out into a closure

function patchJasmineForAsyncError(method: string) {
    let oldFn = window[method];
    function create_logger(fn: Function) {
        return (done) => {
            let p = fn();
            if (p instanceof Promise) {
                p.then(done).catch(e => {
                    console.error(e);
                    done.fail(e);
                });
            }
        }
    }

    if (method === 'it' || method === 'fit') {
        window[method] = function(desc: string, fn: Function) {
            oldFn(desc, create_logger(fn));
        }
    } else {
        window[method] = function (axn: Function) {
            oldFn(create_logger(axn));
        }
    }
}

We can clean this up a bit more by getting rid of some magic. I also got rid of the () => done() because that's the same as done.

function patchJasmineForAsyncError(method: string) {
    let oldFn = window[method];
    function create_logger(fn: Function) {
        return (done) => {
            let p = fn();
            if (p instanceof Promise) {
                p.then(done).catch(e => {
                    console.error(e);
                    done.fail(e);
                });
            }
        }
    }

    let allowed_methods = ['it', 'fit'];
    if (allowed_methods.includes(method)) {
        window[method] = function(desc: string, fn: Function) {
            oldFn(desc, create_logger(fn));
        }
    } else {
        window[method] = function (axn: Function) {
            oldFn(create_logger(axn));
        }
    }
}

We can also reduce some of the duplication in assigning to window[method] if we're okay with adding a slight slow-down to the function, which should be fine.

function patchJasmineForAsyncError(method: string) {
    let oldFn = window[method];
    function create_logger(fn: Function) {
        return (done) => {
            let p = fn();
            if (p instanceof Promise) {
                p.then(done)
                 .catch(e => {
                    console.error(e);
                    done.fail(e);
                });
            }
        }
    }
    let allowed_methods = ['it', 'fit'];
    window[method] = function() {
        if (allowed_methods.includes(method)) {
            oldFn(arguments[0], create_logger(arguments[1]));
        } else {
            oldFn(create_logger(arguments[0]));
        }
    }
}


Generate n appointments in the next m months I work with an API where I need to write code to generate n appointment dates in the coming m months (from today). For the purpose, I write a Random date generator class provided. One condition is I need to use the java.sql.Date for the class.

import java.sql.Date;
import java.time.LocalDate;
import java.util.Calendar;
import java.util.GregorianCalendar;
import java.util.Objects;
import java.util.Random;
public class RandomDate {

    private final Random random;

    private final Date currentDate;
    private final int months;

    public RandomDate(Random random, Date currentDate, int months) {

        this.random = random;
        this.currentDate = currentDate;
        this.months = months;
    }

    public Date getRangeEndDate() {

    Calendar calendar = new GregorianCalendar();

    calendar.setTimeInMillis(this.currentDate.getTime());
    calendar.add(Calendar.DATE, this.months * 30);

    return new Date(calendar.getTimeInMillis());
}

public Date generateRandomDate(Date endDate) {

    int start = (int) this.currentDate.toLocalDate().toEpochDay();
    int end = (int) endDate.toLocalDate().toEpochDay();

    long randomDay = start + random.nextInt(end - start);
    return Date.valueOf(LocalDate.ofEpochDay(randomDay));
}

}

I have an issue that within the code I use the time classes from 3 different packages (java.sql, java.time, and java.util). How can I write it more elegantly?

The essential part of your code is:

static LocalDate random(LocalDate start, int months, Random rnd) {
    LocalDate end = start.plusMonths(months);
    int days = (int) ChronoUnit.DAYS.between(start, end);
    return start.plusDays(rnd.nextInt(days + 1));
}

I have no idea why you need more code than this to express the idea.

I don't see any practical reason to use either the old and ugly java.util.Date or the even uglier java.util.Calendar or the inappropriate java.sql.Date (since this code has nothing to do with databases, let alone SQL). All you need are the classes from java.time, and java.time.temporal.


Prime number generator exercise I have been trying to create a class that would list prime numbers forever. I know it has already been done but I think I learned some valuable principles during my trials.

This project helped me understand (to a better extent) return values, arguments for methods, simple math arithmetic and what I personally enjoyed working with was the power of the for and if/else statements.

I would appreciate any insight into improvements that would increase this code performance and/or any obvious flaws with my code structure.

//first the class is declared
public class startListingPrimeNumbers {

    // method is created to start the prime number listing from 1 to infinity
    // and beyond
    public void startFromOneListingPrimeNumbers() {

        // Create the primary for loop using doubles so the program will
        // continue on infinitely
        for (double x = 1; x > 0.000; x = x + 1.000) {

            // The double c is for determining numbers that only have 2 factors
            // are prime
            double c = 2.000;

            // I use the primeLogger to keep track of each numbers amount of
            // factors it contains, to later compare this value to c to know if
            // it's prime or not
            double primeLogger = 0.000;

            // the multiplier is used to iterate through the for loop below
            double multiplier;

            // this is the bread and butter, I take multiplier and iterate it
            // until it is greater than x, if a remainder exists from x divided
            // by multiplier then my primeLogger adds nothing to itself
            // if there is no remainder then primeLogger adds one, meaning
            // multiplier is a factor of x.

            for (multiplier = 1.000; multiplier <= (x); multiplier = multiplier + 1.000) {
                if (x % multiplier == 0) {
                    primeLogger = primeLogger + 1.000;
                } else {
                    primeLogger = primeLogger + 0.000;
                }
            }

            // here we see if there are only 2 factors for x, and the console
            // prints the number
            if (c == primeLogger) {
                System.out.println(x + " is a prime number");
            }

            // primeLogger is set back to zero to go on to the next x value
            // based on the primary for loop description
            primeLogger = 0.000;
        }
    }
}

The major problem here is using imprecise floating point values. Not only does it slow the entire process (minor quibble), but it will produce incorrect results once the values pass the integral cutoff of the field width. If you truly want to allow calculations approaching infinity (or what? 24 bits?) you need to switch to BigInteger which can model "exact" integers of any size.

  Okay, they probably cap out at a very, very large number...much larger than doubles, but will likely last past the life of your PC.

This issue is so critical to the correctness of the solution that I'll leave it to stand on its own and leave any other review issues for others.


Write strings to a file while downloading new one simultaneous I created this console app that gets some JSON strings from a web request using a Parallel.For loop,

class Program
{
    private static readonly object ConsoleLock = new object();

    static void Main(string[] args)
    {
        //DECLARATION
        var link = "http://www.mytestrequest.com/id="
        int start = 1000;
        int end = 2000;
        var requestDid = 0;
        var requestToDo = end - start;
        ParallelOptions parallelOptions = new ParallelOptions();
        parallelOptions.MaxDegreeOfParallelism = 8;

        List<string> jsonResponses = new List<string>();
        var baseAddress = new Uri("http://www.mytestrequest.com");
        var cookieContainer = new CookieContainer();
        cookieContainer.Add(baseAddress,new Cookie("JSESSIONID", "xxx"));
        var handler = new HttpClientHandler() {CookieContainer = cookieContainer};
        var client = new HttpClient(handler) {BaseAddress = baseAddress};

        //CODE START HERE
        var task = Parallel.For((long)start, (long)end, parallelOptions, async i =>
        {
            lock (ConsoleLock)
            {
                Console.SetCursorPosition(0, 0);
                Console.Write($"Progress {requestDid++}/{requestToDo}     ");
            }

            jsonResponses.Add(client.GetStringAsync(link + i).Result);
        });

        while (!task.IsCompleted)
        {
            Thread.Sleep(1000);
        }

        Console.WriteLine("Writing to file");

        using (var streamWriter = new StreamWriter("N:/sctest.txt"))
        {
            foreach (var sc in jsonResponses)
            {
                streamWriter.WriteLine(sc);
            }
        }
        client.Dispose();
        handler.Dispose();
    }

I was trying to achieve the best result performance for this purpose and I thought it was possible to write to a file what I already got in the list while still downloading other elements but didn't know how to achieve it, should I use ContinueWith()? But how?

I would also like to avoid to make too many requests at the same time, what I've noticed is that if I try to add the await to the GetStringAsync() it seems to make thousands of requests at same time, instead, if I wait the Result this seems to only move over following the MaxDegreeOfParallelism.

You have a lot of interesting things going on in your code. There are many ways to solve your problem, here is an approach utilizing an ActionBlock. I used .NET Core 3.1 to build the sample, but ActionBlock is available in the System.Threading.Tasks.Dataflow NuGet Package if it isn't included in your framework by default.

1) Change your entry point from static void Main(string[] args to static async Task Main(string[] args async main has been available since C#7.1 (https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-7-1). If you can't use C#7.1 or greater, you can still do it with something like DoAsyncWork().GetAwaiter().GetResult()

2) List is not guaranteed to be thread safe and I noticed you add to your list outside your lock. I elected to use a ConcurrentDictionary instead since that is guaranteed thread safe. I try to avoid my own locks whenever I can and in this case, if you get rid of the Console writes, you could dump your lock altogether.

3) I elected to use Interlocked.Increment(ref requestDid) instead of {requestDid++} and then read it with an Interlocked.Read. Interlocked operations are thread safe, so you would be okay to use these even if you removed lock (ConsoleLock) (although you are only using those variables to write to the Console anyway)

4) In an async/await world, Thread.Sleep is almost always wrong. You usually want to await Task.Delay instead. https://stackoverflow.com/questions/20082221/when-to-use-task-delay-when-to-use-thread-sleep

5) I didn't Dispose the httpclient or handler because of https://stackoverflow.com/questions/15705092/do-httpclient-and-httpclienthandler-have-to-be-disposed (although since the program is terminating anyway, it doesn't really matter in this case)

6) As far as the ActionBlock goes, there are a lot of articles out there on it, but here is an ok start https://docs.microsoft.com/en-us/dotnet/standard/parallel-programming/dataflow-task-parallel-library. Note how I set EnsureOrdered to false. If you don't need the items to be processed in order, I find it best to set it to false. In this case, since you are writing the file after you have retrieved ALL the results, you could simply sort the values coming out of the ConcurrentDictionary

I set SingleProducerConstrained to true, because in this case post is not being called concurrently. This allows the ActionBlock to make some optimizations. https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.dataflow.executiondataflowblockoptions.singleproducerconstrained?view=netcore-3.1

MaxDegreesOfParallelism can be a tough choice. I sometimes cringe when I see a hardcode for that in real world applications because the optimal value for this will likely vary based on what hardware or virtualization platform you are running on. However, the optimal value can be hard to come by, especially when running a container. This is a great example of how nuanced the choice can be. https://github.com/dotnet/runtime/issues/622

7) Finally, I have found when testing performance like this, to make sure you do so WITHOUT a debugger attached. Attached debuggers can really slow things down. I built this code in release mode and then ran it from the command line.

using System;
using System.Collections.Concurrent;
using System.Diagnostics;
using System.IO;
using System.Net;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;
using System.Threading.Tasks.Dataflow;

namespace RequestTest
{
    static class Program
    {
        static async Task Main(string[] args)
        {
            var startTime = Stopwatch.GetTimestamp();
            //DECLARATION
            int start = 1000;
            int end = 2000;
            long requestDid = 0;
            var requestToDo = end - start;

            object ConsoleLock = new object();
            ConcurrentDictionary<string, string> jsonResponses = new ConcurrentDictionary<string, string>();
            var baseAddress = new Uri("https://localhost:5001/");
            var cookieContainer = new CookieContainer();
            cookieContainer.Add(baseAddress, new Cookie("JSESSIONID", "xxx"));
            var handler = new HttpClientHandler() { CookieContainer = cookieContainer };
            var client = new HttpClient(handler) { BaseAddress = baseAddress };
            var actionBlock = new ActionBlock<string>(async x =>
            {
                var response = await client.GetStringAsync(new Uri(baseAddress + x));
                jsonResponses.TryAdd(x, response); //note this actually returns a bool to indictate if it was added successfully, as long as x is unqiue, I can't imagine a scenario where this would return false
                lock (ConsoleLock)
                {
                    Console.SetCursorPosition(0, 0);
                    Interlocked.Increment(ref requestDid);
                    Console.Write($"Progress {Interlocked.Read(ref requestDid)}/{requestToDo}     ");
                }

            }, new ExecutionDataflowBlockOptions { MaxDegreeOfParallelism = 8, EnsureOrdered = false, SingleProducerConstrained = true });

            for (var counter = start; counter<end; counter++)
            {
                actionBlock.Post("api/test?requestid="+counter.ToString());
            }
            actionBlock.Complete();
            await actionBlock.Completion;
            var endRequestTime = Stopwatch.GetTimestamp();
            Console.WriteLine();
            Console.WriteLine();
            Console.WriteLine();
            Console.WriteLine();
            Console.WriteLine($"Executed Requests in {(endRequestTime - startTime)} ticks");
            Console.WriteLine("Writing to file");

            using (var streamWriter = new StreamWriter("C:\\temp\\sctest.txt"))
            {
                foreach (var sc in jsonResponses.Values)
                {
                    streamWriter.WriteLine(sc);
                }
            }

            var endFileTime = Stopwatch.GetTimestamp();
            Console.WriteLine($"Completed File write in {(endFileTime - endRequestTime)} ticks");
        }
    }
}

```


Stripping whitespace in a CSV file I am interested in removing leading/trailing whitespace in a .csv file, and I was wondering if there's a better way to execute this:

with open("csv_file.csv", "rb") as infile:
    r = csv.DictReader(infile)
    fieldnames = r.fieldnames #Creates list of fieldnames

    for row in r:
        for f in fieldnames:
            row[f] = row[f].strip()

I am fine with using this method, but I was wondering if there's a way to circumvent using a nested for loop.

I don't think you need the b flag when opening a csv file.
Python 3 doesn't let me use it. I think you can drop it.

To eliminate one level of nesting,
you can use a dict comprehension, and the update method of a dictionary:

with open("/path/to/file.csv") as infile:
    reader = csv.DictReader(infile)
    fieldnames = reader.fieldnames

    for row in reader:
        row.update({fieldname: value.strip() for (fieldname, value) in row.items()})

Notice that I also renamed the variables, as they were very poorly named.
I also removed the "r" flag from the open, as that's the default mode anyway.

I don't think there's a simpler way to do this.
I tried using a dialect, but this is the best I could get:

with open("/path/to/file.csv") as infile:
    csv.register_dialect('strip', skipinitialspace=True)
    reader = csv.DictReader(infile, dialect='strip')
    fieldnames = reader.fieldnames

    for row in reader:
        print(row)

Which does NOT work as intended: whitespace is stripped from the left of each field, but not from the right.
My first thought was actually to set delimiter=r'\s*,\s*' when registering the dialect,
but that doesn't work because delimiter must be a single character,
it cannot be a regex.
So, if you want to strip spaces from the values,
you have no choice but do it yourself.


Retrieving the lotto numbers - Node.js I'm looking for feedback as to how I've structured the async calls, and If anything should be written differently.

endpoints.js

module.exports = {
  lotto: 'https://www.norsk-tipping.no/api-lotto/getResultInfo.json?drawID=',
  keno: 'https://www.norsk-tipping.no/api-keno/getResultInfo.json?drawID=',
  extra: 'https://www.norsk-tipping.no/api-extra/getResultInfo.json?drawID=',
  vikinglotto: 'https://www.norsk-tipping.no/api-vikinglotto/getResultInfo.json?drawID=',
  joker: 'https://www.norsk-tipping.no/api-joker/getResultInfo.json?drawID=',
  eurojackpot: 'https://www.norsk-tipping.no/api-eurojackpot/getResultInfo.json?drawID='
};

service.js

var request      = require('request');
var gametypes    = require('./endpoints');

function toJSON(response){
    var data    = response.toString(),
        result  = data.match(/(^{[\s\w\W]+}$)/gm).join('');

    return JSON.parse(result);
}

function getResults(opts, callback){
    var url                     = gametypes[opts.type],
        fromDrawID              = opts.fromDrawID,
        toDrawID                = opts.toDrawID,
        numberOfRequestsWanted  = toDrawID - fromDrawID,
        numberOfRequestsDone    = 0,
        data                    = [];

    //If either the object is missing or
    //the object with no key of 'type' is passed in - return error.

    if(!opts.type){
        return callback(new Error('Missing required param: Object with key: type'), null);
    }

    if(fromDrawID && toDrawID) {
        console.log('Fetching the requested results from https://www.norsk-tipping.no...');

        while(fromDrawID <= toDrawID) {
            doRequest(url + fromDrawID++, callback);
        }
    } else {
        console.log('Fetching the latest result from https://www.norsk-tipping.no...');
        doRequest(url, callback);
    }

    function doRequest(url, callback) {

        request(url, function(error, response, body) {
            if(!error && response.statusCode === 200) {
                data.push(toJSON(body));

                if(numberOfRequestsDone++ === numberOfRequestsWanted) {
                    return callback(null, data);
                }

            } else {
                return callback(error, null);
            }
        });
    }
}

module.exports = getResults;

example usage

var lotto    = require('./service');
var options  = { type: 'vikinglotto', fromDrawID: 1, toDrawID: 500};

lotto(options, function(err, results) {
    if (err) console.log(err);

    else {
        console.log(data);
    }
});

Suggesting you move to using promises instead of callbacks. This way, you don't have to do the tango with calling callbacks, collecting results and all. You can simply replace the request module with the request-promise module.

function getResult(options){
  ...
  return doRequest(options);
}

...

lotto(options).then(results => console.log(data), err => console.log(err));

There's little merit of putting doRequest inside getResults. It's only being used in getResults but it's not exposed anyways. Suggesting you move it out into the module.

JavaScript doesn't have default argument values (yet) but you can easily do the same thing using Object.assign. Provide a default object, and merge to a new object the defaults and the one from the arguments. This way, you avoid having to do a lot of default logic and value checks.

var defaults = {...}

function getResults(options){
  var mergedOptions = Object.assign({}, defaults, options);
  ...
}

Back to promises, firing multiple async calls in parallel using callbacks will require you to collect results and keep checking if all the requests have responded. When using Promises, you can easily use Promise.all to listen to an array of promises.

var request = require('request-promise');

function getResult(options){
  ...
  var drawRange = toDrawID - fromDrawID;

  // Create an array of drawIds and map them to promises
  var promises = Array(drawRange).fill(fromDrawID).map((fromDrawID, i) => {
    return request(url + (fromDrawID + i));
  });

  return Promise.all(promises);
}


Computing the autocorrelation between 2D time series arrays in python I have a list of 2D arrays which comes from a time evolution PDE (in \$(x, y, t)\$) that I solved numerically. There are \$k\$ arrays, which all have the same dimensions, and the arrays correspond to a solution field of the PDE at times \$t = 0, 0.1, \dots, 0.1k\$. I wrote a program that takes in this list and computes the autocorrelation between the arrays for varying lag times.

From the definition of the autocorrelation function for wide-sense stationary processes, we have

$$R(\tau) = \frac{\mathbb{E}[(X_{t} - \mu)(X_{t + \tau} - \mu)]}{\sigma^{2}} = \frac{1}{\sigma^{2}} \sum_{t = 0}^{k-\tau} (X_{t} - \mu)(X_{t + \tau} - \mu)$$

Here, \$X_{t}\$ represents our 2D array at time \$t\$ and \$\mu\$, \$\sigma\$ the average and standard deviation, respectively, of the entire list of 2D arrays. My code for this is as follows

import numpy as np
import matplotlib.pyplot as plt

### Data
# List of data arrays called 'Data'.

### Expectation
New_Data = []
O = np.ones((len(Data[0][0]),len(Data[0][1]))) # Resolution of data arrays.
A = sum(Data)/len(Data)                        # Average 2D array
M = np.mean(A)                                 # Average of average 2D array
S = np.std(A)                                  # Standard deviation of average 2D array

for i in range(len(Data)):
    New_Data.append((Data[i]-M*O)/S) # (X_t-mu)/sigma

### Autocorrelation for varying lags.
Count = 1
R = []
while Count < len(New_Data)//2: # Arbitrary choice for max lag time.
    Matrix_Multiply = []
    for j in range(len(New_Data)-Count):
        Matrix_Multiply.append(np.multiply(New_Data[j],New_Data[j+Count]))
    R.append(sum(Matrix_Multiply))
    Count = Count+1

Solution = []
for k in range(len(R)):
    Solution.append(np.mean(R[k]))

t = [0.1*k for k in range(1,len(Solution)+1)]

### Plotting
plt.xlabel('Lag time')
plt.ylabel('Matrix sum')
plt.title('Field autocorrelation over time')
plt.semilogy(t, Solution)
plt.savefig('I_hope_this_works.png')

I'm sure there are some redundant steps in there, so I was wondering if anyone can see how I could make the code cleaner? Also, if anyone knows how to apply pythons numba package to make the code faster, it would be greatly appreciated, as I haven't been able to figure it out from the documentation in the hyperlink.

EDIT

In response to the comment below, here is some arbitrary data to use

Data = []
n = 20
for k in range(n):
    Data.append(np.random.randint(n, size=(n,n)))

For my data, I have ~800 arrays of size 256x256. I don't think I'm able to upload that for anyone to use.

EDIT 2

I just realised that we don't need to keep the data in the R list, so we can remove it as well as the lines

Solution = []
    for k in range(len(R)):
        Solution.append(np.mean(R[k]))

and edit the while loop to

### Autocorrelation for varying lags.
Count = 1
Solution = []
while Count < len(New_Data)//2:                # Arbitrary choice for max lag time.
    Matrix_Multiply = []
    for j in range(len(New_Data)-Count):
        Matrix_Multiply.append(np.multiply(New_Data[j],New_Data[j+Count]))
    R = sum(Matrix_Multiply)
    Solution.append(np.mean(R))
    Count = Count+1

though it doesn't provide much of a markup in terms of speed.

So here is a slightly simplified version that uses more numpy functionalities, where your solution manually iterates over the outer lists:

def autocorrelate_graipher(Data):
    Data = np.array(Data)
    A = Data.mean(axis=0)                        # Average 2D array
    New_Data = (Data - np.mean(A)) / np.std(A)
    for count in range(1, len(New_Data) // 2):
        i = np.arange(len(New_Data) - count)
        yield np.multiply(New_Data[i], New_Data[i+count]).sum(axis=0).mean()

It returns the same result as your function, albeit as a generator, so you need to call list on the output.

Unfortunately it still has the manual loop over the lags, which I at least made a for loop instead of a manual while loop.

Note that Python has an official style-guide, PEP8, which people are encouraged to follow. It recommends using lower_case for variables and functions, so Data should be data, New_Data should be new_data, A should be a, or even better average.


Thread safe static collection, is this code safe A small caching utility, can you tell if it is thread safe?

public static class CacheService
{
    private readonly static object locker = new object();
    private static Dictionary<string, Dictionary<string, object>> data = new Dictionary<string, Dictionary<string, object>>();

    public static void Insert(string partition, string key, object value)
    {
        lock (locker)
        {
            if (!data.ContainsKey(partition))
            {
                var newVal = new Dictionary<string, object>();
                newVal.Add(key, value);
                data.Add(partition, newVal);
            }
            else
            {
                if (!data[partition].ContainsKey(key))
                {
                    data[partition].Add(key, value);
                }
                else
                {
                    data[partition][key] = value;
                }
            }
        }
    }

    public static bool KeyExists(string partition, string key)
    {
        return data[partition] != null && data[partition][key] != null;
    }

    public static V Get<V>(string partition, string key)
    {
        return (V)data[partition][key];
    }

}

No, it's not safe because you're locking only when writing. You prevent multiple writings to occur simultaneously but:

Caller might call KeyExists() in the middle of Insert(), in this case:

data[partition][key] might be not null but still empty.
data or data[partition] might be in an intermediate invalid internal state.

If you add a Delete() function then reading may go through the same indeterminate states.
When calling Get() you can go through the same indeterminate states as above, plus if you add Delete() then you might be in the middle of that (also Delete() might be called after KeyExists() and before Get()).

That said let's see few other improvments you can make:

Calling ContainsKey() and then searching the value in the dictionary is pretty inefficient because search is done twice. There is a TryGetValue() method for this:

When you  have an else section you may want to keep the if condition positive, negation is a small ! symbol which may be unnoticed.

with dictionary[key] = value you do not need to call ContainsKey(), that check is already done efficiently inside the setter.

In code (C#7, if earlier version you can't use out var and sub has to be declared before):

public static void Insert(string partition, string key, object value)
{
    lock (locker)
    {
        if (data.TryGetValue(partition, out var sub))
        {
            sub[key] = value;
        }
        else
        {
            sub = new Dictionary<string, object>();
            sub.Add(key, value);
            data.Add(partition, sub);
        }
    }
}

Now add locking also in the functions to read from the dictionary and you're almost done.

You do not validate parameters. I don't know if for your usage scenarios a null value (or an empty string) for partition, key and values are allowed or not. If not then you must validate your inputs. Something as simple as:

if (partition == null)
    throw new ArgumentNullException(nameof(partition));

if (partition.Length == 0)
    throw new ArgumentException("Partition cannot be an empty string", nameof(partition));

What about a string made of spaces? In that case you might change second check to:

if (String.IsNullOrWhiteSpace(partition))
    throw new ArgumentException("Partition cannot be...", nameof(partition));

Now you can consider if Monitor (what's used by lock) is the most appropriate synchronization mechanism for your cache. If you expect multiple concurrent reads and (after an initial period) very few writes then a ReadWriterLockSlim may give you much much better performance. Profile.

I left these points to the end but they're actually the first things you should consider.

What about testing? Static methods are a nightmare for testing (if you want to replace them with something else at calling point). You may use a default singleton instance (not the perfect thing to do but better than nothing).

Your class has nothing of a cache. It's a sort of MultiMap but it has nothing to do with a cache. What I'd do? Design a not thread-safe MultiMultiMap<TKeyOuter, TKeyInner, TValue> (eventually considering to implement ILookup) which I might reuse somewhere else (even if the standard MultiMap<TKey, TValue> is probably much more common).

Now I can design a thread-safe access class around it (call it cache if you want).

Now you should start putting some caching logic inside this class, in this moment you have everything at calling point (because CacheService has too few responsibilities). What about:

A method which accepts a Func<object> instead of object itself?
A method to clear the cache.
A method to set an expiration for each item.
A method to limit the memory size used for caching.

Then you might want to abstract cache storage (memory in this case) from cache interface. What if objects you're constructing are pretty huge and expensive to build? Maybe switching to disk storage is still better than re-build them (for example if they come from network data...). Move interface to a base class (or...an interface) and derive this memory cache from that.

Did you consider to use System.Runtime.Caching.MemoryCache instead?


Animating a div element I would like to see how would you write this code for best practices, it will help me to learn from you.

This is my jQuery code I use on scroll event to addClass and animate div element.

$(document).on("scroll", function() {

    var about = $(".media-about"),
        info = $(".media-info"),
        history = $(".media-history"),
        dimension = $(".media-dimension");

    var scroll = $(this).scrollTop();

    if (scroll >= 803) {
        about.closest(".animated").addClass("tada");
    } if (scroll >= 1723) {
        info.closest(".animated").addClass("tada");
    } if (scroll >= 2096) {
        history.closest(".animated").addClass("tada");
    }
    if (scroll >= 2548) {
        dimension.closest(".animated").addClass("flipInY");
    }

    // console.log(typeof scroll);
    console.log(scroll);
});

Structurally, it looks ok to me. I'd be worried about the hard-coded numbers, though. If your page changes, you'll have to update all those.

I'm betting the numbers match the offsets of some elements, so it'd be more flexible to determine the numbers at runtime, using jQuery's .offset() function.

I would perhaps do something like this (assuming the offsets you want are for the elements you're currently selecting):

// set up your "scroll triggers" (using "klass" since "class" is a reserved word)
var scrollTriggers = [
  { selector: ".media-about", klass: "tada" },
  { selector: ".media-info", klass: "tada" },
  { selector: ".media-history", klass: "tada" },
  { selector: ".media-dimension", klass: "flipInY" }
];

// on load, get the elements and their offsets
$(function () {
  scrollTriggers.forEach(function (trigger) {
    trigger.element = $(trigger.selector);
    trigger.offset = trigger.element.offset().top;
  });

  // sort according to offset
  scrollTriggers.sort(function (a, b) { return a.offset - b.offset });
});

// on scroll, loop through the triggers
$(document).on("scroll", function () {
  var scrollTop = $(this).scrollTop(),
      trigger, i, l;
  for(i = 0 , l = scrollTriggers.length ; i++) {
    trigger = scrollTriggers[i];
    if(trigger.offset <= scrollTop) {
      trigger.element.closest(".animated").addClass(trigger.klass);
    } else {
      break; // no need to check the others; their offsets are all higher
    }
  };
});

It's still doing a some unnecessary work (finding closest(".animated") for elements you've already scrolled past and handled), but it shouldn't be a huge drain on things.

Point is that this way, it's easier to keep the JS in sync with the actual markup (no hard-coded numbers), and we've reduced duplication.

An even more automated way to do this would be to give all the relevant elements a special class, e.g. scroll-trigger, and give them a data-* attribute, e.g. data-triggered-class="tada" to hold the class name that should be added when triggered.

With that, the code becomes

var scrollTriggers = [];

// on load, get the elements and their offsets
$(function () {
  scrollTriggers = $(".scroll-trigger")
    .map(function () {
      var element = $(this);
      return {
        element: element,
        offset: element.offset().top,
        klass: element.data("triggered-class")
      };
    })
    .get()
    .sort(function (a, b) { return a.offset - b.offset });
});

// (same on-scroll handling as above)

Now, there's no need to add elements to the JS at all; just declare triggers in the HTML.


An .htaccess for our entire site We have this long .htaccess mainly rewriting our URLs. Everything works fine (except blog titles containing a ' , but we're working on that) but I'm sure it can be shorter. Feel free to review our code and propose some generic or simpler ways to rewrite the URLs instead of rewriting each and every concerned page like the example below.

Options +FollowSymlinks
RewriteEngine on

RewriteCond %{HTTP_HOST} ^oursite\.site$ [OR]
RewriteCond %{HTTP_HOST} ^www\.oursite\.site$
RewriteRule ^admin\/?$ "http\:\/\/oursite\.site\/admin\/connexion" [R=301,L]

# Rewrite spaces as long as there are more than 1 spaces in URI
RewriteRule "^(\S*)\s+(\S*\s.*)$" /$1-$2 [L,NE]

# Rewrite spaces when there is exactly 1 space in URI
RewriteRule "^(\S*)\s(\S*)$" /$1-$2 [L,R=302,NE]

#Redirect non-www to www
RewriteBase /
RewriteCond %{HTTP_HOST} ^www\.(.*)$ [NC]
RewriteRule ^(.*)$ http://%1/$1 [R=301,L]

#Rewrite urls blog
RewriteRule blog/(.*)/([0-9]+)-(.*)$ blogpost.php?brand=$1&blog_id=$2&titre=$3

#Rewrite urls brands
RewriteRule brand/([0-9]+)-(.*) brand.php?id=$1&brand=$2

#Rewrite urls shop brands
RewriteRule shop/brands/([0-9]+)-(.*) shopbrand.php?id=$1&brand=$2

#Rewrite urls shop category
RewriteRule shop/category/([0-9]+)-(.*) shopcategory.php?id=$1&cat_nom=$2

#Rewrite ADMIN profile
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/profil$ admin/profil.php?user_id=$1&brand=$2

#Rewrite ADMIN panel
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/panel$ admin/panel.php?id=$1&brand=$2

#Rewrite ADMIN panel-blog
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/blog$ admin/panel-blog.php?id=$1&brand=$2

#Rewrite ADMIN panel-blog-add
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/blog-add$ admin/panel-blog-add.php?user_id=$1&user_brand=$2

#Rewrite ADMIN panel-blog-modif
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/blog-modif$ admin/panel-blog-modif.php?id=$1&brand=$2

#Rewrite ADMIN post-modif
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/post-modif/([0-9]+)$ admin/post-modif.php?user_id=$1&user_brand=$2&blog_id=$3

#Rewrite ADMIN add
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/add$ admin/add.php?user_id=$1&user_brand=$2

#Rewrite ADMIN update
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/update/([0-9]+)$ admin/update.php?user_id=$1&user_brand=$2&blog_id=$3

#Rewrite ADMIN delete
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/delete/([0-9]+)$ admin/delete.php?id=$1&brand=$2&blog_id=$3

#Rewrite ADMIN panel-prod
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/boutique$ admin/panel-prod.php?id=$1&brand=$2

#Rewrite ADMIN panel-prod-add
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/boutique-add$ admin/panel-prod-add.php?id=$1&brand=$2

#Rewrite ADMIN panel-prod-modif
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/boutique-modif$ admin/panel-prod-modif.php?id=$1&brand=$2

#Rewrite ADMIN prod-modif
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/produit-modif/([0-9]+)$ admin/prod-modif.php?id=$1&brand=$2&prod_id=$3

#Rewrite ADMIN prod-add
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/produit-add$ admin/prod-add.php?user_id=$1&user_brand=$2

#Rewrite ADMIN prod-update
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/produit-update/([0-9]+)$ admin/prod-update.php?id=$1&brand=$2&prod_id=$3

#Rewrite ADMIN prod-delete
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/produit-delete/([0-9]+)$ admin/prod-delete.php?id=$1&brand=$2&prod_id=$3

#Rewrite ADMIN faq
RewriteRule admin/([0-9]+)-([a-zA-Z0-9\-]+)/faq$ admin/faq.php?id=$1&brand=$2

#Remove php extensions
RewriteCond %{REQUEST_FILENAME}.php -f
RewriteRule ^(.*)$ $1.php [L]

ErrorDocument 404 http://oursite.site/404

#CACHESYSTEM
<FilesMatch "(?i)^.*\.(ico|flv|jpg|jpeg|png|gif|js|css)$">
ExpiresActive On
ExpiresDefault A2592000
</FilesMatch>

#MAINTENANCE SYSTEM (if required)
#RewriteCond %{REQUEST_URI} !/maintenance.php$
#RewriteCond %{REMOTE_ADDR} !^555\.555\.555\.555
#RewriteRule $ /maintenance [R=302,L]

Unfortunately I don't think there is much you can do here because of how the targets of your regexes are different. I was going to say you could change some of these to use groups, for example:

RewriteRule blog/(.*)/([0-9]+)-(.*)$ blogpost.php?brand=$1&blog_id=$2&titre=$3
RewriteRule brand/([0-9]+)-(.*) brand.php?id=$1&brand=$2
RewriteRule shop/brands/([0-9]+)-(.*) shopbrand.php?id=$1&brand=$2

These all end with the pattern ([0-9]+)-(.*) so you almost could extract the first part and use that file, like this:

RewriteRule (.*)/([0-9]+)-(.*)$ $1?id=$2&brand=$3

However you can't because e.g. shop/brands doesn't match shopbrand.php (and not everything has brand=). There just isn't enough commonality in the paths to make this sort of thing feasible.

If your goal is to simply get rid of .htaccess, using a script and using a dispatcher with PHP logic would be an alternative, as stated in the question comments. Personally I don't think that would be any simpler though, I think it would just be moving the "problem" into a different file--one that Apache needs to start up a PHP thread/process to solve, instead of doing it on its own.

The only way, IMHO, to improve this situation would be to architect the site differently and/or abandon the old URLs so you don't need to rewrite them. If you can change what the URLs are, then I would suggest switching to a single index.php dispatcher and MVC or REST pattern (or use a framework that handles this, like Symfony).