Implementing a cancellable "infinite" loop I'm making a Windows service that watches a directory for new files to process them.

I hit a race condition between the file copying going on and me trying to process it. Going by a SO answer I wrote the following wait-and-retry method to make sure I can work with the file:

public static void WaitForFile(string file, CancellationToken token, int retryIntervalMillis = 200)
{
    while (!token.IsCancellationRequested)
    {
        try
        {
            using (new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.None))
                return;
        }
        catch
        {
            Thread.Sleep(retryIntervalMillis);
        }
    }

    throw new OperationCanceledException(token);
}

My question is: would this be a correct way to make this waiting cancellable? The token is cancelled to shut down the service, the idea is to allow for a clean (-ish) shutdown even in a case a bug means the file in question is inadvertently kept open indefinitely.

(Or if the reason the file can't be opened is some other, non-intermittent condition I haven't guarded against yet.)

For instance, is it okay to use the token this way in my method, and also expect the same token to cancel a BlockingCollection's consuming enumerator?

My intent is to base the service around the expectation it can be hard-crashed at any time, so I'm fine with an exception taking out most of the call stack leading to it.

Edit:

These are changes that I have since made to the code

const int ErrorSharingViolation = -2147024864;

public static FileStream WaitForFile(string file,
                                 CancellationToken token,
                                 FileMode mode = FileMode.Open,
                                 FileAccess access = FileAccess.Read,
                                 Action<Stream> action = null,
                                 int retryIntervalMillis = 200)
{
    while (!token.IsCancellationRequested)
    {
        try
        {
            var stream = File.Open(file, mode, access, FileShare.None);
            if (action != null)
            {
                using (stream)
                {
                    action(stream);
                }
            }
            return stream;
        }
        catch (IOException exception)
        {
            if (exception.HResult != ErrorSharingViolation)
            {
                throw;
            }
            else
            {
                Thread.Sleep(retryIntervalMillis);
            }
        }
    }
    throw new OperationCanceledException(token);
}

Your catch statement is catching any error (EVERY ERROR), which is not a good thing.  I assume there are several errors that you are anticipating, but you shouldn't catch them all.  You will run into bugs and won't know what is going on because the errors will be bulked into your cancellation.

I would catch only the exceptions that you know about and are ready for, meaning that you have a solution for that exception.

You also might want to set it so that right before WaitForFile is exited, it closes the file no matter what happens inside the method.


Bezier curves of grade n I made a function which creates Bezier curves of grade n. What do you think?

float interpolate(float n1, float n2, float prec)
{
    return n1 + ((n2-n1) * prec);
}

std::vector<Vector2f> make_bezier(const std::vector<Vector2f>& anchors, double accuracy=10000.0)
{
    if(anchors.size()<=2)
        return anchors;

    std::vector<Vector2f> end;
    end.push_back(anchors[0]);

    for(float i=0.f; i<1.f; i+=1.f/accuracy)
    {
        std::vector<Vector2f> temp;
        for(unsigned int j=1; j<anchors.size(); ++j)
            temp.push_back(Vector2f(interpolate(anchors[j-1].x, anchors[j].x, i),
                                    interpolate(anchors[j-1].y, anchors[j].y, i)));

        while(temp.size()>1)
        {
            std::vector<Vector2f> temp2;

            for(unsigned int j=1; j<temp.size(); ++j)
                temp2.push_back(Vector2f(interpolate(temp[j-1].x, temp[j].x, i),
                                         interpolate(temp[j-1].y, temp[j].y, i)));
            temp = temp2;
        }
        end.push_back(temp[0]);
    }

    return end;
}

Just a few comments:

std::vector<Vector2f> make_bezier(const std::vector<Vector2f>& anchors, double accuracy=10000.0)

You're using float for all your other floating point variables. Unless there's some really good reason to use double here (for accuracy) I'd consider changing it to a float as well.

std::vector<Vector2f> end;
end.push_back(anchors[0]);

for(float i=0.f; i<1.f; i+=1.f/accuracy)

I'd write this a bit differently. As it stands, i can accumulate rounding errors from one iteration of the loop to the next. I'd prefer to do something like:

for (int j=0; j<accuracy; j++) {
    float i = 1.0f/j;
    // ...

This way, each value of i is computed independently, and rounding from one iteration doesn't affect its value in the next.

    std::vector<Vector2f> temp;
    for(unsigned int j=1; j<anchors.size(); ++j)
        temp.push_back(Vector2f(interpolate(anchors[j-1].x, anchors[j].x, i),
                                interpolate(anchors[j-1].y, anchors[j].y, i)));

Literally all your calls to interolate (at least in this code) happen in pairs. That being the case, I think I'd rewrite it a bit to take a couple of vector2fs as parameters, and return a vector2f containing the interpolation on both the x and y components of the inputs. It also uses i as a third input. My immediate reaction would be to define interpolate as a lambda that captured i, and took the other two parameters as inputs.

vector2f interpolate(vector2f in1, vector2f const &in2) {
    in1.x = /* ... */;
    in1.y = /* ... */;
    return in1;
}

With that in place, the loop above can be reduced to a fairly simple application of std::transform, something like this:

 std::transform(anchors.begin(), anchors.end(),
                anchors.begin()+1,
                std::back_inserter(temp));

Of course, pretty much the same would happen here:

        for(unsigned int j=1; j<temp.size(); ++j)
            temp2.push_back(Vector2f(interpolate(temp[j-1].x, temp[j].x, i),
                                     interpolate(temp[j-1].y, temp[j].y, i)));

...as well.


Fibonacci sequence binary tree console logger I wrote code to show the structure of a Fibonacci sequence pattern in a graphical way. First I structure a tree given a variable the user inputs for how many iterations of the sequence they want to print. Then, it converts the numbers into the Fibonacci sequence.

My concern is that this doesn't seem very efficient, first I build a tree, then go through each node again and do the entire Fibonacci calculation on it. Instead, I was hoping for a suggestion to change the code so that it could generate the nodes and the Fibonacci number at the same time. Maybe, instead of two recursive functions, there is just one that calls itself:

fibo(num)=fibo(num-1)+fibo(num-2)

where (num-1) is the left node and (num-2) is on the right.

from binarytree import Node

 # Generates a binarytree with the correct *iterations* numbers.
def makeTree(node):
    if node.value > 1:
        node.left = makeTree(Node(node.value - 1))
        node.right = makeTree(Node(node.value - 2))
    return node

 # Converts the iterated binary tree numbers into fibonacci numbers
def fibonacci(a, b , num):
    if num > 0:
        a += b
        return fibonacci(b, a, num - 1)
    return a

a = int(input("Enter number: "))
fibTree = makeTree(Node(a))

for x in fibTree:
    x.value = fibonacci(0, 1, x.value)

print(fibTree)

Expected result if a user enters 5 as how many iterations they want.

One thing I would recommend is using memoization.  Make a dictionary of fibonacci numbers that you've already calculated (do the math once when the user gives you input).  When you go to calculate a node's fibonacci value, just do a check in the dictionary for the appropriate value.  This will avoid calculating the lower levels so much.
Actually, if you only know the level the node is on, you could instead put the fibonacci values in an array (python list) where the index of the array is the number that should be shown on that level of the tree.
Either way, there are ways to use memoization to avoid duplicate effort, and it can make a big difference in your speed for a lot of problems.


Splitting company name into 3 strings I wrote an algorithm which should cut a companies name into 3 strings.

  Input: 1 String

  Output: 3 String.

  Conditions:

  String 1 2 and 3 shall not be longer then 35 signs. If the Input string is longer then it should be cut to a length of 105.

If you have fun and be interested in algorithms it would be nice if you take a look at it.

   Public Sub CompanyCut()
  //3 output Strings
    Dim var1 As String = ""
    Dim var2 As String = ""
    Dim var3 As String = ""

    If Module1.insertdict("company").Length > 35 Then

        Dim s1 As String = Module1.insertdict("company").ToString

        If s1.Length > 105 Then
            s1 = Microsoft.VisualBasic.Left(s1, 105)
        End If

        //Split String into array at every Whitespace
        Dim pattern As String = "\s"
        Dim sa() As String = Regex.Split(s1, pattern)

        //Variables for loop
        Dim i As Integer = 0
        Dim varyn As Boolean = True
        Dim varyn1 As Boolean = False
        Dim varyn2 As Boolean = False

        //loop which fills var1 var2 and var3 with arrayfields untill size 35
           would be reached

        For i = 0 To sa.Length - 1

            If var1.Length < 35 AndAlso varyn = True Then
                If var1.Length + 1 + sa(i).Length < 35 Then
                    var1 = var1 + " " + sa(i).ToString
                Else
                    varyn = False
                    varyn1 = True
                    varyn2 = False

                End If

            End If

            If var2.Length < 35 AndAlso varyn1 = True Then
                If var2.Length + 1 + sa(i).Length < 35 Then
                    var2 = var2 + " " + sa(i).ToString
                Else
                    varyn1 = False
                    varyn = False
                    varyn2 = True

                End If

            End If

            If var3.Length < 35 AndAlso varyn2 = True Then
                If var3.Length + 1 + sa(i).Length < 35 Then
                    var3 = var3 + " " + sa(i).ToString
                Else
                    varyn2 = False

                End If

            End If

        Next

        //my idea was that if it has the same or bigger length all fields must
          be in + the whitespaces

            If var1.Length + var2.Length + var3.Length >= s1.Length Then

            Module1.insertdict("Firma") = var1
            Module1.insertdict("Name2") = var2
            Module1.insertdict("Name3") = var3

        Else

            //this occurs when the string is smaller as 105 signs but not all
              fields  of the array could be placed in the variables.

            Module1.insertdict("Failure") = "Company name need to split by user"

        End If

    Else

        Module1.insertdict("Name2") = ""
        Module1.insertdict("Name3") = ""

    End If

End Sub

  **Edit: I forgot to bring up in the Conditions that if it is possible it should be cut at >the whitespace.

  What to do now? shall I open a new Post?**

My main problem is to find a sub algorithmn which  handles this case

Else

        //this occurs when the string is smaller as 105 signs but not all
          fields  of the array could be placed in the variables.

        Module1.insertdict("Failure") = "Company name need to split by user"

End If

An easy way would be to  cut it with substring like in the answers.
a more elegant way would be to cut the first 35 chars for  the first variable and then trying to cut at whitespaces. If it doesnt works also cut the second 35 for var2. and the rest is going to var3 :)

Just for the fun of it, here's a one-liner using LINQ.

Dim names = (From i In {0, 35, 70} Where i < input.Length Select input.Substring(i, Math.Min(35, (input.Length - i))))

Example

Dim names As String() = (From i In {0, 35, 70} Where i < input.Length Select input.Substring(i, Math.Min(35, (input.Length - i)))).ToArray()
Dim var1 As String = If((names.Length > 0), names(0), String.Empty)
Dim var2 As String = If((names.Length > 1), names(1), String.Empty)
Dim var3 As String = If((names.Length > 2), names(2), String.Empty)


Task scheduler coding exercise In an interview about multithreading I was asked to write code to satisfy the requirements below:

  In the exercise you need to provide implementation of single-threaded TaskScheduler class with the following signatures:

TaskScheduler {

   TaskScheduler(List<Task> tasks)

  int getPriority()

  void execute()

  where Task object is considered interruptible if it implements Interruptible interface.

Now I have to consider the below 4 requirements also in developing the class:

  Define TaskScheduler and Task classes

  Define classes, write a test or vice versa.
  executeAllByPriority()

  Add void executeAllByPriority() method to TaskScheduler, test and implement it.
  executeAllByPriorityWithUninterruptableFirst()

  Add void executeAllByPriorityWithUninterruptableFirst() method to TaskScheduler, implement and test it.
  Bonus

  Provide multi-threaded version of TaskScheduler such that it guarantees that a higher priority task always starts execution
  before a lower priority task.

Keeping these above 4 points in consideration I have coded the below program. Please advise whether it is the correct approach.

public class MultiThreadedTaskScheduler  {

      AtomicInteger priorityCounter = new AtomicInteger(-1);
private ExecutorService executorService = Executors.newFixedThreadPool(10);

private PriorityQueue<Task> taskQueue;
private static final PriorityComparator PRIORITY_COMPARATOR = new PriorityComparator();

        public MultiThreadedTaskScheduler(List<Task> tasks){
            this.taskQueue = new PriorityQueue<Task>(tasks.size(), PRIORITY_COMPARATOR);
            this.taskQueue.addAll(tasks);
        }

        public void executeAllByPriority(){
            executorService.submit(new Runnable() {
                @Override
                public void run() {
                    for (Task task : taskQueue) {
                        executeMethod(task);
                    }
                }
            });

        }

    private void executeMethod(Task task) {
        if (task.getPriority()  > priorityCounter.get() ){
            task.execute();
            priorityCounter.getAndSet(task.getPriority());
        }

    }

        public void executeAllByPriorityWithUnInterruptableFirst(){
            executorService.submit(new Runnable() {
                @Override
                public void run() {
                    PriorityQueue<Task> interruptedQueue = new PriorityQueue<Task>(taskQueue.size(),PRIORITY_COMPARATOR);
                    for (Task task : taskQueue) {
                        if (task instanceof Interruptible && ((Interruptible) task).isInterrupted() ){
                            interruptedQueue.add(task);
                        } else {
                            executeMethod(task);
                        }
                    }

                    for (Task task : interruptedQueue ){
                        executeMethod(task);
                    }
                }
            });

        }

        @Override
        protected void finalize() throws Throwable {
            super.finalize();
            executorService.shutdown();
        }

}

Let's get cracking ;) Nitpick first:

Indentation / Spacing:
Your indentation and spacing aren't fully consistent. It would be beneficial to use an IDE's autoformatting options.

Okay, now that that's out of the way, let's get to the real beef:

AtomicInteger priorityCounter = new AtomicInteger(-1);

This is only used in your executeMethod. And there it's used "wrongly". You're trying to burden the executeMethod with the additional responsibility of checking whether the task you have at hand should be executed now, or not:

if (task.getPriority() > priorityCounter.get()){
    task.execute();
    priorityCounter.getAndSet(task.getPriority());
}

This is not good especially since you fail your if-condition silently. Enqueueing a lower-priority task to the queue does not execute the task and doesn't tell you about that.

That's a big problem and unnecessary. You can completely remove all the stuff in this method, that's got to do with priorityCounter. What remains is task.execute() and that can stand on it's own.

private ExecutorService executorService = Executors.newFixedThreadPool(10);

I don't like this one for multiple reasons:

It isn't final.
You're not using anything anywhere near 10 threads.

If you want to do it "right" you should move all your executorService.submit()-calls directly around the respective task.execute()-calls. Right now you only ever submit two runnables to this 10-thread pool. That's a waste of threads ;)

private PriorityQueue<Task> taskQueue;

Again this could just as well be made final, you could even initialize this eagerly, since the static PRIORITY_COMPARATOR is available anyways ;)

for (Task task : taskQueue) {
   /* stuff in here */
}

First thing I thought when seeing this: "This doesn't respect priority, right?" While that's not true, I personally find the following much more obvious:

while (taskQueue.peek() != null) {
    Task task = taskQueue.poll();
    /* stuff in here */
}

Moving swiftly on to one of the last points I want to make:

if (task instanceof Interruptible && ((Interruptible) task).isInterrupted()) {

Your requirements nowhere state, that only interrupted Interruptibles are to be executed later. Assuming you didn't have the definition of Interruptible given, I had suspected it to be a marker-interface. I'd shorten the code to:

if (task instanceof Interruptible) {

Almost finished:

executorService.submit(new Runnable() {
    @Override
    public void run() {
       /* real stuff here */
    }
});

If you have java 8 available I highly recommend converting this boilerplate to a lambda-expression:

 executorService.submit(() -> { /*code here */ });

One thing about references and shuting down, if I may:

Overriding finalize to shutdown your executor isn't recommended. This answer on SO gives nice impressions.

What you should do instead is implement AutoCloseable  and shut down the executor when overriding the close-method

And last but not least:

One cannot enqueue new Tasks to your MultiThreadedTaskScheduler. While that was not part of the requirements this is one thing that I'd expect of a TaskScheduler ;)


What would you have done differently? How to approach this? so as a beginner in Python, I've been utilizing CodeWars to practice everyday alongside my courses, been happy by the results so far, however, what I really am focusing on is the logical thinking side of things and "Thinking like a programmer". I got a question today on codewars that brought me here to ask. Although the end result of the code I wrote was a "PASS" and it worked out, I know for a fact there is a way to write shorter code using ITERATION for the same result.
The Question was to write a function that takes in a string and returns the string with numbers associated with the alphabetical position. My code is below. Again, it worked, but what would you have done differently? And how do I get into the habit of thinking differently to achieve cleaner code?
def alphabet_position(text):

    alphabet = {'a':'1','b':'2','c':'3','d':'4','e':'5','f':'6','g':'7','h':'8','i':'9','j':'10','k':'11','l':'12','m':'13','n':'14','o':'15','p':'16','q':'17','r':'18','s':'19','t':'20','u':'21','v':'22','w':'23','x':'24','y':'25','z':'26'}
    text = text.lower()
    mylist = []

for letter in text:
    if letter in alphabet:
        mylist.append(alphabet[letter])
    else:
        pass

return ' '.join(mylist)

You are already using iteration, by using a for-loop to iterate over the characters in text.
However, programming is about automating tasks. So thinking like a programmer means finding ways to let the computer do things for you, so you don't have to. For example, instead of creating the dict alphabet by hand, which means typing out all 26 characters and their index, you can let the program do this for you, by making use of the chr() and ord() functions, which convert characters to ASCII values and back, respectively:
alphabet = {}
for i in range(26):
    alphabet[chr(ord('a') + i)] = i + 1

But you can write that more compactly using dict comprehension:
alphabet = {chr(ord('a') + i): i + 1 for i in range(26)}

And now you should realize that you actually don't need alphabet, but can convert the characters in text immediately into indices:
mylist = [ord(letter) - ord('a') + 1 for letter in text]

And if you want you can just replace the whole function with:
def alphabet_position(text):
    return ' '.join([ord(letter) - ord('a') + 1 for letter in text])

However, if you feel more comfortable using for-loops, then by all means keep using those. It also helps if you create a function that converts a single letter to an index; this breaks up your code into simpler chunks that are more easy to reason about:
def alphabet_position_of_letter(letter):
    return ord(character) - ord('a') + 1

def alphabet_position(text):
    positions = [alphabet_position_of_letter(letter) for letter in text]
    return ' '.join(positions)

The above is a bit more verbose, but now the function and variable names help describe what everything means.


Scrape 4chan for alive images I'm trying to learn Clojure recently and I thought writing a simple web app would be a good way to dive in.

This function gets the list of alive threads from the API and reduces, filters and maps until I'm left with a list of image names (which I can append to "http://i.4cdn.org/wg/" to get the URL's.).

This is the first time I'm using defer/@ so I'm not that sure if I used them correctly.

Here is the code:

(defn scrape []
  (let
    [{:keys [status hey body error] :as resp}
      @(http/get "http://a.4cdn.org/wg/threads.json")]
    (->>
      (json/read-str (resp :body))
      (map #(%1 "threads"))
      (reduce concat [])
      (map #(%1 "no"))
      (map #(str "http://a.4cdn.org/wg/thread/" %1 ".json"))
      (map http/get)
      (map deref)
      (map #(%1 :body))
      (map json/read-str)
      (map #(%1 "posts"))
      (reduce concat [])
      (filter #(not (nil? (%1 "tim"))))
      (map #(str (%1 "tim") (%1 "ext")))
      (print))))

Don't need :as resp if only body is ever used.
I'd also drop the :keys destructuring if it's not reused.
While couple of those maps make sense, this sequence is also pretty
repetitive, I'd suggest condensing repeated invocations into a single
call at least.
The (not (nil? ...)) is probably the same as the value itself, so
just (filter #(%1 "tim")) should suffice.
The function should most likely return the list instead of printing
it so the files can be examine further in another part of the code.
Maybe use mapcat instead of the map/reduce/concat combination?

The deref is fine I think, it also depends on how the calls should be
batched together (or not).

All in all:

(ns fourchan.core
  (:require [clojure.data.json :as json])
  (:require [org.httpkit.client :as http]))

(defn scrape []
  (->>
   (json/read-str (@(http/get "http://a.4cdn.org/wg/threads.json") :body))
   (mapcat #(%1 "threads"))
   (map #(http/get (str "http://a.4cdn.org/wg/thread/" (%1 "no") ".json")))
   (mapcat #((json/read-str ((deref %1) :body)) "posts"))
   (filter #(%1 "tim"))
   (map #(str (%1 "tim") (%1 "ext")))))


Reducing execution time for a python program The following is code that I used to do a task. the code runs fine but takes over 2 minutes to get executed. And this is with only one rule (Rule_1), the rule is checked using an .exe ( written in CPP).Actually there are many CSV files that are needed to pass through the rules.

My question is will the program always use this much amount of time, because I have to implement more the 50 rule for the files, or there is any other way around?

import os
import fnmatch
import subprocess
import xml.etree.ElementTree as ElementTree
from xml.parsers.expat import ExpatError
import sys
from shutil import copyfileobj

def locate(pattern, root="Z:/Automation/"):
    '''Locate all files matching supplied filename pattern in and below
    supplied root directory.'''
    for path, dirs, files in os.walk(os.path.abspath(root)):
        for filename in fnmatch.filter(files, pattern):
            yield os.path.join(path, filename)

csv_path_unrefined = []
for xml in locate("*.csv"):
    try:
        ElementTree.parse(xml)
    except (SyntaxError, ExpatError):
        csv_path_unrefined.append(xml)
csv_path = []
for paths in csv_path_unrefined:
    if "results" in str(paths):
        csv_path.append(paths)

def check_rule1(path):
    # path = "PWLLOGGER_DEMO.csv"
    file = 'ConsoleApplication9.exe "' + path + '"'
   # print(file)
    details = os.popen(file).read()
    log_file = open("logs/Rule_1.txt")
    state = log_file.read()
    with open('results/Rule_1_log.log', 'a+') as files:
        files.write("\n========" + path + "========\n")
        files.close
    with open('results/Rule_1_log.log', 'a+') as output, open('logs/Rule_1.txt', 'r') as input:
        copyfileobj(input, output)
    if "failed" in state:
        return False
    else:
        return True

rule_1_passed = []
rule_1_failed = []

for paths in csv_path:
    result_r1 = check_rule1(paths)
    # print(result_r1)
    if result_r1 == False:
        rule_1_failed.append(paths)
        #print("Rule 1 has failed for " + paths)
    elif result_r1 == True:
        rule_1_passed.append(paths)
        #print("Rule 1 has passed for " + paths)
    open('logs/Rule_1.txt', 'w').close()

print(rule_1_failed)
print(rule_1_passed)

Really, to answer your question, you need to profile your code. You need to understand what is taking up all that time. Just by looking at your code, it's impossible to tell, but my guess would be that the most time is spent in one of the following places:

Running every *.csv file you run into through the XML parser. I get the feeling you consider this necessary in order to discard XML files that are pretending to be CSV files. Ideally, you should do this once, then properly label your files thenceforth so you don't have to do this check every time. This strikes me as a potentially very expensive thing to do; as such, I've modified this check below so it only occurs when you might actually be interested in the file later on (that is, its path contains 'results')
Kicking off the external process individually for each file you want to check. Launching processes is not a cheap operation. Ideally, you'd want to EITHER launch a single process for each rule, passing it all relevant file paths for it to check all at once, OR if you wrote your rules in Python, you could read each file in exactly once, then process it through all your rules all at once. Launching a new process for each rule, for each file, is probably a huge source of slowness.

There are also several parts of the code that just seem hacky and bad practice. Also, if you use Python 3.5+, you can use glob instead of your custom locate function. In the spirit of reviewing all your code anyway and making any suggestions that seem appropriate, here's how I'd suggest your code be written, though admittedly without any good suggestions about how to speed up the code (because, again, you MUST profile your code to understand what's actually taking the time):

import os
import subprocess
import xml.etree.ElementTree as ElementTree
from xml.parsers.expat import ExpatError
from glob import iglob

def is_xml(path):
    try:
        ElementTree.parse(path)
        return True
    except (SyntaxError, ExpatError):
        return False

def check_rule1(path):
    subprocess.run(['ConsoleApplication9.exe', path])
    state = open("logs/Rule_1.txt").read()

    with open('results/Rule_1_log.log', 'a+') as output:
        output.write("\n========" + path + "========\n")
        output.write(state)
    return "failed" not in state:

def main():
    csv_path = (path for path in iglob('**/*.csv', recursive=True) if 'results' in path and not is_xml(path))
    rules = [check_rule1]

    for rule_num, rule in enumerate(rules):
        rule_num += 1  # We want to count rules from 1 up
        passed = []
        failed = []

        for paths in csv_path:
            result = rule(paths)
            if result:
                passed.append(paths)
                #print("Rule 1 has passed for " + paths)
            else:
                failed.append(paths)
                #print("Rule 1 has failed for " + paths)

            os.remove('logs/Rule_1.txt')

        # Do something with passed/failed, presumably?

if __name__ == '__main__':
    main()


Copy certain lines from one text file to another I need to open a file, read a line and if that line respect some conditions write it to another file, the line that I'm reading is a normal ASCII string representig HEX values and I need to paste it into a new file as HEX values and not as ASCII string.

What I have is this:

private void Button_Click(object sender, RoutedEventArgs e)
{
    byte[] arrayByte = { 0x00 };
    var linesToKeep = File.ReadLines(fileName).Where(l => l.Contains(":10"));
    foreach (string line in linesToKeep)
    {

        string partialA = line.Substring(9);
        string partialB = partialA.Remove(partialA.Length - 2);
        arrayByte = ToByteArray(partialB);
        using (var stream = new FileStream(fileName+"_gugggu", FileMode.OpenOrCreate))
        {
            FileInfo file = null;
            file = new FileInfo(fileName + "_gugggu");
            stream.Position = file.Length;
            stream.Write(arrayByte, 0, arrayByte.Length);
        }
    }
}
public static byte[] ToByteArray(String HexString)
{
    int NumberChars = HexString.Length;
    byte[] bytes = new byte[NumberChars / 2];
    for (int i = 0; i < NumberChars; i += 2)
    {
        bytes[i / 2] = Convert.ToByte(HexString.Substring(i, 2), 16);
    }
    return bytes;
}

This method is doing what I need but it takes ages to finish, the original files have roughly 70000 lines... Is there a better way to do that in order to increase speed?

EDIT
SOURCE FILE EXAMPLE:

:106AD00000000000000000000000000000000000B6
:106AE00000000000000000000000000000000000A6
:106AF0000000000000000000000000000000000096
:106B00000000000000000000000000000000000085
:106B10000000000000000000000000000000000075
:106B20000000000000000000000000000000000065
:106B30000000000000000000000000000000000055
:106B40000000000000000000000000000000000045
:106B50000000000000000000000000000000000035
:106B60000000000000000000000000000000000025

It is OK to use File.ReadLines(...) because it actually is shortcut to StreamReader.ReadLine() (Not to be confused with File.ReadAllLines()).

I wonder why you reopen the same file for each line you want to save. I would do something like this:

  byte[] arrayByte = { 0x00 };

  using (var stream = new FileStream(fileName + "_gugggu", FileMode.Create))
  {
    foreach (string line in File.ReadLines(fileName).Where(l => l.Contains(":10")))
    {
      ...

This may be the real bottleneck.

I think

      string partialA = line.Substring(9);
      string partialB = partialA.Remove(partialA.Length - 2);

is the same as:

      string subString = line.Substring(9, line.Length - 11); // 11 = 9 + 2

All  in all it could be changed to :

  byte[] arrayByte = { 0x00 };

  using (var stream = new FileStream(fileName + "_gugggu", FileMode.Create))
  {
    foreach (string line in File.ReadLines(fileName).Where(l => l.Contains(":10")))
    {
      string subString = line.Substring(9, line.Length - 11);
      arrayByte = ToByteArray(subString);
      stream.Write(arrayByte, 0, arrayByte.Length);
    }
  }


Is this an appropriate approach and layout for conditionally continuing execution in bash? I've seen others use && for conditionally continuing execution in bash, and found it effective but rather ugly. (I'm not sure if there's a better name for this technique.)

do_something && something_else && the_next_thing

Then I found out that you can simply wrap things in parentheses just because you feel like it. (And in my original motivation was to return to easily return to the calling folder.)

So in the following script, I tried combining those techniques. (The motivation for the script is an attempt to install RabbitVCS for development.)

Questions:

is combining && and () in this way "idiomatic" for bash?
similarly, is using () to return to the calling folder appropriate
how's my indentation? (I normally program in Python ;) )

    #! /bin/bash

    # CC0 To the extent possible under law, the person who associated CC0
    # with this work has waived all copyright and related or neighboring
    # rights to this work. https://creativecommons.org/publicdomain/zero/1.0/

    # Required folders (must exist beforehand):
    #  * Source code and script folders (see export statements below)
    #  * Nautilus 3: ~/.local/share/nautilus/python-extensions
    #  * CLI: /usr/bin (always exists)
    #  * GEdit: ~/.local/share/gedit/plugins

    # Out of scope for this script:
    #  * Setting up folders above
    #  * Keeping RabbitVCS source code up-to-date

    # Set customisable folder paths:
    # Source code, cloned from GitHub
    export SOURCE_FOLDER=~/Code/rabbitvcs
    # User scripts location for RabbitVCS CLI (must be in .bashrc or equivalent)
    export SCRIPT_FOLDER=~/Code/scripts

    if [ "$1" = "--help" ] || [ -z $1 ] || [ "$2" ];
    then

        # if "--help" is called, or too many or too few parameters given, display help
        echo "Install specified branch of RabbitVCS with Nautilus, Gedit and CLI clients."
        echo "(Assumes prerequisites are installed and target branch and folders exist.)"
        echo "Usage: install-rabbitvcs-branch "

    else
        ( # parentheses so that we return to original folder

            # install all prerequisites, even the obvious ones
            # (package names as used in Ubuntu 13.10)
            # (also triggers prompt for sudo password as soon as possible)
            sudo apt-get install gedit git hicolor-icon-theme ipython meld subversion python python-configobj python-dbus python-dulwich python-gobject python-gobject-2 python-gtk2 python-gtkspell python-nautilus python-simplejson python-support python-svn -qq && (

                # kill existing Nautilus and RabbitVCS processes (if they exist)
                pgrep -f nautilus && nautilus -q
                pgrep -f service.py && xargs kill

                # checkout requested branch
                cd $SOURCE_FOLDER && git checkout $1 && (

                    # (re)install RabbitVCS library
                    sudo python setup.py check && sudo python setup.py install && (

                        # (re)install RabbitVCS clients
                        echo "Installing RabbitVCS clients..."
                        cp clients/nautilus-3.0/RabbitVCS.py ~/.local/share/nautilus/python-extensions
                        cp clients/gedit/rabbitvcs-plugin.py ~/.local/share/gedit/plugins
                        cp clients/gedit/rabbitvcs-gedit3.plugin ~/.local/share/gedit/plugins
                        cp clients/cli/rabbitvcs $SCRIPT_FOLDER
                        echo "done."
                    )
                )
            )
        )
    fi

Firstly, this will most likely do a wrong thing if the first argument is empty:

if [ "$1" = "--help" ] || [ -z $1 ] || [ "$2" ];

You should probably use something like this instead (note added quotes):

if [ "$1" = "--help" ] || [ -z "$1" ] || [ $# -gt 1 ];

In general you should quote all variables which are not guaranteed to be set, to be non-empty and to only contain alphanumeric characters.

Secondly, I would not call myself a seasoned bash hacker, but parenthesis are imo more common if you need to run a small command in another directory, e.g.

(cd -P "$anotherdir"; make fun)

When it comes to longer code pieces you can either use a function or simply pushd and popd, e.g.

pushd .
cd "$anotherdir"
# do something else
popd

As for formatting, I also use four-space indentation but prefer more dense code and therefore put then on the same line as if. Additionally, I usually write following code if I need several commands to run in sequence only if everything worked out fine:

stderr() {
    cat - 1>&2
}

fail() {
    printf "%s\n" "$@" | stderr
    return 1
}

commandA "$arg1" "$arg2" || fail "ERROR: something went wrong"
commandB "$arg1" "$arg2" || fail "ERROR: something else went wrong"

UPDATE: Incorporate rolfl's suggestion to use a fail() function.


Java Pig Latin Translator Over the last few days I created this Pig Latin Translator just for fun. I would really appreciate it if anybody could give suggestions of how to improve the code and make the program more efficient.

Here is the GitHub link for the project.

For anybody who doesn't know the Pig Latin, feel free to look at the README, which explains what Pig Latin is, and the rules of the language. The code is in PigLatin.java.

    /*  -------------------- Program Information --------------------
    Name Of Program: PigLatin.java
    Date of Creation: 7/3/2018
    Name of Author(s): Agnik Banerjee
    Version of Java: 1.8.0_171
    Created with: IntelliJ IDEA 2017.3.5 Community Edition
    -------------------- Program Information --------------------  */

import java.util.Scanner;

public class PigLatin {

    private static Scanner scan = new Scanner(System.in);

    public static void main(String[] args) {
        System.out.print("Enter one or more words that you would like to translate to Pig Latin: ");
        final String userInput = scan.nextLine();
        scan.close();

        String[] word = userInput.split(" "); // Splits the string into an array of words
        String output = "";

        for (int i = 0; i < word.length; i++) {
            String pigLatinWord = translateWord(word[i]); // Translates each word individually
            output += pigLatinWord + " "; // Joins the translated word back into the output
        }

        System.out.println("Original Word(s): " + userInput);
        System.out.println("Translation: " + output + "\n");
    }

    public static String translateWord(String word) {
        String lowerCaseWord = word.toLowerCase();
        int pos = -1; // Position of first vowel
        char ch;

        // This for loop finds the index of the first vowel in the word
        for (int i = 0; i < lowerCaseWord.length(); i++) {
            ch = lowerCaseWord.charAt(i);

            if (isVowel(ch)) {
                pos = i;
                break;
            }
        }

        if (pos == 0) {
            // Translating word if the first character is a vowel (Rule 3)
            return lowerCaseWord + "yay"; // Adding "yay" to the end of string (can also be "way" or just "ay")
        } else {
            // Translating word if the first character(s) are consonants (Rule 1 and 2)
            String a = lowerCaseWord.substring(pos); // Extracting all characters in the word beginning from the 1st vowel
            String b = lowerCaseWord.substring(0, pos); // Extracting all characters located before the first vowel
            return a + b + "ay"; // Adding "ay" at the end of the extracted words after joining them.
        }
    }

    // This method checks if the character passed is a vowel (the letter "y" is counted as a vowel in this context)
    public static Boolean isVowel(char ch) {
        if (ch == 'a' || ch == 'e' || ch == 'i' || ch == 'o' || ch == 'u' || ch == 'y') {
            return true;
        }
        return false;
    }
}

I would suggest the following points to make this programme more object-oriented:

Try using a collection object provided by Java instead of using array

String[] word = userInput.split(" ");

could be

List<String> words = Arrays.asList(userInput.split(" "));

or using external library Guava

List<String> words = Splitter.on(" ").splitToList(userInput);

Avoid comments where you can.

In this case I can see you can easily avoid comment if you can wrap up the particular code snippet with the meaningful name. For instance You could take the user input and breaking it in a list in separate class and call the class from your main

List<String> words = new UserInputHandler().getInputAsList();

Try to use foreach loop over for loop:

You can easily use the foreach loop to iterate over words

for (String word : words) {
    String pigLatinWord = translateWord(word); // Translates each word individually
    output += pigLatinWord + " "; // Joins the translated word back into the output
}

Again as suggested before you can wrap the code inside for loop in a separate class so that you could call

for (String word : words) {
    output = new PigLatinTranslator(word, output).translate();
}


Tight coupling in using call back method I am using below code and for result i am using interface for call back purposes.

@Override
public void onResult(String result) {

    if (this.mClient != null) {
        this.mClient.cancel();
        mProcessing = false;
    }

    try {
        JSONObject j = new JSONObject(result);
        JSONObject j1 = j.getJSONObject("status");
        int j2 = j1.getInt("code");
        if (j2 == 0) {
            JSONObject metadata = j.getJSONObject("metadata");
            //
            if (metadata.has("humming")) {
                JSONArray hummings = metadata.getJSONArray("humming");
                for (int i = 0; i < hummings.length(); i++) {
                    JSONObject tt = (JSONObject) hummings.get(i);
                    String title = tt.getString("title");
                    JSONArray artistt = tt.getJSONArray("artists");
                    JSONObject art = (JSONObject) artistt.get(0);
                    String artist = art.getString("name");
                    tres = tres + (i + 1) + ".  Title: " + title + "    Artist: " + artist + "\n";
                }
            }
            if (metadata.has("music")) {
                JSONArray musics = metadata.getJSONArray("music");
                for (int i = 0; i < musics.length(); i++) {
                    JSONObject tt = (JSONObject) musics.get(i);
                    String title = tt.getString("title");
                    JSONArray artistt = tt.getJSONArray("artists");
                    JSONObject art = (JSONObject) artistt.get(0);
                    String artist = art.getString("name");
                    tres = tres + (i + 1) + ".  Title: " + title + "    Artist: " + artist + "\n";
                }
            }
            if (metadata.has("streams")) {
                JSONArray musics = metadata.getJSONArray("streams");
                for (int i = 0; i < musics.length(); i++) {
                    JSONObject tt = (JSONObject) musics.get(i);
                    String title = tt.getString("title");
                    JSONArray channelId = tt.getJSONArray("channel_id");
                    tres = tres + (i + 1) + ".  Title: " + title + "    Channel Id: " + channelId + "\n";
                }
            }
            if (metadata.has("custom_files")) {
                JSONArray musics = metadata.getJSONArray("custom_files");
                for (int i = 0; i < musics.length(); i++) {
                    JSONObject tt = (JSONObject) musics.get(i);
                    String title = tt.getString("title");
                    //tres = tres + (i + 1) + ".  Title: " + title + "\n";
                    tres = tt.getString("acr_id");
                }
            }
            //tres = tres + "\n\n" + result;
            if (baseClass instanceof MainActivity)
                ((MainActivity) baseClass).onResultFound(tres);
        } else {
            tres = result;
            if (baseClass instanceof MainActivity)
                ((MainActivity) baseClass).onResultNotFound(tres);
        }

    } catch (JSONException e) {
        tres = result;
        e.printStackTrace();
        if (baseClass instanceof MainActivity)
            ((MainActivity) baseClass).onErrorFound(tres);
    }

    //mResult.setText(tres);

}

My question is that is it the right way of coding to give call back or it causes tight coupling issue using below code.

if (baseClass instanceof MainActivity)
            ((MainActivity) baseClass).onResultNotFound(tres);

Interface:

public interface IResultListener {

    /*
    * This is call back listener when acr starts recognition sound of audio/video
    * and methods are available according to result
    * */

    // result found successfully
    void onResultFound(String result);

    // result not found
    void onResultNotFound(String result);

    // some exception occurred
    void onErrorFound(String result);
}

Instead of casting to MainActivity, you should be casting to IResultListener.

if (baseClass instanceof IResultListener)
            ((IResultListener) baseClass).onResultNotFound(tres);

That said, usually one registers a listener. So you'd have a List<IResultListener> somewhere, and then in this method you'd call a method fireEventResultFound(String result), which iterates over the list and calls onResultFound for each of them. This is called the Observer pattern (you can google that if you want to know more), and it decouples the reporting class from the observing class.

I also think you should move the parsing of hummings and musics, streams and custom_files to their own methods. That would greatly simplify this onResult method.

Also,

tres = tt.getString("acr_id");

Is this a possible bug? Everywhere else you're appending, but here you just overwrite the result.


Reliability polynomial calculation The reliability polynomial (specifically, the all-terminal reliability polynomial) is, given a value \$0 \le p \le 1\$, outputs the probability that a given graph \$G\$ is connected.

Note: at the link given, the probability is given to an edge being deleted, and not to an edge existing (which is the definition we will use here).

Of course, if the graph is already not connected, then it has probability 0 that it is connected. If the graph has exactly 1 vertex and no edges, it always will be connected (so has reliability polynomial 1).

The usual "brute-force" calculation of the polynomial is by the "Factoring Theorem": for any edge \$e\$ in the graph \$G\$:

$$Rel(G; p) = p*Rel(G*e; p) + (1-p)*Rel(G/e; p)$$

where \$G*e\$ is the resulting graph of contracting the edge \$e\$, and \$G/e\$ is that of deleting the edge \$e\$.

I implemented this calculation using Sympy (for multiplying polynomials) and networkx (for using graphs). I would like to get some feedback on the structure of the code as well as readability.

Note: I actually do need MultiGraph here because contracting an edge will introduce parallel edges, which do make a difference in the calculation.

import networkx as nx
import random
import sympy

p = sympy.symbols('p')

def relpoly(G):
    H = nx.MultiGraph(G)
    rel = _recursive(H)
    return sympy.simplify(rel)

def _recursive(G):
    # If the graph is not connected, then it has a rel poly of 0
    if not nx.is_connected(G):
        return sympy.Poly(0, p)

    # if # edges > 0, then we perform the two subcases of the
    #   Factoring theorem.
    if len(G.edges()) > 0:
        e = random.choice(G.edges())
        contracted = nx.contracted_edge(G, e, self_loops=False)
        G.remove_edge(*e)
        rec_deleted = _recursive(G)
        rec_contracted = _recursive(contracted)
        s = sympy.Poly(p)*(rec_contracted) + sympy.Poly(1-p)*(rec_deleted)
        return s

    # Otherwise, we only have 0 edges and 1 vertex, which is connected,
    #   so we return 1.
    return sympy.Poly(1, p)

print(relpoly(nx.petersen_graph()))

Some suggestions:

_recursive isn't a particularly descriptive name. A lot of things can be recursive. _relpoly_recursive may be a better name.
You can factor out the Poly call

s = sympy.Poly(p*rec_contracted + (1-p)*rec_deleted, p)

also note that I've removed the redundant parentheses.
Calling simplify on the result is completely useless, since rel is a Poly and Poly instances are always stored in canonical (expanded) form.
What is the value of going through the edges randomly? Don't you end up going through all of them.
As I noted in a comment, modifying the graph in-place seems problematic. I can't tell for sure just by looking at the code if it does the right thing or not. It seems that you should rather move the

H = nx.MultiGraph(G)

line to the top of the recursive function, and operate on H instead of G in the function.


Building a query URL in Scala I'm working on a webapp in ScalaJS and want to create a query url for requesting some JSON.

Right now I'm using a method called urlBuilder to take the query options from an Options case class and return a String that represents a usable query URL. The urlBuilder method, despite my best efforts, has become somewhat ugly.

Am I missing out on any syntactic sugar or language features that could make this a little easier to understand/cleaner looking? Is pattern matching + string interpolation the best strategy here?

case class GameRegions(americas: Boolean, europe: Boolean, asia: Boolean)

case class Options(startRank: Option[Int], endRank: Option[Int], startDate: Option[Date], endDate: Option[Date], startTime: Option[Date], endTime: Option[Date], regions: GameRegions, gameMode: Boolean)

//Builds the URL based on the provided options object
def urlBuilder(baseUrl: String, o: Options): String = {
  baseUrl + "?" + (o.startRank match {
    case Some(x) => s"rank[0]=$x&"
    case None => ""
  }) + (o.endRank match {
    case Some(x) => s"rank[1]=$x&"
    case None => ""
  }) + (o.startDate match {
    case Some(x) => s"added[0]=@${x.getTime / 1000}&"
    case None => ""
  }) + (o.endDate match {
    case Some(x) => s"added[1]=@${x.getTime / 1000}&"
    case None => ""
  }) + (o.startTime match {
    case Some(x) => s"time[0]=@${x.getTime / 1000}&"
    case None => ""
  }) + (o.endTime match {
    case Some(x) => s"time[1]=@${x.getTime / 1000}&"
    case None => ""
  }) + (o.regions match {
    case GameRegions(false, false, false) => ""
    case GameRegions(x, y, z) => {
      var i: Int = -1
      if (x) {
        i = i + 1
        s"region[$i]=Americas&"
      } else {
        ""
      } + (if (y) {
        i = i + 1
        s"region[$i]=Europe&"
      } else {
        ""
      }) + (if (z) {
        i = i + 1
        s"region[$i]=Asia&"
      } else {
        ""
      }) + "&"
    }
  }) + s"format[0]=${if (o.gameMode) "Standard" else "Wild"}"
}

Sometimes pattern matching over Option could be replaced with fold or map/getOrElse

o.startRank.fold("")(x => s"rank[0]=$x&")

o.startRank.map(x => s"rank[0]=$x&").getOrElse("")

You could notice that url parameters have a pattern key=value joined with &. To leverage this pattern url builder could be split into separate steps:

create a map Map[String,String] of parameters i.e.

Map(
  "rank[0]" -> o.startRank.fold("")(_.toString),
  ...
  "time[0]" -> o.startTime.map(time => time.getTime/1000).map(_.toString).getOrElse("")

join parameters into string

map.filter(_._2.nonEmpty).map {
  case (key, value) => s"$key=$value"
}.mkString("&")


Novel sorting algorithm I have devised a novel sorting algorithm, named Oracle Sort, which I believe is far superior to all currently known sorting algorithms (under certain assumptions). It utilizes an oracle to achieve a constant time complexity with regard to both the length of the input sequence and the number of distinct elements in the input sequence.

However, it seems that current computing is not yet ready for such an advanced algorithm, and therefore certain compromises had to be made and the algorithm had to be altered. As a result of these alterations, the performance of the provided algorithm has been much degraded. Therefore, I ask for any advice regarding the improvement of the time complexity of the algorithm as it stands.

Given below is an example implementation of the algorithm in Python 3.

from itertools import count, product

def oraclesort(input_seq):
  """
  Find a sequence (via enumeration), with is equal the sorted input sequence.

  Under ideal circumstances the enumeration would be done in parallel for all
  sequences of length up to at least the length of the input, and enumerating
  each of these sequences would take constant time. However, currently we are
  not able to achieve such results, so we have to resort to a less efficient
  sequential enumeration.

  The equality of enumerated candidate sequence and the sorted input sequence
  is determined by an oracle (the oracle_equals_sorted() function), which can
  tell in (assumed) constant time, whether the given candidate is equal to the
  sorted input. This, combined with the constant time needed to generate the
  candidates, gives us a total time complexity of O(1); the best known time
  complexity of an arbitrary-input sorting algorithm.
  """
  for i in count():
    for candidate_seq in product(input_seq, repeat=i):
      if oracle_equals_sorted(candidate_seq, input_seq):
        return candidate_seq

def oracle_equals_sorted(candidate_seq, input_seq):
  """
  Oracle which checks whether candidate_seq equals the sorted input_seq.

  The oracle should work in constant time (or ideally, in no-time), but we are
  limited to deterministic Turing Machines, and therefore, we are limited
  to certain implementations of the oracle, and these implementations happen
  to have non-optimal (eg. nonzero) time complexity.

  Despite being suboptimal, this implementation of the oracle is very elegant
  in that it internally uses the oracle sort to determine whether the given
  candidate sequence is equal to the sorted input sequence, hence completing
  a beautiful cycle of mutual recursion.
  """
  candidate_lst = list(candidate_seq)
  input_lst = list(input_seq)

  if not candidate_lst and not input_lst:
    return True

  if not candidate_lst or not input_lst:
    return False

  _, min_idx = min((val, idx) for idx, val in enumerate(input_lst))
  input_lst_without_min = input_lst[:min_idx] + input_lst[min_idx+1:]

  return candidate_lst[0] == input_lst[min_idx] and \
         candidate_lst[1:] == list(oraclesort(input_lst_without_min))

well done providing doc strings
sticking to the Style Guide for Python Code makes Python code easier to grasp, especially for someone who didn't write it
to make the naming more convincing, you should factor out sequential_deterministic_stand_in_for_seeing_the_required_result() and sequential_deterministic_stand_in_for_checking_the_result()
As a bonus, this removed implementation detail from the docstrings of "the oracle functions".
the nested loops in oraclesort() seem to iterate all permutations of elements from input_seq - as indicated in the docstring.
Using itertools.permutations() is easier to read and should test sequences of like length, only.
(Strictly faster than the related bogobogosort for not repeatedly checking the same sequence.)
instead of determining the index of the minimum value and using slices to construct input_lst_without_min, you could use just determine the minimum value, compare it to the start of the candidate sequence and remove it from the sequence to sort recursively:
input_lst_without_min = input_seq[:]
input_lst_without_min.remove(min(input_seq))


class design improvement - user validation I'm looking to improve this class - any suggestions?  It checks for empty, name, email, and password.
The regex for email is very simple.  A very lengthy article from the Linux Journal for improving upon this is here.  The character set for password I borrowed from the NASA signup page.  For name I allow letters, periods, and dashes.  Testing for empty was developed with in this SO Post

class check
  {
  static function empty_user($a)
    {
    return (int)!in_array('',$a,TRUE);
    }
  static function name($a)
    {
    return preg_match('/^[a-zA-Z-\.]{1,40}$/',$a);
    }
  static function email($a)
    {
    return preg_match('/^[a-zA-Z0-9._s-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{1,4}$/',$a);
    }
  static function pass($a)
    {
    return preg_match('/^[a-zA-Z0-9!@#$%^&*]{6,20}$/',$a);
    }
  }

First of all, as mentioned by @user7212, use php filter_var() for email validation. Please see below for example:

/**
 * Validate email
 *
 * @param string $email
 *
 * @return bool
 */
public static function email($email)
{
    return filter_var($email, FILTER_VALIDATE_EMAIL);
}

Second, why not naming your function params? If the email validation function would be called $email, if would be much easier to understand then $a.

in_array() function, according to it's documentation, returns bool. Why converting it to int? The result of the check should be bool.

In the pass() function, are you checking a string or a hash? Which brings us to the following item - write comments for your code! PHPDoc is a good example on how to do that.

And the last, but not the least - the name of the class. Check is not the most appropriate name. I would consider Validation or Valid.

Hope this helps.


Expanding list on webpage The below produces the output I want. Namely, it initially displays the first item on the list and the first expander link. When an expander link is clicked, it hides that link, unhides one more item in the list, and unhides the next expander link.

I'm wondering if it can tightened up. JQuery is a definite no-no.

 window.onload = function() {
  var anchors = document.getElementsByTagName('a');
  for(var i = 0; i < anchors.length; i++) {
   var anchor = anchors[i];
   if(anchor.classList.contains('expander_link')) {
    anchor.onclick = function(e) {
     e.preventDefault();
     e.target.setAttribute('style', 'display:none');
     var href = e.target.getAttribute("href")
     var unhide = document.getElementById(href);
     unhide.setAttribute('style', '');
     var data = e.target.getAttribute("data");
     var link = "link_" + (parseInt(data)+1);
     var next = document.getElementById(link);
     next.setAttribute('style', '');
    }
   }
  }
 }
  body {
    margin: 15pt 20pt 0pt 20pt;
    padding: 0;
  }

  p {
    margin: 8pt 0 8pt 0;
    padding: 0;
  }

  .center {
    margin: auto;
    align-self: center;
    text-align: center;
  }

  h4 {
    color: #004080;
    font-size: 120%;
    margin: 100pt 0 0 0;
  }

  .nobullets {
    list-style: none;
  }

  .hidden {
    display: none;
  }`

  table {
    border-spacing: 0;
  }

  th, td {
    text-align: center;
    padding: 3pt;
    margin: 0;
  }

  ul {
   list-style-type: none;
   margin-left:0;
   padding-left:0;
  }

  ul ul {
    list-style: disc;
    margin-left: 20pt;
  }

  li[id]{
    border-top: 1px solid #AAAAAA;
  }

  a#link_0 + [id] {
    border-top: none;
  }

  .block {
    display: block;
   }

  .expander_link {
    padding-top: 8pt;
    padding-bottom: 30pt;
    font-size: 120%;
    font-weight: bold;
    color: #004080;
    text-align: center;
    align-self: center;
    text-decoration: none;
    display: block;
    border-top: 1px solid #AAAAAA;
  }

Expanding list

  part one

show part two

 part two

show the third part

part 3

show part 4

part 4

My solution relies on the CSS rule which says, "hide all list items that come after the one with the active class."

li.active ~ li {
  display: none;
}

The JavaScript deals with getting the index of the list item containing the active class and re-assigning the active class to the next list item (if there is one).

window.addEventListener("load", init);

var btn, list_length, ul;

function init() {
  ul = document.getElementsByTagName("ul")[0];
  btn = document.getElementsByTagName("button")[0];
  list_length = ul.children.length;

  btn.addEventListener("click", function() {
    if (getItemIndex(getActiveItem()) < (list_length -1)) {
      showNextItem();
    }
  });
}

function getActiveItem() {
  return document.getElementsByClassName("active")[0];
}

function getItemIndex(active_item) {
  var nodeList = Array.prototype.slice.call(ul.children);

  return nodeList.indexOf(active_item);
}

function getListItem(index) {
  return document.getElementsByTagName("li")[index];
}

function showNextItem() {
  var active_item = getActiveItem(),
      next_index  = getItemIndex(active_item) + 1,
      next_item   = getListItem(next_index);

  active_item.classList.remove("active");
  next_item.classList.add("active");

  if ((next_index + 1) < list_length) {
    btn.getElementsByTagName("span")[0].innerHTML = getListItem(next_index + 1).innerHTML;
  } else {
    btn.classList.add("hide");
  }
}
h1 {
  font-size: 1.2em;
}

button {
  font-size: 1em;
}

button.hide {
  display: none;
}

ul {
  margin: 0 0 1em;
  padding: 0;
  border-bottom: 1px solid gray;
  width: 40em;
}

li {
  list-style: none;
  padding: 0.8em 0;
  border-top: 1px solid gray;
}

li.active ~ li {
  display: none;
}

Expanding List

part one

part two

part three

part four

<button>Show part two
</button>


Age calculator in python I am new to python and to the StackExchange sites. I made an age calculator but i want to improve it till it's a reliable thing. Can i shorten the code? What can be Done more? Does it need any additions?
Thanks in advence.
from datetime import datetime

print("A simple program to calculate your age")
print()

date = datetime.now()
year = int(date.year)
month = int(date.month)
day = int(date.day)
input_year = int(input("Enter the year you were born: "))
if input_year <= 0 or input_year > 2020:
    print("don't you know your birthday??")
    exit()
input_month = int(input("Enter the month you were born: "))
if input_month <= 0 or input_month > 12:
    print("don't you know your birthday??")
    exit()
input_day = int(input("Enter the day you were born: "))
if input_day <= 0 or input_day > 31:
    print("don't you know your birthday??")
    exit()
age_year = year - input_year
age_month = month - input_month
if age_month < 0:
    age_year = age_year - 1
    age_month = 12 - (input_month - month)
age_day = day - input_day
if age_day < 0:
    age_month = age_month - 1
    age_day = 31 - (input_day - day)
print("you are " + str(age_year) + " years, " + str(age_month) + " months and " + str(age_day) + " days old.")

Generally, it is often not a good idea to hardcode values. For example, take a look at this piece of code. It won't work correctly from 2021 onwards:
if input_year <= 0 or input_year > 2020:
    print("don't you know your birthday??")

Better do it like this:
date = datetime.now()
current_year = int(date.year)

if input_year <= 0 or input_year > current_year:
    print("don't you know your birthday??")

This will make your program work in future years without the need to change the value for the current year manually.


Simple httpcode monitor I'm making a simple httpcode monitor as an example for teaching myself PHP OOP. I have written the code below and would be interested in having it improved.

In particular, I'm not sure about this line:

$httpcode = $website->get_httpcode($url);

This seems very inefficient to me as I have to to pass the URL back to the website class right after I have already done just that two lines above. Shouldn't it already remember its URL? That might be a dumb question, but in my mind, I've already made an object called website and given it a URL. Then if I want the httpcode of the URL, then I feel there should be some way for it to remember its URL which I passed two lines above.

class website {
var $url;

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

public function get_url() {
    return $this->url;
}

public function get_httpcode($url) {
    //get the http status code
    $agent = "Mozilla/4.0 (compatible; MSIE 5.01; Windows NT 5.0)";
    $ch=curl_init();
    curl_setopt ($ch, CURLOPT_URL,$url );
    curl_setopt($ch, CURLOPT_USERAGENT, $agent);
    curl_setopt ($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt ($ch, CURLOPT_VERBOSE,false);
    curl_setopt($ch, CURLOPT_TIMEOUT, 10);
    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, FALSE);
    curl_setopt($ch, CURLOPT_SSLVERSION, 3);
    $page=curl_exec($ch);
    $httpcode = curl_getinfo($ch, CURLINFO_HTTP_CODE);
    curl_close($ch);
    return $httpcode;
}
}

// create an instance of the website class and pass the url
$website = new website("http://www.google.com");
$url = $website->get_url();
$httpcode = $website->get_httpcode($url);

Two things:

Always specify the visibility. Don't use the old var keyword, use public, private or protected. Since the class-member $url contains internal information, you'll probably want to hide that information (you've already implemented a getter method so that the information can be read but not written):

class website {
    protected $url;

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

    // ...

Every instance of this class seems to be meant to represent one website. But your method "get_httpcode()" works for every website (it expects a URL; this is exactly what you've already noticed by yourself). It would be more natural to skip the parameter and return the HTTP-Statuscode for the url that is bound to the instance (you've "remembered" the URL in the class-member variable $this->url):

class website {
    // ...

    public function get_httpcode() {
        // ...
        curl_setopt($ch, CURLOPT_URL, $this->url);
        // ...
    }
}

// Usage:
$google = new website("http://www.google.com");
$statusCode = $google->get_httpcode();


All values of boolean-triple Let a,b,c be a boolean value. I need to print all values of expression a && b && c. I'm writing the class

public class BooleanTriple{
    private boolean a,b,c;
    public BooleanTriple(boolean a, boolean b, boolean c){ this.a =a; this.b=b; this.c=c;}

    /**
    *Increments the value of triple with lexicographical ordering
    */
    public void incr(){
        if (!a) a=true;
        else if(!b){
            a= false;
            b= true;
        else if (!c){
            a= false;
            b= false;
            c=true;
        }
    }
    public boolean logProduct(){ return a && b && c;}
}

And Main class:

public class Main{
    BooleanTriple bTriple = new BooleanTriple(false,false,false);
    public static void main(String[] args){
        for (int i=0; i<8; i++){
            System.out.println(bTriple.logProduct());
            bTriple.incr();
        }
    }
}

But i think, that it's bad implementation. Can you correct me?

I'll give you a hint. Think of a && b && c as a 3 digit binary number

So all possibles values will be:

a | b | c
0 | 0 | 0
0 | 0 | 1
0 | 1 | 0
0 | 1 | 1
1 | 0 | 0
1 | 0 | 1
1 | 1 | 0
1 | 1 | 1

As you can see, you want to flip c every time and check the value of a and b to determine what they will be. b will change any time c is 1 and a will change any time b and c are 1. This should be enough to give you a better implementation.


Checking whether a shop record exists I have a method to check if the Id exists in the data or not, for example:

func (r *ReviewData) Exist() bool {
    // TODO :  checking the shopID value is nil or not.
    // it is a good way to check the value before go to the execution
    query := fmt.Sprintf(
        `
        SELECT
        'x'
        FROM
        ws_active_seller
        WHERE
        shop_id = %d
        `,
        r.ShopID,
    )

    result := postgres.QueryRow(query)
    var data []uint8
    result.Scan(&data)

    if len(data) < 1 {
        return false
    }

    if data[0] != 'x' {
        return false
    }

    return true
}

Notice here that I'm not using any parameters to pass into method here.
But There is another approach to do this with :

func (r *ReviewData) Exist(shpoID int64) bool {
    // TODO :  checking the shopID value is nil or not.
    // it is a good way to check the value before go to the execution
    query := fmt.Sprintf(
        `
        SELECT
        'x'
        FROM
        ws_active_seller
        WHERE
        shop_id = %d
        `,
        shopID,
    )
    // the rest is the same
}

The different with two method above is the parameters. I would like to know what's a good practice for this.

Any ideas on what can be improved?

In Go you have a choice as to whether you treat structs like an object with its own methods, and you have an object-oriented feel to your code, or you treat data like parameters, and you pass the data around in a more procedural format. Let's look at your first method first.

The ReviewData struct is a struct, and you've declared a method on it that has no parameters. The data required in the method comes from the instance of the struct itself, r.ShopID. The logic there is clear (as an aside .... semantically it's a bit ambiguous because, if you have an r instance, it seems like it should Exist... it's a bit odd to see logic like if abc.Exists() {... because if abc did not exist it seems like abc should be nil ;-)

Regardless, your first version of the code looks very natural to me if you're treating the ReviewData as an "object" with an object-oriented perspective.

The second version of the code is flawed in its current state. The problem is that the second version has the signature:

func (r *ReviewData) Exist(shpoID int64) bool {

but, inside the function, you never use r. Why is it a method on an instance that you never use the instance's data for? It should be just:

func Exist(shopID int64) bool {

(NB: I changed the spelling to be correct... you really should make sure the code works before submitting it to Code Review - broken code is frowned on, and code that does not compile is very frustrating. It could not have compiled with shpoID instead of shopID)

Anyway, the second version is good as a static type of procedure/function, instead of an object instance-based method.

So, depending on how you use the code, the general structure of other methods on ReviewData, and the business logic about whether you create an instance of ReviewData before the shopID exists, or not, determines whether you use the method based version (the first one), or the function version (the second one).

If you store the shopID on the ReviewData instance (and in no other places), then its better to use the first mechanism. If the shop ID is used in multiple places then it may be best to have a package for shops themselves, and then you code could look like if shops.Exists(id) .... (which is the second version).

Bottom line, the right solution depends on how your data is encapsulated, and how you reuse the code.


Calculator for calculating price per feet amounts So I have a customers site I am remaking into an mvc3 app and they have an existing JavaScript pricing calculator.  Is this safe and efficient?  I am no JavaScript guy, but it looks pretty solid to me.

And here is the form.

<form name="carpet" action="#">

        Cost Estimate

        <input class="inp" type="text" />
        : Enter Square Footage

        <input class="inp" type="text" value="$0" readonly="readonly" />
        : Estimated Cost For Carpet

    <input class="inp" type="text" value="$0" readonly="readonly" />
    : Estimated Cost For Tile

    <input class="inp" type="text" value="$0" readonly="readonly" />
    : Estimated Cost For VCT and Wood

    <input class="inp" type="text" value="$0" readonly="readonly" />
    : Estimated Cost For Travertine Tile

    <input type="reset" />

</form>

I am worried this is old and outdated. Could I do this more effectively with C# in the app?

About the only thing that I don't like in it is that you have to edit the code each time a price changes. I'd prefer that to come from a dynamic source, especially if you are going to use it in more than one spot.


Display the last N lines of a text file with line numbers in reverse order Do you think the way I'm doing it is fine? Is this the most typical way to do it? Share your thoughts please.

# Display the last three lines of a text file with line numbers in reverse order
# Can come quite in handy when reading log file entries
cat -n [FILE] | tac | head --lines=3

Test:

# Let's create a text file to test our command against
cat > test.txt

# Copy and paste these lines into the terminal window and press Ctrl+D to let
# the shell know that we're done entering our stuff
house
car
boy
cat
lake
sea
laptop
rain

# Run the command
cat -n test.txt | tac | head --lines=3

# Brief description:
# cat -n test.txt | - Pipe the output with line numbers to the tac command
# tac |             - Reverse the order and pipe along to the head command
# head --lines=3    - Display only the first three lines

# Output:
     8  rain
     7  laptop
     6  sea

# The number next to the first line in our output (8  rain) should be equal to
# the number of lines in the entire file. We can check that using the word count
# utility:
wc -l test.txt

# Output (It's 8 too. Looks like the test passes):
8 test.txt

cat -n is kind of an anti pattern. The cat command is for displaying the content, adding line numbering is a violation of the single responsibility principle.

tac is not portable. Although it's quite common in Linux, not so much in UNIX and osx.

Instead of head --lines=3 it's more common to use the shorter form head -n 3.

I don't quite get why you would want to see the last n lines reversed. If reversing is not important, then a portable alternative to print the last n lines with line numbers could be using awk and tail:

awk '{ print NR ":" $0 }' file | tail -n 3

You could write the whole thing in Awk, and also reverse the lines. It will be longer to write, but portable, and solved in a single process rather than a pipeline of multiple commands.


Transforming an array: take the square root of each perfect square, else square the number SquareOrSquareRoot should get an array of integers and return a new array. If the number at index i is a "square" number the returned array at index i should have its square root. If the original number is not a "square" number then the returned array in index i should be the number squared.

Coming from Python (where this could be done in a single line using list comprehension or using map), I find it very odd that this is probably one of the shortest ways to achieve that in Go (please prove me wrong).

package main

import (
    "fmt"
    "math"
)

func SquareOrSquareRoot(arr []int) []int{
    arr_to_return := make([]int, len(arr))
    for index, value := range arr {
        val_sqrt := math.Sqrt(float64(value))
        if val_sqrt == math.Trunc(val_sqrt) {
            arr_to_return[index] = int(val_sqrt)
        } else {
            arr_to_return[index] = value * value
        }
    }

    return arr_to_return
}

func main() {
    arr := []int{100, 101, 5, 5, 1, 1}
    fmt.Println(SquareOrSquareRoot(arr))
    // [10 10201 25 25 1 1]
}

Naming conventions aside (short names in camelCase are typically used in Go), this is the right way of doing what you want. The authors of Go have a different view of what is more readable & maintainable than the authors of Python :-)


Getting recent stations I made a database for my Android app using SQLite and it works fine. However, as I understood, SQLite calls shouldn't be made on UI thread.

Is the below okay or not?

/**
     * @param howMany Amount of Stations returned, passing null returns all.
     *                Sorted after last time searched.
     * @return
     */
    public ArrayList<Station> getRecentStations(final int howMany) {
        final ArrayList<Station> stations = new ArrayList<Station>(howMany);

        new AsyncTask<Void, Void, Cursor>() {

            @Override
            protected Cursor doInBackground(Void... params) {
                SQLiteDatabase db = getReadableDatabase();

                Cursor cursor = db.query(TABLE_NAME_RECENT, COLUMNS_RECENT, null, null, null, null, COLUMN_TIME_SEARCHED + " DESC", Integer.toString(howMany));

            if (cursor != null) {
                    cursor.moveToFirst();
                }
                while (!cursor.isAfterLast()) {

                    Station s = new Station();
                    s.setStationId(cursor.getInt(0));
                    s.setStationName(cursor.getString(1));
                    s.setLatitude(Double.parseDouble(cursor.getString(2)));
                    s.setLongitude(Double.parseDouble(cursor.getString(3)));
                    s.setType(cursor.getString(4));
                    s.setTimeSearched(cursor.getString(5));
                    cursor.moveToNext();
                    stations.add(s);

                }

                db.close();
            }

        }.execute();
        return stations;

    }

However as I understood SQLite calls shouldn't be made on UI thread.

To be honest I wasn't aware of this: I make SQLite calls on the UI thread in many apps.
I think it's fine, unless you expect the calls might be slow.
But since you already got it working with AsyncTask,
then you can just as well leave as it is.

I have some other recommendations though.

Change the method to return List<Station> instead of ArrayList<Station>. You should try to use interface types (List<>) whenever possible instead of implementations (ArrayList<>).

A simpler way to iterate over the cursor:

while (cursor.moveToNext()) {
    Station s = new Station();
    s.setStationId(cursor.getInt(0));
    s.setStationName(cursor.getString(1));
    s.setLatitude(Double.parseDouble(cursor.getString(2)));
    s.setLongitude(Double.parseDouble(cursor.getString(3)));
    s.setType(cursor.getString(4));
    s.setTimeSearched(cursor.getString(5));
    stations.add(s);
}

But actually... Why does doInBackground return a Cursor? Why not do everything in this method and do nothing in onPostExecute? It seems that would make a lot more sense.


Removing duplicate code from basic collision detection implementation The code is part of a very basic 2D platformer based on code from the book Killer Game Programming in Java.

The methods are part of a TileMapManager class which is handling basic collision detection.

The first method returns the greatest distance that an entity can travel in the y axis (up to deltaY) without colliding with a tile:

public int getMaxDeltaY(Point initialPoint, int deltaY){

    //validate params
    if(deltaY>=tileMap.getTileHeight()) throw new IllegalArgumentException("Cannot validate if deltaY is larger than brickHeight");
    if(deltaY==0) return deltaY;

    int x = initialPoint.x;
    int testY = initialPoint.y + deltaY;

    if(isInsideTile(x, testY)){

        //get which row we are colliding with
        int mapRow = testY/tileMap.getTileHeight();
        //the distance between our position and
        //the top of the tile we are colliding with
        int topOffset = testY - (mapRow * tileMap.getTileHeight());
        //get the amount we should move to place
        //ourselves just next to the tile (top or bottom)
        int maxDeltaY = 0;

        //moving up case
        if(deltaY <0){
            maxDeltaY = deltaY + (tileMap.getTileHeight()-topOffset);
        }
        //moving down case
        else if(deltaY >0){
            maxDeltaY = deltaY - topOffset;
        }
        return maxDeltaY;
    }
    //we won't collide so it's okay to move deltaY
    return deltaY;
}

The second method does the same thing in the x axis:

public int getMaxDeltaX(Point initialPoint, int deltaX){
    //validate params
    if(deltaX>=tileMap.getTileWidth()) throw new IllegalArgumentException("Cannot validate if moveStep is larger than brickWidth");
    if(deltaX==0) return deltaX;

    int testX = (initialPoint.x + deltaX);
    int y = initialPoint.y;

    //if we collide
    if(isInsideTile(testX, y)){

        //get which column we are colliding with
        int mapCol = testX/tileMap.getTileWidth();
        //the distance between our position and
        //the left side of the tile we are colliding with
        int leftOffset = testX - (mapCol * tileMap.getTileWidth());
        //get the amount we should move to place
        //ourselves just next to the tile (left or right)
        int maxDeltaX = 0;

        //moving left
        if(deltaX < 0){
            maxDeltaX = deltaX + (tileMap.getTileWidth() - leftOffset);
        }
        //moving right
        else if(deltaX > 0){
            maxDeltaX = deltaX - leftOffset;
        }

        return maxDeltaX;
    }
    //we won't collide so it's okay to move deltaX
    return deltaX;
}

The algorithm (from the book) is essentially the same so I wanted to remove the duplication.

The code differs in only a few places:

use getTileWidth() or getTileHeight()
use x or y to create test point
use x or y to create offset
use mapRow or mapCol

So a basic way could be:

pass tile width/height as argument tileDimension
create test point before calling method (turn initialPoint into testPoint)
pass test x/y as argument testCoordinate
pass row/col as argument?  Can't think of good name (mapIndex?)

But now I have loads of confusing arguments and duplicate information (testCoordinate is contained in testPoint). I read that this is also a big no no, and even a beginner like me can see that this would be ugly.

Another way might be to pass the basic information and a flag for the axis, x or y. But this would still involve a bunch of arguments and then confusing switching in the body.

So I'm hoping there's a simple pattern that is used in these situations and will magically get rid of all duplication and leave a short method that's easy to read and use...

What is the best way to refactor these methods?

It seems this is a job for a Parameter class.

public abstract class DeltaParameters{
  protected ? tileMap;
  protected int x, y;

  DeltaParameters(Point point, ? tileMap){
    this.x = point.x;
    this.y = point.y;
    this.tileMap = tileMap;
  }

  public int getX(){
    return x;
  }

  public int getY(){
    return y;
  }
  public abstract int getTileSideSize();
  public abstract int getMovingCoordinate();
  public abstract void offsetMovingCoordinate(int delta);
}

public class DeltaXParameters extends DeltaParameters{
  DeltaXParameters(Point point, ? tileMap){
    super(point, tileMap);
  }

  public int getTileSideSize(){
    return tileMap.getTileWidth();
  }
  public int getMovingCoordinate(){
    return x;
  }
  public void offsetMovingCoordinate(int delta){
    x += delta;
  }
}

I leave to you the job of guessing what would be DeltaYParameters :p

public int getMaxDelta(DeltaParameters dparams, int delta){
    if(delta>=dparams.getTileSideSize()) throw new IllegalArgumentException("This message is yet to be determined. It could be another method in DeltaParameters");
    if(delta==0) return delta;

    dparams.offsetMovingCoordinate(delta);
    if(isInsideTile(dparams.getX(), dparams.getY())){
        int map = dparams.getMovingCoordinate() /dparams.getTileSideSize() ;
        int offset = dparams.getMovingCoordinate() - (map * dparams.getTileSideSize() );
        int maxDelta = 0;

        if(delta < 0){
            maxDelta = delta + (dparams.getTileSideSize() - offset);
        }
        else if(delta > 0){
            maxDelta = delta - offset;
        }

        return maxDelta;
    }
    return delta;
}

Now you could call getMaxDelta just like this

getMaxDelta(new DeltaXParameters(new Point(0, 0), tileMap), 5);

What I have done here is to take the advantage of polymorphism over DeltaParameters class. Which could not be a so straigthforward method to refactor is now proven to be in a quite simple fashion imho.


Non-curses pager in C (revision 2) In this revision (follow-up of this post) I've fixed reading from stdin, and also tried to minimise the damage caused by typing additional characters. You might want to see my original post on this.

#include <stdio.h>
#include <stdlib.h>

/*
 * A very SIMPLE pager that does not require curses. If your curses library
 * is broken, this is practically the only portable one you can easily get.
 */

int
main(
    int argc,
    char *argv[])
{
    FILE *fp;
    FILE *tty;
    int ch;

    if (argc < 2) {
        fp = stdin;
    } else if ((fp = fopen(argv[1], "r")) == NULL) {
        perror(argv[1]);
        return EXIT_FAILURE;
    }

    if ((tty = fopen("/dev/tty", "r")) == NULL) {
        perror("/dev/tty");
        return EXIT_FAILURE;
    }

    while ((ch = fgetc(fp)) != EOF) {
        if (ch == '\n') {
            while (fgetc(tty) != '\n');
        } else {
            putchar(ch);
        }
    }

    if (ferror(fp)) {
        perror(argv[1]);
        return EXIT_FAILURE;
    }

    fclose(fp);
    return EXIT_SUCCESS;
}

Some questions:

How can I avoid having to read from /dev/tty? I know that the device file is available on practically all Unixes, but it won't work on Windows.
Also, I want to avoid dependency on any library except standard C.

Making the code portable again
You did a fine job cleaning up last code's bugs. My answer will be fairly short this time.

How can I avoid having to read from /dev/tty? I know that the device
file is available on practically all Unixes, but it won't work on
Windows.

Well, I have some good news and some bad news.
Bad News
The bad news is that I'm pretty sure the only way you aren't going to get that bug where you substitute the file for STDIN is if you continue reading from /dev/tty. After all, you can't possibly read from STDIN because that's where the file is.

You can stop now. As a quick and smart fix, you can drop support for reading from STDIN (I mean, who would use a pager to read something they are typing into STDIN?). Or, if you want you can read on for the good news.

Good News
There's a solution that allows you to keep on reading from /dev/tty and that is to use the amazing Windows API. Luckily, you can do that in C because Windows provides a header file windows.h that gives you full access to the Windows API in your C/C++ code.
Now, you may think to just include the windows.h file in your code and use whatever the API has to read the keyboard. Well, you can't quite do that because if you try compiling that on *nix, you'll get an error saying that the windows.h could not be found.
An easy fix is to use the pre-processor:
#include <stdio.h>
#include <stdlib.h>

#ifdef _WIN32
#include <windows.h>
#endif

This will make sure to only include the windows.h file if the operating system is a Windows one.
Then, in the section where you would normally open and read from the /dev/tty file, you would encase that in:
#ifdef linux
...
#endif

and the Windows code in the respective pre-processor statements.
As for what actually goes inside the Windows code sections, I am unsure. Through some quick searching, I came across ReadConsole in the API index. However, I have not tested it out. If that does not work, feel free to explore the API for other possibilities.


Find the smallest number of perfect squares that sum to a target Given a positive integer n, find the smallest number of perfect squares (for example, 1, 4, 9, 16, ...) that sum to n.

sums = {0:0, 1:1}
def numSquares(n: int) -> int:
    if n in sums:
        return sums[n]
    else:
        s = 2**31
        squares = [i**2 for i in range(1, int(n**(1/2)) + 1)]
        while len(squares) and squares[-1] > n:
            squares.pop()
        for i in squares:
            s = min(s, numSquares(n - i) + 1)
        sums[n] = s
        return s

I'm also having a bit of trouble calculating the time complexity (using Big O notation) due to the recursion.

I understand that it's at least \$O(\sqrt n)\$ due to the loop but I'm unsure of how to factor in the recursive call.

Thank you very much!

Caching

You are using sums as a cache for your dynamic programming.  Python comes with built-in caching (functools.lru_cache), so you don't need to implement your own.  Using it will avoid polluting the global namespace.

Integer Square-Root

With Python3.8 comes a built-in math.isqrt, rendering the idiom int(n**(1/2)) obsolete.

Unnecessary filtering

    squares = [i**2 for i in range(1, isqrt(n) + 1)]
    while len(squares) and squares[-1] > n:
        squares.pop()

Here, you are generating all the squares up to (and including if it is a perfect square) n.  Then, you check if the last item is larger than n.  It is impossible for it to be, so squares.pop() will never be executed, making the entire while loop redundant code which can be deleted.

Unnecessary (and suspicious) minimum

    s = 2**31
    for i in squares:
        s = min(s, numSquares(n - i) + 1)

Here, 2**31 is used as a value larger than any possible value, so you can compute the minimum in the for loop.  But is it really larger than any possible value???  I can think of larger values, but perhaps it comes from the challenge text.

Still, Python provides a better way: the min(...) function and list comprehension:

    s = min(numSquares(n - i) + 1 for i in squares)

This loops through all squares, and evaluates numSquares(n - i) + 1 for each one, and selects the minimum value.  No magic "value larger than any possible value" value is required.

Still, it can be made even more efficient.  If there are 10,000 squares, we add one to each result, and take the minimum.  That's 10,000 plus one operations.  If we compute the minimum value, and add one to that, we've saved 9,999 additions.

    s = min(numSquares(n - i) for i in squares) + 1

Unnecessary list creation

The code would now read:

    squares = [i**2 for i in range(1, isqrt(n) + 1)]
    s = min(numSquares(n - i) for i in squares) + 1

We create a list of squares and then immediately loop through that list exactly once, and never use it again.  Creating the list is an unnecessary step, that consumes memory and wastes time.  Just compute the squares on the fly!

    s = min(numSquares(n - i ** 2) for i in range(1, isqrt(n) + 1)) + 1

PEP-8

Functions and variables should be in snake_case, not mixedCase.  Your function should be named num_squares.

There should be a space around binary operators like ** and /.

Variable names like s are meaningless.  Variable names should be descriptive.  s is the minimum count of the terms summing to n; min_terms would be more descriptive.

Updated Code

from functools import lru_cache
from math import isqrt

@lru_cache(maxsize=None)
def num_squares(n: int) -> int:

    root_n = isqrt(n)
    if root_n ** 2 == n:
        return 1

    return min(num_squares(n - i ** 2) for i in range(1, root_n + 1)) + 1

Further improvements

Consider 10001.  It is not a perfect square.  So you start subtracting squares off it, beginning with \$1^2\$ and discover \$10001 - 1^2 = 100^2\$, for min_terms of 2.  You should stop searching; there will be no better answer.  But instead, you will continue with \$10001 - 2^2\$, and figure out the num_squares() of that, and then try
\$10001 - 3^2\$ and so on.  This is all busy work.

Determining how to prune the search tree left as exercise to student.


node.js game CPU usage My friend and I are making a node.js game, and we have been testing CPU. After profiling, it was determined that this process called zlib is sucking most of the CPU/RAM.

3 clients connected to a game is fine, but when 12~13 players are connected it uses 58% where zlib is using about 30% of this CPU.

inclusive      self           name
ticks   total  ticks   total
64775   58.5%  64775   58.5%  /lib/x86_64-linux-gnu/libc-2.19.so
25001   22.6%    224    0.2%  LazyCompile: *callback zlib.js:409
//this one is a different zlib
7435    6.7%     82    0.1%  LazyCompile: ~callback zlib.js:409

Is there any way to decrease the CPU usage from this? Or is there a reason why it is increasing so much?

I have done some reading and I am told it is from socket.io, so here is our section of socket sending most of the data:

for (var i = 0; i < users.length; i++) {
    if (u.room == users[i].room && users[i].x + users[i].radius >= u.x - u.screenWidth / 2 - 20 && users[i].x - users[i].radius <= u.x + u.screenWidth / 2 + 20 && users[i].y + users[i].radius >= u.y - u.screenHeight / 2 - 20 && users[i].y - users[i].radius <= u.y + u.screenHeight / 2 + 20) {
        if (users[i].id == u.id) {
            visiblePlayers.push({
                x: users[i].x,
                y: users[i].y,
                angle: users[i].angle,
                hue: users[i].hue,
                radius: users[i].radius,
                squeeze: users[i].squeeze,
                name: users[i].name,
                dead: users[i].dead,
                isPlayer: true,
                kills: users[i].kills
            });
        } else {
            visiblePlayers.push({
                x: users[i].x,
                y: users[i].y,
                angle: users[i].angle,
                hue: users[i].hue,
                radius: users[i].radius,
                squeeze: users[i].squeeze,
                name: users[i].name,
                dead: users[i].dead
            });
        }
        // SEND DYING INFO: (FOR OFFLINE ANIMATION):
        if (users[i].dying) {
            visiblePlayers[visiblePlayers.length - 1].dying = true;
        }
    }
}
var visibleEnergy = [];
for (var i = 0; i < energies.length; i++) {
    if (u.firstSend || (energies[i].updated && energies[i].room == u.room)) {
        var anim = energies[i].animate;
        if (u.firstSend)
            anim = true;
        visibleEnergy.push({
            x: energies[i].x,
            y: energies[i].y,
            radius: energies[i].radius,
            index: i,
            animate: anim,
            hue: energies[i].hue,
            room: energies[i].room
        });
    }
}

// SEND PLAYER UPDATES TO CLIENTS:
sockets[u.id].emit('serverTellPlayerMove', visiblePlayers,
    visibleEnergy);

As I wrote in a comment, changing the code here won't change zlib usage, as that's something that's happening at a lower level. Still, I wanted to review the code. However, as TastyLemons rightfully notes in a comment, if you can simply send less stuff all the time, it'll help. But with that much CPU usage for zlib, I'm guessing there's something wonky in the node/zlib/server configuration.

For the first loop (users loop):

That is a very long and complex if condition! And it's filled with magic numbers, as well as numbers that I assume you don't need to calculate over and over again.
The object you're building is mostly the same in both the inner branches, so it'd be easier to build the common part first, and then only add the parts that differ.
There are other small changes that would make things simpler, like setting the dying boolean in the object before pushing it to the array.
You can make good use of the map and filter array methods

For the condition, you're checking if the user is in the same room, and in view of the u user (Sidenote: u is not a terribly descriptive label, and it doesn't help that you sometimes use player and other times use user to refer to the same kind of entity).

I'd make that a method, for instance on the User prototype, if you have such a thing:

User.prototype.canSee = function (otherUser) {
  // initial check
  if(this.room !== otherUser.room) {
    return false;
  }

  // x and y distances between user and other's "radius"
  var dx = Math.abs(otherUser.x - this.x) - otherUser.radius,
      dy = Math.abs(otherUser.y - this.y) - otherUser.radius;

  return dx <= this.xViewDistance && dx <= this.yViewDistance;
};

(if you don't have a User prototype, you can change the function to be a plain function that takes two arguments.)

I'm assuming here, that the *ViewDistance values are already calculated, since I imagine they don't really change (or at least don't change all the time). So if the screenWidth doesn't change much, you could calculate a screenHalfWidth and keep it around, so you don't always have to divide by 2 to get the same result. And the 20 needs to be a needs constant (I know JS doesn't have constants; I mean a variable you treat as constant). Something like SCREEN_WIDTH_MARGIN and maybe a similar thing for height.

So xViewDistance might be defined as user.screenHalfWidth - SCREEN_WIDTH_MARGIN, which itself is a value that probably won't change all the time, and can thus be stored.

Point is, you end up with code, where:

your magic numbers are labelled so you know what they mean
your magic numbers are defined in exactly 1 place, so you only need to hunt down the same number in 23 different places if it needs to change
calculations are done once, and the results stored, rather than recalculated constantly despite not having changed.

For building the objects I'd again suggest a method on the User prototype (though again, it can also be a function that just takes an argument). Something like:

User.prototype.getInfo = function (isPlayer) {
  var info = {
    x: this.x,
    y: this.y,
    angle: this.angle,
    hue: this.hue,
    radius: this.radius,
    squeeze: this.squeeze,
    name: this.name,
    dead: this.dead,
    dying: this.dying
  };

  if(isPlayer) {
    info.isPlayer = true;
    info.kills = this.kills;
  }

  return info;
};

Putting it together, you might get:

var visibleUsers = users.filter(function (user) {
  return user.id === currentUser.id || currentUser.canSee(user);
}).map(function (user) {
  return user.getInfo(user.id === currentUser.id);
});

That's everyone's info, including the current user/the player, ready to be sent.

For the energies, the story's similar: Add some methods/functions to do the heavy lifting, and use filter/map to iterate the array. Only trick is that if firstSend is true, we could really skip the filter function, but I've left it pretty much as-is:

var visibleEnergies = energies.filter(function (energy) {
  return currentUser.firstSend || (energy.updated && energy.room === currentUser.room);
}).map(function (energy) {
  var info = energy.getInfo();
  info.anim = info.anim || currentUser.firstSend; // override anim if necessary
  return info;
});