Throttling REST requests in C# I'm rather new to C#, and found relevant subclassing examples surprisingly hard to come by. I've whipped up this class that appears to work, but am pretty sure this is not optimal:

private class ThrottledRestClient
{
    int pause;  // mininum ms between requests
    double rpm;  // max requests per minute
    long last_request_time;
    RestClient client;
    public ThrottledRestClient(string url, HttpBasicAuthenticator auth, double rpm = 60)
    {
        this.client = new RestClient(url);
        this.client.Authenticator = auth;
        this.rpm = rpm;
        this.pause = (int)((60 / rpm) * 1000);
    }

    public IRestResponse Execute(RestRequest request)
    {
        long now = DateTime.Now.Ticks;
        int wait = pause - (int)((now - this.last_request_time) / TimeSpan.TicksPerMillisecond);
        if (wait > 0)
        {
            //Console.WriteLine("Waiting ms: " + wait);
            Thread.Sleep(wait);
        }
        this.last_request_time = DateTime.Now.Ticks;

        return this.client.Execute(request);
    }
}

How could this be better? Is there an existing solution I haven't found?

I'm rather new to C#, and found relevant subclassing examples
  surprisingly hard to come by

You just needed to try a bit harder! You are doing pretty much what you were supposed to do logic wise. What you are trying to do here obviously (maybe), is a specialization of a RestClient, so the RestClient class is the best candidate to inherit from. From there you may override the Execute method.

Don't be afraid to use verbose variables. The rpm name could be firstly reminded as rotations per minute which doesn't really apply here. Also try to name your instances fields in camelCase, you may or may not prefix them with _.

I also didn't get why your pause is (60 * 1000) / rpm = 1000 with the default rpm, but one thing I know is that you don't need to store it in a field because it may be calculated. I would also recommend using Environment.TickCount instead of DateTime.Now.Ticks, which is more performant.

With those modifications the code would become the following:

public class ThrottledRestClient : RestClient
{
    private readonly int _requestsPerMinute;
    private int _lastRequestTime;

    public ThrottledRestClient(int requestsPerMinute)
    {
        _requestsPerMinute = requestsPerMinute;
    }
    public override IRestResponse Execute(IRestRequest request)
    {
        int elapsedTime = Environment.TickCount - _lastRequestTime;
        int pause = (60/_requestsPerMinute)*1000;
        int wait = pause - elapsedTime;
        if (wait > 0)
        {
            Thread.Sleep(wait);
        }
        var response = base.Execute(request);
        _lastRequestTime = Environment.TickCount;
        return response;
    }
}

EDIT: To answer you comment. You can use this client like you would use the previous one eg:

var client = new ThrottledRestClient(60);
client.Authenticator = new HttpBasicAuthenticator("user", "password");
//so on so forth;
client.BaseUrl = "http://google.pt";
client.Request(new RestRequest());


Determine the third octet of an IP address based on user entered site number I am trying to create a function for a larger script to determine the third octet of a site IP address based off a user entered site number. I have actually come up with the function, but was hoping that someone could help me come up with something that is simpler and more elegant, instead of the multitude of nested if statements that I came up with. Here are the rules:

For sites numbered 0001 - 0255, the third octet is the last 3 digits of the site number, less any zeroes preceding a real number (e.g. Site 0007 = 10.10.7.xxx)
For sites numbered 0256 - 0399 and 0500 - 1999 (except 1202), the third octet is the last 2 digits of the site number less any zeroes preceding a real number (e.g. site 0315 = 10.22.15.xxx)
For sites numbered 401 - 499, the third octet is the last two digits of the site number +100 (e.g. 0401 = 10.20.101.xxx)
Site 1202 is an exception to the above rule, where the third octet is the last 3 digits (.202)
For sites 5000 and above (5000 - 9999), the first digit is dropped, and then the rules above are followed. (e.g. 8248 = 10.12.48.xxx)

Here is what I have come up with. It is not very elegant, but it seems to work. Note that the first three lines will be in another function in the script, but are here for testing purposes.

$global:site = Read-Host 'What is the 4 digit site number?' #must include leading zeros
[int]$global:sitecheck=$site #need for comparasion operators
[string]$global:sitestring=$site

if ($sitecheck -le 255)
    {
    if ($sitestring.substring(0,3) -eq '000') #(i.e. 0001 - 0009)
        {
        $global:thirdoct = $sitestring.substring(3,1) #(i.e. 1-9)
        }
        elseif ($sitestring.substring(0,2) -eq '00') #(i.e. 0010 - 0099)
            {
            $global:thirdoct = $sitestring.substring(2,2) #(i.e. 10 - 99)
            }
            elseif ($sitestring.substring(0,1) -eq '0') #(i.e. 0100 - 0255)
            {
            $global:thirdoct = $sitestring.substring(1,3) #(i.e. 100 - 255)
            }
}
elseif ($sitecheck -ge 256 -and $sitecheck -le 399 )
    {
    if ($sitestring.substring(2,1) -eq '0')
        {
        $global:thirdoct = $sitestring.substring(3,1)
        }
        else
        {
        $global:thirdoct = $sitestring.substring(2,2)
        }
}
elseif ($sitecheck -ge 401 -and $sitecheck -le 499)
    {
    $global:thirdoct = 100+$sitestring.substring(2,2)
    }
elseif ($sitecheck -ge 501 -and $sitecheck -le 799)
    {
    if ($sitestring.substring(2,1) -eq '0')
        {
        $global:thirdoct = $sitestring.substring(3,1)
        }
        else
        {
        $global:thirdoct = $sitestring.substring(2,2)
        }
    }
elseif ($sitecheck -eq 1202)
    {
    $global:thirdoct = $sitestring.substring(1,3)
    }
elseif ($sitecheck -ge 5000 -and $sitecheck -le 9999)
    {
    $site5000 = [int]$sitestring.substring(1,3)
    if ($site5000 -le 255)
       {
       if ($sitestring.substring(1,2) -eq '00')
           {
           $global:thirdoct = $sitestring.substring(3,1)
           }
                elseif ($sitestring.substring(1,1) -eq '0')
                {
                $global:thirdoct = $sitestring.substring(2,2)
                }
                    else
                    {
                    $global:thirdoct = $sitestring.substring(1,3)
                    }
    }
    elseif (($site5000 -ge 256 -and $site5000 -le 399) -or ($site5000 -ge 501 -and $site5000 -le 999))
        {
        $global:thirdoct = $sitestring.substring(2,2)
        }
    elseif ($site5000 -ge 401 -and $site5000 -le 499)
        {
        $global:thirdoct = 100+$sitestring.substring(2,2)
        }
    }

Let's start with a table of inputs and outputs, because it may not be exactly the same as what you expect from your script.  The first column is the input range, the other column is the transformation applied for those inputs.

0000-0255       self
0256-0399       last2
0400            undefined
0401-0499       -300
0500            undefined
0501-0799       last2
0800-1201       undefined
1202            last3
1203-4999       undefined
5000-5255       last3
5256-5399       last2
5400-5500       undefined
5501-5999       last2
6000+           same as 5000-5999

The best way to manage these cases is with a data structure.  A switch statement can work too but there's going to be a lot of repetition, and you really want something that lets you focus on the data.

In this version, the ranges are an array of arrays.  Each inner array is a lower bound, an upper bound, and a script block that transforms input to output.  As a special case, a negative result from the script block means "restart the conversion with this new value (after making it positive)"—this lets us reuse the low patterns for most of 5000-5999, and then reuse the 5000-5999 pattern for 6000-9999.

We simply traverse the array looking matches, and apply the transforms from matching rows. (There's only ever one matching row per call to convert, in this example, but the code allows overlapping ranges; matching transforms will apply in order).

$ranges = @(
    @(    0,  255, { $_ }                  ),
    @(  256,  399, { $_ % 100 }            ),
    @(  401,  499, { $_ - 300 }            ),
    @(  501,  799, { $_ % 100 }            ),
    @( 1202, 1202, { $_ % 1000 }           ),
    @( 5000, 5399, { -($_ % 1000) }        ), # redo using last 3 digits
    @( 5501, 5999, { -($_ % 1000) }        ),
    @( 6000, 9999, { -($_ % 1000 + 5000) } )  # redo using 5000 + last 3
)

$convert = {
    $_ = [int]$args[0]
    $out = -1
    foreach ($range in $ranges) {
        if ($_ -ge $range[0] -and $_ -le $range[1]) {
            $out = & $range[2]
            # negative result means re-do the conversion with a new value
            if ($out -lt 0) { $out = & $convert ($out * -1) }
        }
    }
    return $out
}

$site = Read-Host 'enter the site code: '
$octet = & $convert $site
if ($octet -ge 0) {
    echo $octet
} else {
    echo "error - $site has no defined octet"
}


Asynchronous retry method I've written an asynchronous retry method as an answer for this question. I'd like to get your opinion of the implementation and whether there are better ways to implement this. You could also implement this with async-await but I thought this would be a more efficient implementation.

public static Task RetryAsync(Func<bool> retryFunc, CancellationToken cancellationToken, int retryInterval)
{
    var tcs = new TaskCompletionSource<object>();

    var timer = new Timer((state) =>
    {
        var taskCompletionSource = (TaskCompletionSource<object>)state;

        if (!taskCompletionSource.Task.IsCompleted)
        {
            try
            {
                if (cancellationToken.IsCancellationRequested)
                {
                    taskCompletionSource.SetException(new TaskCanceledException("RetryAsync cancelled"));
                }
                else if (retryFunc())
                {
                    taskCompletionSource.SetResult(null);
                }
            }
            catch (Exception ex)
            {
                taskCompletionSource.SetException(ex);
            }
        }

    }, tcs, 0, retryInterval);

    //// Once the task is complete, dispose of the timer so it doesn't keep firing.
    tcs.Task.ContinueWith(t => timer.Dispose(),
                          CancellationToken.None,
                          TaskContinuationOptions.ExecuteSynchronously,
                          TaskScheduler.Default);

    return tcs.Task;
}

You could also implement this with async-await but I thought this would be a more efficient implementation.

When performance matters, don't guess, measure. When it doesn't matter (which is 97 % of the time according to some), write code that is readable and maintainable.
Why do you think small increase in efficiency (most likely less than 1 ms) would matter here, when the retry interval is probably going to be hundreds of milliseconds or more (and can't effectively be less than 15 ms)?

Func<bool> retryFunc

Consider adding another overload that allows you to retry async functions (i.e. Func<Task<bool>> retryFunc).

CancellationToken cancellationToken

It might make sense to make this an optional parameter, some users might not need cancellation.

int retryInterval

I think it would be better to use TimeSpan here, that way both your code and the code of your users becomes more clear. If you want to keep using int, document very clearly the unit used, possibly by even renaming the parameter to something like retryIntervalMs.

You don't want to be the next Mars Climate Orbiter.

When the CancellationToken is canceled, why do you wait for the timer tick to cancel the returned Task? You could use Register() to make sure the Task is canceled as soon as the CancellationToken is.

if (!taskCompletionSource.Task.IsCompleted)

What's the purpose of this check? If it's because you're worried that retryFunc might run longer than retryInterval, then I think your logic is flawed. When I have an operation that takes 1 minute to run and I ask for retry after 5 seconds, I probably don't want to have 12 instances of the operation running at the same time.

You could achieve this by using Change().

And even when you want this behavior, you should probably also switch to using TrySet versions of the TaskCompletionSource methods, to avoid unnecessary exceptions.

if (cancellationToken.IsCancellationRequested)
{
    taskCompletionSource.SetException(new TaskCanceledException("RetryAsync cancelled"));
}

This way, the Task will be in the Faulted state. To get it to the correct Canceled state, use SetCanceled().

//// Once the task is complete, dispose of the timer so it doesn't keep firing.
tcs.Task.ContinueWith(t => timer.Dispose(),
                      CancellationToken.None,
                      TaskContinuationOptions.ExecuteSynchronously,
                      TaskScheduler.Default);

This closure is important also because it keeps the Timer rooted, so it prevents it from being GCed prematurely. I would expand the comment to explain that.

(Also, why are you using four slashes for a comment? Two are enough.)


Directory size checker So I semi-wrote directory size checker by copying some parts from Stackoverflow.
It works as follows:

Select a directory you want to check the size of subdirectories inside.
It goes scans all the directories inside the chosen directory. If there are also just files, that's not a problem the recursive function can deal with that as well.
It goes through every subdirectory and checks for the file size. If there's a permission problem, it returns -1 (this way we can easily tell which directories caused acces issues).
The directory name and it's "mass" gets stored in a dictionary.
The whole dictionary gets sorted.
Items that are +1GB get the "GB" tag, while the ones between 0 and 1 get the "MB" tag, just for better precision.
(I like to change between directories faster, so I added an option to give "r" as an input to scan for a second time. Closing the script and reopening it takes time imo)

Now I can see which folder is the biggest and which ones are empty, I don't know why but in Windows I need to go 1:1 and check each folder size individually, which I dislike a lot. This way I can check folders that contain hundreds of gigabytes in seconds.
I would like to know what I could have done better or what was obnoxious.
import os
from tkinter import *
from tkinter.filedialog import askdirectory

def Main():
    Tk().withdraw()  # use to hide tkinter window
    Dir = askdirectory(title="Select Folder")  # shows dialog box and return the path

    print("LOADING...")  # Sometimes it takes long to output results, this just indicates that the program is working and not broken
    sort_dict = Sorter(Dir)  # Give the directory as an input

    print("\n-SORTED-\n")
    for i in sort_dict:
        if i[1] > 1:  # continues using gigabytes
            fixed_float = float("{:.3f}".format(i[1]))
            data_type = "GB:   "
        elif i[1] < 0:  # no acces
            print(f"NO ACCES : {i[0]}")
            continue
        else:  # convert to megabytes
            mega_float = i[1] * 1024
            fixed_float = float("{:.3f}".format(mega_float))
            data_type = "MB:   "

        dflt_white_space = 8
        white_space = " " * (dflt_white_space - len(str(fixed_float)))
        print(f"{fixed_float}  {white_space + data_type + i[0]}")

    if input("\nPress 'r' to retry, press anything else to exit\n") == "r":
        Main()

def get_dir_size(Dir):  # Returns the `directory` size in bytes.
    total = 0
    try:
        for entry in os.scandir(Dir):  # print("[+] Getting the size of", directory)
            if entry.is_file():  # if it's a file, use stat() function
                total += entry.stat().st_size
            elif entry.is_dir():  # if it's a directory, recursively call this function
                total += get_dir_size(entry.path)
    except NotADirectoryError:  # if `directory` isn't a directory, get the file size
        return os.path.getsize(Dir)
    except PermissionError:  # if for whatever reason we can't open the folder, return -1
        return -1  # returns a negative size that will be visible (to use conditions) after the conversion
    return total

def get_subdirs(Dir):
    dir_array = []
    for x in os.listdir(Dir):
        dir_array.append(x)
    return dir_array

def Sorter(Dir):

    Dict = {}
    dir_array = get_subdirs(Dir)

    for x in range(len(dir_array)):
        file_size = float(get_dir_size(Dir + "\\" + dir_array[x])) / 1073741824  # Converts the bytes to GB

        Dict[dir_array[x]] = file_size

    sort_dict = sorted(Dict.items(), key=lambda x: x[1], reverse=True)
    return sort_dict

if __name__ == "__main__":
    Main()

Function equal to built-in
def get_subdirs(Dir):
    dir_array = []
    for x in os.listdir(Dir):
        dir_array.append(x)
    return dir_array

This is exactly the same as os.listdir, so just use it instead of the whole function.
Iterate simpler
for x in range(len(dir_array)):
    file_size = float(get_dir_size(Dir + "\\" + dir_array[x])) / 1073741824  # Converts the bytes to GB

    Dict[dir_array[x]] = file_size

Can become:
for dir in dir_array:
    file_size = float(get_dir_size(Dir + "\\" + dir)) / 1073741824  # Converts the bytes to GB

    Dict[dir] = file_size

Constants at the start
Putting constants at the start of the code is better than putting them inside, so at the start:
BYTES_IN_GIGABYTE = 1073741824

User message
Your code works for files so:
Dir = askdirectory(title="Select Folder")

becomes:
Dir = askdirectory(title="Select Folder or File:")

Fancy progressbar
A fancy way to signify that the program is working can be found here
-1 lost in code
You return -1 sometimes from get_dir_size but you never check for it.
One comment on the logic
for i in sort_dict:
    if i[1] > 1:  # continues using gigabytes

I would rename i to dir and add a comment saying what i[1] is.
Cross platform compatibility
As suggested by @s.dallapalma avoid concatenating paths with the plus (+) sign, and use os.path.join or pathlib instead. Thus, the code can work on different OS.
I liked the attention to handling of exception and the new clean format strings.


Nodejs functions to load and save RSA keys I'm creating my first real life Nodejs project. I've created some files and all work, but I'm not satisfied about what my files look likes. It's unreadeble.

For example, I work of a file to manage JWT sessions and RSA keys.

For this file, I need two function to load and save the RSA private key.

This Node.js style guide says to put require at the top of documents.

'use strict';

const logger = require('./logger')(module.filename);
const NodeRSA = require('node-rsa');
const jwt = require('jsonwebtoken');
const fs = require('fs');

const loadKeyFromDisk = (path) => new Promise((resolve, reject) => fs.readFile(path, 'utf-8', (error, data) => {
  if (error) {
    reject(error);
    return;
  }
  resolve(data);
}));
const saveKeyToDisk = (path, key) => new Promise((resolve, reject) => fs.writeFile(path, key, 'utf-8', (error) => {
  if (error) {
    reject(error);
    return;
  }
  resolve();
}));

[...]

modules.exports = {...};

But, I also learn to put variables into small context as possible

'use strict';

const logger = require('./logger')(module.filename);
const NodeRSA = require('node-rsa');
const jwt = require('jsonwebtoken');

const { loadKeyFromDisk, saveKeyToDisk } = (() => {
  const fs = require('fs');

  return {
    loadKeyFromDisk: (path) => new Promise((resolve, reject) => fs.readFile(path, 'utf-8', (error, data) => {
      if (error) {
        reject(error);
        return;
      }
      resolve(data);
    })),
    saveKeyToDisk: (path, key) => new Promise((resolve, reject) => fs.writeFile(path, key, 'utf-8', (error) => {
      if (error) {
        reject(error);
        return;
      }
      resolve();
    }))
  };
})();

[...]

modules.exports = {...};

I've also tried some others style, but I think it's don't look like professionnal and clean code...

'use strict';

const logger = require('./logger')(module.filename);
const NodeRSA = require('node-rsa');
const jwt = require('jsonwebtoken');
const fs = require('fs');

const loadKeyFromDisk = (path) => {
  return new Promise((resolve, reject) => {
    fs.readFile(path, 'utf-8', (error, data) => {
      if (error) {
        reject(error);
        return;
      }
      resolve(data);
    });
  });
};
const saveKeyToDisk = (path, key) => {
  return new Promise((resolve, reject) => {
    fs.writeFile(path, key, 'utf-8', (error) => {
      if (error) {
        reject(error);
        return;
      }
      resolve();
    });
  });
};

[...]

modules.exports = {...};

Did you have somes advices or idea to improve my coding style?

My complete code Here : https://pastebin.com/kXvxNd2k

require style

I (and most of the Node developers I've worked with) prefer the style where require is put on top. As stated in the guide you linked, it gives a clearer overview of what are dependencies of the module/file you're writing. No one likes to see that if some condition is successful, then a new module will be loaded. It's just unnatural.

On the other hand, you shouldn't look at const foo = require('bar') as just another variable assignment and treat it as such when you're writing Node or any other JS code. Calling require function will load a file (if not previously loaded, read more here) and make its exported object available through the variable in the file you're writing. This same thing in some other language like C/C++ would be expressed as #include <foo>, which exposes the whole file to another file.

Other things I've noted

Usage of fs module

I see that you have helper functions that wrap fs.readFile and fs.writeFile in promises. I'm not sure if you are aware of 2 things:

There is a promisify method which can do this for you.
There already exist promisifed versions of this functions.

My suggestion is to use fs.readFileSync and fs.writeFileSync instead of wrapping them yourself.

Line 55

I'm not sure what was your intention here, but you've just called a function which returns a promise without waiting for the promise to be resolved. Since you're writing to the disk, this operation might not finish once a function from your returned object gets called. I'd just add await here.

Line 73

First, add a space between the ) and ? to be consistent with the style you have for : and stuff around it.
Second, break that long condition into either a function, or to a several variables. I'd personally go here with functions:

const notInvalidated = (invalidations, payload) => !invalidations[payload.userUuid] || payload.invalidationCounter === invalidations[payload.userUuid]
const hasValidIat = (invalidBefore, payload) => payload.iat >= invalidBefore

And then your condition could be written as:

notInvalidated(invalidationByUserUUID, payload) && hasValidIat(invalidBefore, payload) ? payload : false

Still long, but more readable.

module.exports mess

My personal preference is to have module.exports either export a function or an object, so make it as short as possible. But from your code on pastebin, it looks like your core logic is put there. For me, this is unreadable and I would look how to refactor this into something which will return either a single function or just a simple object.
In your case, I'd first move that function to a new named function, something like this:

const myJwt = async (keyPath) => { /* the rest */ }

module.exports = myJwt

From there, I'd move the functions in the object you are returning to the file level:

const generateNewKeyPair = async () => { /* ... */ }
/* also for sign, verify and invalidateOldUserToken */

const myJwt = async (keyPaht) => {
  /* whatever goes here */
  return {
    generateNewKeyPair,
    sign,
    verify,
    invalidateOldUserToken,
  }
}

module.exports = myJwt

This refactoring will be a bit tricky, because you have these two variables invalidationByUserUUID and invalidBefore which should be shared with the functions that I suggested extracting. You should maybe consider to wrap this in a class and return the class instead of a function. Either that, or you'll have to pass invalidationByUserUUID and invalidBefore with every function call.

Hope my comments make sense. I didn't do a whole code refactoring since I'm not 100% sure what you wanted to achieve, but if needed, I can give it a try.


Replacing the occurence of the character in string Given a string (for example: "a?bc?def?g"), write a program to
  generate all the possible strings by replacing ? with 0 and 1.

Example:
Input : a?b?c?
Output: a0b0c0, a0b0c1, a0b1c0, a0b1c1, a1b0c0, a1b0c1, a1b1c0, a1b1c1.

I have developed a program as shown below but please advise if something more efficiently can be done.

public class ReplaceQuestionMark {
    public ArrayList<String> replace(String target){
        return replaceHelper(target, target.length()-1);
    }

    public ArrayList<String> replaceHelper(String target, int to){
        char c = target.charAt(to);
        if (to == 0){
            ArrayList<String> res = new ArrayList<String>();
            if (c == '?'){
                res.add("0");
                res.add("1");
            }
            else{
                res.add(c+"");
            }
            return res;
        }
        ArrayList<String> res = new ArrayList<String>();
        ArrayList<String> preRes = replaceHelper(target, to-1);
        if (c == '?'){
            for (String token: preRes){
                res.add(token + "0");
                res.add(token + "1");
            }
        }
        else{
            for (String token: preRes){
                res.add(token + c);
            }
        }
        return res;
    }

    public static void main(String[] args){
        ReplaceQuestionMark rqm = new ReplaceQuestionMark();
        ArrayList<String> res = rqm.replace("a?b?c?");
        for (String s: res){
            System.out.println(s);
        }
    }
}

An easy way to generate all of the possible 0 and 1 combinations is to use binary numbers.

If there are 3 ?'s then the combinations of 0's and 1's will be all binary numbers from 0 to 2^3 - 1:

0:  000
1:  001
2:  010
3:  011
4:  100
5:  101
6:  110
7:  111

So all you need to do is count the number of question mark characters (N) and then count from 0 to 2^N - 1 to generate all the possible 0 and 1 combinations.

Also I notice that half of these strings will have a 0 in the first position, and half will have a 1. I wonder if this feature could be used to reduce the amount of iteration you need to do.


Basic function to output repeating dates from iCal format I have a function that implements parts of reading iCal repetition rules, and returns a list of dates when a certain event takes place.

Questions/Concerns of mine

Any comments on the use of anonymous functions? I added them to keep the code as DRY as possible.
One thing I rather dislike is the line return strtotime('+' . $increment . ' ' . $rrule_freq_map[$rrule['FREQ']], $start_timestamp); which casts from integer to string and back for no good reason. Couldn't think of a better way to accomplish the adding of time.
On a Drupal specific note, maybe the use of date in $single_day_output() should be replaced with format_date, but I'm not sure if there are any gains from that.
$next_timestamp() will break if used in more than one location. Currently not an issue, would be interesting to hear suggestions on that.

/**
 * Render the date and times of events.
 *
 * Has some internal helper functions do not make sense outside of this scope.
 */
function _bornholm_custom_calendar_event_dates($datetime) {
  // Ensure we have the date_repeat_split_rrule().
  module_load_include('inc', 'date_api', 'date_api_ical');

  $start_timestamp = strtotime($datetime['value']);
  $start_day = date('Y-m-d', strtotime($datetime['value']));
  $end_day = (isset($datetime['value2'])) ? date('Y-m-d', strtotime($datetime['value2'])) : NULL;
  $end_timestamp = (isset($datetime['value2'])) ? strtotime($datetime['value2']) : NULL;
  $output = '';

  // Build the output for a single days event information.
  $single_day_output = function($timestamp) {
    $output = t('@day d. @date t. @time', array(
      '@day' => t(date('D', $timestamp)),
      '@date' => date('d.m.Y', $timestamp),
      '@time' => date('H:i', $timestamp),
    ));

    return ucfirst($output);
  };

  // Check whether or not the current has a repeat rule.
  $repeats = function ($datetime) {
    if ($datetime['rrule'] !== NULL) {
      list($rrule, $exceptions, $additions) = date_repeat_split_rrule($datetime['rrule']);
      if ($rrule['FREQ'] !== 'NONE') {
        return $rrule;
      }
    }

    return NULL;
  };

  // When an event repeats, step forward the timestamp once.
  $next_timestamp = function($rrule, $start_timestamp) {
    static $count = 0;

    $rrule_freq_map = array(
      'DAILY' => 'day',
      'WEEKLY' => 'week',
      'MONTHLY' => 'month',
    );

    $increment = $count++ * $rrule['INTERVAL'];

    return strtotime('+' . $increment . ' ' . $rrule_freq_map[$rrule['FREQ']], $start_timestamp);
  };

  if ($rrule = $repeats($datetime)) {
    for ($i = 0; $i < $rrule['COUNT'] && $i < 100; $i++) {
      $new_timestamp = $next_timestamp($rrule, $start_timestamp);

      if ($end_day === NULL || $start_day === $end_day) {
        $output .= $single_day_output($new_timestamp);

        if ($end_timestamp) {
          $output .= ' - ' . date('H:i', $new_timestamp + ($end_timestamp - $start_timestamp));
        }
      }
      else {
        $output .= $single_day_output($new_timestamp) . ' ' . t('until') . '
' . $single_day_output($new_timestamp + ($end_timestamp - $start_timestamp)) . '
';
      }
    }
  }
  else {
    if ($end_day === NULL || $start_day === $end_day) {
      $output = $single_day_output($start_timestamp);
      if ($end_timestamp) {
        $output .= ' - ' . date('H:i', $end_timestamp);
      }
    }
    else {
      $output = $single_day_output($start_timestamp) . ' ' . t('until') . '
' . $single_day_output($end_timestamp);
    }
  }

  return $output;
}

Something to keep in mind about anonymous functions (also known as lambda functions), is that they should only be used for tasks that are only going to be performed once. These functions are not compiled at runtime and so must be explicitly compiled each time they are run. Another thing to keep in mind, just because you can do something, doesn't mean you should. Sure this works, but its confusing and hard to read. Why use lambda functions at all? It looks like what you actually want is a class.

Judging by some of this code I'd hazard to say that you are coming from Java? I say this mostly because of the way you are throwing around lambda functions in a single "main" function. Reminds me of a lot of Java code I've seen. That and your calling an array a map. Just know that PHP is NOT Java. You don't need just one function/class, to house all your other functions. Just pretend they are in a class by default and you'll do fine. Or implement them to a PHP class, they are more or less the same. If you are not from Java, then you might want to look at class tutorials, but first forget you ever heard of lambda functions. You'll probably never need them.

Another thing, that main function, _bornhom_custom_calendar_event_dates(), is entirely too long for a function. Functions should do one thing, and do it well. Classes should also do just one thing, but on a larger scale by using many functions to do smaller things. For example: A class could get a valid date set and have many functions (or methods in a class context) that cared for getting single dates, checking if they were valid, and compiling them together. Abstracting these lambda functions will help immensely with this and the DRY principle.

For quoting DRY principle, I immediately spot code that violates it.

$start_timestamp = strtotime($datetime['value']);
$start_day = date('Y-m-d', $start_timestamp);

Instead of checking if $datetime['value2'] is set repeatedly, do it once to save processing time. And change that name. "value2" is entirely too vague. What differentiates it from value? What even is "value"? Try to be short, but descriptive. Describing what these "values" are supposed to contain will get you started.

$end_day = NULL;
$end_timestamp = NULL;

$value2 = isset( $datetime[ 'value2' ] ) ? strtotime( $datetime[ 'value2' ] ) : FALSE;
if( $value2 ) {
    $end_day = date( 'Y-m-d', $value2 );
    $end_timestamp = $value2;
}

Of course, if you could change your code so that $end_timestamp could have a default value of FALSE instead of NULL, you could just set $end_timestamp above and then check to see if $end_day needed to be set and completely bypass the need of a $value2 variable.

What is function t() as seen in your $single_day_output lambda function? Where does it come from? Just like above, please choose better names.

What is "rrule" and why does it have a child named "rrule"? Maybe the first should be called a "rule set"? I don't know, but the names should be unique.

Break up your long lines. There's no reason to have them so long.

$single_newday = $single_day_output($new_timestamp);
$time = $single_day_output($new_timestamp + ($end_timestamp - $start_timestamp))
$output .= $single_newday . ' ' . t('until') . '
' . $time . '
';

Right now I can't follow your code well enough to help you with any of your specific questions, but if you clean it up a bit I'll take another look. Hope this helps!


Parsing an XML file with elements for keys and values I'm parsing an XML file that is structured like this:

<root>
  <entry>
    <elem1> key </elem1>
    <elem2> value </elem2>
  </entry>
  <entry>
  ....
  </entry>
</root>

</root>

My first code was using .parse() and .getRoot() but I had memory issues (1.5G memory use for 350M file). I solved my problem using an iterator and .clear() after reading this paper.

import xml.etree.ElementTree

myDict = {}

xmlIterator = xml.etree.ElementTree.iterparse('file.xml')

while True:
    try:
        _ , elem1 = next(xmlIterator)
        _ , elem2 = next(xmlIterator)
        _ , _     = next(xmlIterator)

    except StopIteration:
        break

    myDict[elem1.text]=elem2

    elem1.clear()
    elem2.clear()
    _.clear()

The issue is I need to access several children at the same time:

The first one is my key <elem1>
The second one is the value I need <elem2>
The third is not relevant <entry>

I would like to know if this is a good practice, or if there is a better way to do that (better performance or readability).

I also tried to follow the "Easier to ask for forgiveness than permission" principle with the exception block for ending the loop, but I'm not sure about multiple statements in the same exception block.

In fact, I also don't understand why the third element I get with next() is <entry>.

This isn't the answer you want. But, _ is most commonly the discard variable.
You use it when you don't want to use the variable, but you need to specify a variable.
Take:

for _ in range(10):
    print('hi')

We don't want the output from the range, we just want to print 'hi' ten times.
And so you should not use this for the variable you use .clear() on.

I'd use zip rather than the try-except.
Sure there's Look Before You Leap (LBYL) and Easier to Ask for Forgiveness than Permission (EAFP).
But you don't need to use either here.
Also, by using zip you can change your while loop to a for loop too.

And so you should be able to get something like:

import xml.etree.ElementTree

elements = {}
xml_iterator = xml.etree.ElementTree.iterparse('file.xml')

for (_, elem1), (_, elem2), (_, elem3) in zip(xml_iterator, xml_iterator, xml_iterator):
    elements[elem1.text] = elem2
    elem1.clear()
    elem2.clear()
    elem3.clear()


Nginx config with some conditionals This config has 3 main features but there seems to be a lot of duplication and I wonder if I could improve it.

1 Detect all pngs and jpgs in static/img and try a webp version if the requesting browser supports it

2 Detect non-ES6 supporting browsers and serve site.babel.js, otherwise serve site.js which is un-babelified

3 Proxy all other requests to a node app running on port 3000

upstream node_upstream {
    server node:3000;
    keepalive 64;
}

#Required since SSL termination is higher up at the AWS load balancer
map $http_x_forwarded_proto $is_https {
  default off;
  https on;
}

map $http_accept $webp_suffix {
    default   "";
    "~*webp"  ".webp";
}

map $http_user_agent $script_file {
    default                                 "site.js";
    "~MSIE"                                 "site.babel.js";
    "~Trident"                              "site.babel.js";
    "~Opera.*Version/[0-9]\."               "site.babel.js";
    "~Opera.*Version/[0-1][0-9]\."          "site.babel.js";
    "~Opera.*Version/2[0-1]\."              "site.babel.js";
    "~AppleWebKit.*Version/[0-9]\..*Safari" "site.babel.js";
    "~Chrome/[0-9]\."                       "site.babel.js";
    "~Chrome/[0-2][0-9]\."                  "site.babel.js";
    "~Chrome/3[0-3]\."                      "site.babel.js";
    "~Chrome/4[0-3]\."                      "site.babel.js";
    "~Edge/1[0-3]\."                        "site.babel.js";
}

server {
    root /var/www/html/src;

    gzip on;

    # AWS traffic will come via a load balancer with the original protocol stored in http_x_forwarded_proto
    # If this is set to http then we need to redirect any request onto https equivalent
    # Should be ignored in non AWS environment as http_x_forwarded_proto should not be set
    if ($http_x_forwarded_proto = "http") {
      rewrite ^(.*)$ https://$http_host$1 permanent;
    }

    location = /static/site.js {
        root  /var/www/html/src;
        expires 1y;
        access_log off;
        add_header Cache-Control "public";
        try_files /static/$script_file =404;
    }

    location ~* ^/static/img/.+\.(png|jpg)$ {
        root /var/www/html/src;
        add_header Vary Accept;
        expires 1y;
        access_log off;
        add_header Cache-Control "public";
        try_files $uri$webp_suffix $uri =404;
    }

    location ~* ^/static/.*$
    {
        root  /var/www/html/src;
        expires 1y;
        access_log off;
        add_header Cache-Control "public";
    }

    location / {
        proxy_redirect off;
        proxy_set_header   X-Real-IP            $remote_addr;
        proxy_set_header   X-Forwarded-For  $proxy_add_x_forwarded_for;
        proxy_set_header   X-Forwarded-Proto $scheme;
        proxy_set_header   Host                   $http_host;
        proxy_set_header   X-NginX-Proxy    true;
        proxy_set_header   Connection "";
        proxy_http_version 1.1;
        proxy_pass         http://node_upstream;
    }

    error_log /var/log/nginx/error.log;
    access_log /var/log/nginx/access.log;
}

root

The root directive is inherited. You have the same statement in the server block, and repeated in a number of location blocks. Only the first statement is necessary. See this document for details.

location ~* ^/static/.*$

This can be replaced by the prefix location:

location /static/ { ... }

The prefix location is more efficient than the regular expression location. The precedence order is different, but that does not affect your current configuration. See this document for details.

rewrite ^(.*)$ https://$http_host$1 permanent;

You can replace this with a return statement, thus eliminating a regular expression.

return 301 https://$http_host$request_uri;

Nested location blocks

A number of statements are common to three location blocks, all of which represent URIs that begin with /static/. The expires, access_log and add_header directives are inherited.

You could restructure your locations as follows:

location /static/ {
    root  /var/www/html/src;
    expires 1y;
    access_log off;
    add_header Cache-Control "public";

    location = /static/site.js {
        try_files /static/$script_file =404;
    }
    location ~* ^/static/img/.+\.(png|jpg)$ {
        add_header Vary Accept;
        add_header Cache-Control "public";
        try_files $uri$webp_suffix $uri =404;
    }
}
location / {
    ...
}

This would improve efficiency as URIs which do not begin with /static/ will not need to be checked against the regular expressions.

The add_header directive has an additional inheritance rule. See this document for details.


Checking if there are tags or categories in the database The code below is used to check if there are tags or categories in the database. If they exist, they are not added to the database, otherwise they are added.

Is this code efficient and easy to read?

private void CreateTagCategoryBlog(string Tags, string category)
    {
        int lastInsertedId = postRepo.GetAll().LastOrDefault().BlogPostId;
        var allTags = tagRepo.GetAll().Where(e => e.Name.Contains(Tags)).ToList().DefaultIfEmpty();
        var allCategories = catRepo.GetAll().Where(e => e.Name.Contains(category)).ToList().DefaultIfEmpty();

        foreach (var x in GetItems<Tag>(Tags, tagRepo))
        {
            blogTag.Add(new BlogTag
            {
                TagId = x.TagID,
                BlogPostId = lastInsertedId,
                DateAdded = DateTime.Now,
                LastModifiedDate = DateTime.Now,
                IsDeleted = false
            });
        }

        foreach (var x in GetItems<Category>(category, catRepo))
        {
            blogCat.Add(new BlogCategory
            {
                BlogPostId = lastInsertedId,
                CategoryId = x.CategoryId,
                DateAdded = DateTime.Now,
                LastModifiedDate = DateTime.Now,
                IsDeleted = false
            });
        }
    }

Parameters should be camelCased, so Tags should be tags. As far as I can tell though only one tag is passed in, so it should be the singular tag.
x isn't a very good variable name. It looks like you used it as a generic name because you were copy/pasting. The two loops are slightly different, but not by a whole lot. Consider how you could possibly pass the string and repository into a method instead.


Rotation of elements in an array We had to write a program that would perform rotation of elements in an array. The array size was entered by user, so it had to be in a sense, dynamic.

Rotation or Circular Shifting

Initial Array: 1 3 7 4 8 6 5 2 9

Shift by: 4

Final Array: 6 5 2 9 1 3 7 4 8

Notice, in rotation, each elements position increased by shift amount. Also, elements in the end came to the front.

My Try

I used an approach in which I had to create a new array of size = shift amount. This array stored the shift no. of elements in the end of array. Now I shifted all the elements in the beginning of array by shift amount. In the end, I copied the elements of shift sized array to beginning of original array.

I tried to use minimum memory possible so only used shift sized array. I and keep it a bit fast (shift%=n statement to reduce inefficiency if shift >= n).

// Rotation

#include <iostream>

int main() {

    int n;
    std::cout << "Array size: ";
    std::cin >> n;
    int* list = new int[n];

    std::cout << "Enter elements:\n";
    for( int i = 0; i < n; ++i) {
        std::cin >> list[i];
    }

    int shift;
    std::cout << "Shift amount: ";
    std::cin >> shift;

    shift %= n; // to reduce work if shift > n
    int* temp = new int[shift];

    for( int i = n-shift; i < n ; ++i) {
        temp[i-n+shift] = list[i];
    }

    for( int i = n-1; i >= shift; --i) {
        list[i] = list[i-shift];
    }

    for( int i = 0; i < shift; ++i) {
        list[i] = temp[i];
    }

    delete [] temp;

    // Printing
    for( int i = 0; i < n; ++i) {
        std::cout << list[i] << "  ";
    }

    delete [] list;
}

Question

My question is that all the optimization that i was able to make were a bit obvious and natural. I wanted to know if there was a significant algorithmic, or memory optimization possible for this problem.

PS

I know there's a function std::rotate available from algorithm, but we had to solve this problem on our own.

I see some things that may help you improve your program.

Separate I/O from calculations

The program has three basic phases.  First, it gets input, then it manipulates that input, and then it produces output.  I would recommend putting the rotation code into a separate function.

Consider signed vs. unsigned

Does it make sense to have a negative array size?  Does it make sense to have a negative shift value?  I'd answer "no" to the first and "yes" to the second question.  For that reason, I'd recommend making n a size_t or unsigned type and also adding code to handle negative rotations.

Rethink your algorithm

The code currently creates a temporary array, but all that's really needed is a single temporary int.  Here's one way to do it:

unsigned gcd(unsigned a, unsigned b) {
    return b == 0 ? a : gcd(b, a % b);
}
void rotate(unsigned n, int shift, int *list)
{
    if (shift < 0) {
        shift = n -(-shift % n);
    } else {
        shift %= n;
    }
    if (shift == 0) {
        return;
    }
    for (unsigned cycles = gcd(n, shift); cycles; --cycles) {
        unsigned i = 0;
        unsigned nexti = n-shift;
        for (int temp=list[nexti+cycles-1]; nexti; i = nexti) {
            nexti = i+shift;
            if (nexti >= n) {
                nexti -= n;
            }
            std::swap(temp, list[i+cycles-1]);
        }
    }
}

Worked example

This is probably easier to understand with an example.  Let's start with a simple array or length 7 = { 0, 1, 2, 3, 4, 5, 6} and a requested shift amount of 4.  When the for loop starts we have this:


This piece of ABAP code should run through multiple tables and run a COUNT in all of them. The code runs through multiple tables to perform a count for a given ID. the ID is given by the user and then the program would look through all of the tables to check how many records each table has, that also has that ID. This is then returned to the user in an ALV with the table ID, the table Name and the number of records.

The program is run an pretty large sized tables and takes about 4 minutes to finish. Since I am fairly new with ABAP and OpenSQL I was wondering how this query could be improved to boost performance. I tried first checking if the ID exists in the table with SELECT SINGLE, and if it exists launch the count but that only made it slower with about a minute.

LOOP AT lt_where_used_data_of_coll ASSIGNING <fs_curr_tab>.

  SELECT ddtext AS table_description
  FROM dd02t
  INTO <fs_curr_tab>-table_description
  WHERE tabname = <fs_curr_tab>-table_id AND ddlanguage = 'EN'.
  ENDSELECT.

  lv_curr_table_name = <fs_curr_tab>-table_id.

        SELECT COUNT(*) AS nr_of_records
        FROM (lv_curr_table_name)
        INTO <fs_curr_tab>-nr_of_records
        WHERE id = p_c_id.
ENDLOOP.

The internal table in the program that is being filled already has the ID's of the tables to search through. The first SELECT inside the loop gets the name of the table corresponding with the ID. The second SELECT statement goes through all those tables and counts the number of records that contain the ID given by the user.

How can this statement be improved to run faster?

Your first query will only keep 1 value for <fs_curr_tab>-table_description so you might as well go for

  SELECT SINGLE ddtext AS table_description
  FROM dd02t
  INTO <fs_curr_tab>-table_description
  WHERE tabname = <fs_curr_tab>-table_id AND ddlanguage = 'EN'.

The AS table_description is not needed since you are not using CORRESPONDING, I also do not like the hardcoded language, I would take the user language. I also prefer EQ over =

  SELECT SINGLE ddtext
  FROM dd02t
  INTO <fs_curr_tab>-table_description
  WHERE tabname EQ <fs_curr_tab>-table_id
    AND ddlanguage EQ sy-langu.

If this is called with a large amount of tables (say over 50), then you should definitely consider reading all the table names at once with FOR ALL ENTRIES IN before the loop.

Finally, this could go horribly wrong if that table does not have an 'ID' column. If it were me I would check first the DD03L table to make sure the table does have an ID column. If I am not mistaken, DBIF_RSQL_INVALID_REQUEST is a non-catchable exception and would send the program straight into an abend.


Yet another hash tuple in C++17 Is there something terribly wrong with this implementation?

template <class T>
constexpr inline std::size_t hash_combine(T const& v,
  std::size_t const seed = {}) noexcept
{
  return seed ^ (std::hash<T>()(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2));
}

template <typename T> struct hash;

template <typename ...T>
struct hash<std::tuple<T...>>
{
  template <typename A1, typename ...A, std::size_t ...I>
  static auto apply_tuple(std::tuple<A1, A...> const& t,
    std::index_sequence<I...>) noexcept
  {
    if constexpr(sizeof...(A))
    {
      return hash_combine(std::get<0>(t),
        hash<std::tuple<A const&...>>()({std::get<I + 1>(t)...})
      );
    }
    else
    {
      return std::hash<std::remove_cv_t<std::remove_reference_t<A1>>>()(
        std::get<0>(t));
    }
  }

  auto operator()(std::tuple<T...> const& t) const noexcept
  {
    return apply_tuple(t,
      std::make_index_sequence<sizeof...(T) - 1>()
    );
  }
};

EDIT: This is better, I think:

template <typename ...T>
struct hash<std::tuple<T...>>
{
  template <std::size_t ...I>
  static auto apply_tuple(std::tuple<T...> const& t,
    std::index_sequence<I...>)
  {
    std::size_t seed{};

    return ((seed = hash_combine(std::get<I>(t), seed)), ...);
  }

  auto operator()(std::tuple<T...> const& t) const
  {
    return apply_tuple(t, std::make_index_sequence<sizeof...(T)>());
  }
};

Your code doesn't handle empty tuples gracefully.  The first version generates a compilation error, and the second version returns void.  For the first version, you can add an apply_tuple overload for empty tuples.  For the second version, you can change

return ((seed = hash_combine(std::get<I>(t), seed)), ...);

to

((seed = hash_combine(std::get<I>(t), seed)), ...);
return seed;

Is 0 really a good initial seed?  Some prime number, I guess, might be better (e.g., 672807365, which is what MSVC returns for std::hash<std::nullptr_t>{}(nullptr)).

The tuple unpacking can be simplified with std::apply, avoiding index_sequences:

template <typename... Args>
struct hash<std::tuple<Args...>> {
    std::size_t operator()(const std::tuple<Args...>& tuple) const noexcept
    {
        return std::apply([](const auto&... args) {
            auto seed = static_cast<std::size_t>(672807365);
            ((seed = hash_combine(args, seed)), ...);
            return seed;
        }, tuple);
    }
};

(live demo)


Early return, unless return value is nil I have a lot of code that looks like this:

value = TopicLinkClick.create_from(new_params)
return value unless value.nil?

# do something else

I find this code is not so good because creating a temporary variable is troublesome and disruptive of my workflow, and it takes time to read and understand a large block of code that is meaningless.

Is there a better way for me to do this?

Naming temporary variable is troublesome because [...] sometimes it costs time to read and understand a large block of code meanwhile it doesn't give any meaning.

Well, then give it some meaning. The name value doesn't describe much of anything. Instead, you could do something like

def some_method
  new_click = TopicLinkClick.create_from(new_params)
  return new_click unless new_click.nil?
  # do something else
end

You can shorten it a bit, but I wouldn't recommend it, as it's not quite as clear in my opinion

def some_method
  return new_click if new_click = TopicLinkClick.create_from(new_params)
  # do something else
end

Your other options are of course to not return early, but use a good old conditional instead

def some_method
  new_click = TopicLinkClick.create_from(new_params)
  unless new_click
    # do something else
  end
end

Alternatively, if you've got your code factored really nicely into methods, you can do

def some_method
  TopicLinkClick.create_from(new_params) || some_other_method(...)
end

Or, if you need a bit more logic, you can use a begin..end block, but again it's not as nice in my opinion.

def some_method
  TopicLinkClick.create_from(new_params) || begin
    # do something else
  end
end

Overall, though, your current approach is fine. Just use more descriptive names.

If you have a method that's simply a shortcut for calling either method A or method B, then I'd recommend using the method_a || method_b trick. Otherwise, like @schism, I'd recommend against doing too much on one line (as in code blocks 2 and 5)


Infix to postfix Converter implementation Please suggest some ways to improve my code for calculating a postfix expression from an infix expression entered by the user. The size of the input (Infix string) should not exceed 10 charaters since the maximum size of the input string mentioned in the code is 10.

#include<stdio.h>
#include<conio.h>
#include<ctype.h>

char Infix[10];
char Symbol[10];
char Postfix[10];

void Push();
void Pop();
int Preced(char, char);

void main()
{
   clrscr();
   printf("Enter a Infix Expression:");
   scanf("%s",&Infix);
   Push();
   printf("\nPostfix of Entered Expression: %s",Postfix);
   getch();
}

void Push()
{
   int i=0, j=0, k=0, s=0;
   while(Infix[i]!='\0')
   {
      if(Infix[i]=='('||Infix[i]==')'||Infix[i]=='*'||Infix[i]=='+'||Infix[i]=='-'||Infix[i]=='/'||Infix[i]=='%'||Infix[i]=='^')
      {
     Symbol[j++]=Infix[i];
      }
      else if(isalpha(Infix[i]))
      {
     Postfix[k++]=Infix[i];

      }
      if(Preced(Symbol[j-2],Symbol[j-1])==1)
      {
     Postfix[k++]=Symbol[j-2];
     Symbol[j-2]=Symbol[j-1];
     Symbol[j-1]=NULL;
     j=j-1;
      }
      if(Symbol[j-1]==')')
      {
    s=j-1;
    while(Symbol[--s]!='(')
    {
      printf("%c ",Symbol[s]);
      Postfix[k++]=Symbol[s];
      Symbol[s]=Symbol[s+1];
      Symbol[s+1]=NULL;
    }
      }
      i++;
   }
   while(--j>=0)
   {
     if(Symbol[j]=='^'||Symbol[j]=='*'||Symbol[j]=='/'||Symbol[j]=='%'||Symbol[j]=='+'||Symbol[j]=='-')
       Postfix[k++]=Symbol[j];
   }
}

int Preced(char c1, char c2)
{
  if(c1=='^'&&c2=='^')                                           //equal priorities.
  {
    return 1;
  }
  else if((c1=='+'||c1=='-')&&(c2=='+'||c2=='-'))                      //equal
  {
    return 1;
  }
  else if((c1=='*'||c1=='/'||c1=='%')&&(c2=='*'||c2=='/'||c2=='%'))     //equal
  {
    return 1;
  }
  else if(c1=='^'&&(c2=='*'||c2=='/'||c2=='+'||c2=='-'||c2=='%'))
  {
    return 1;
  }
  else if((c1=='*'||c1=='/'||c1=='%')&&(c2=='+'||c2=='-'))
  {
    return 1;
  }
  else
  {
    return -1;
  }
}

The intent of your preced function is completely unclear. I can't make heads or tails of what it's supposed to be doing as a whole. It appears to return 1 for two operators of equal precedence, but it also returns 1 for some cases when they're not equal--but it's not at all clear what those cases are supposed to be.

As a general rule, my immediate reaction would be to encode operator precedence into a table, and then have a little bit of logic to deal directly with precedence it found for them. So, I'd probably start with some code just to get the precedence:

int get_precedence(char ch) {

    char const *operators[] = {
        "^",
        "+-",
        "*/%"
    };
    int i;

    for (i=0; i<3; i++)
        if (strchr(operators[i], ch) != NULL)
            return i;
    return -1;
}

Using this, the "return 1 if they're equal" part could look something like this:

int preced(char ch1, char ch2) {
    if (get_precedence(ch1) == get_precedence(ch2))
        return 1;
}

I'm not quite sure about the rest of the logic, so I won't try to deal with that right now, but I'd almost be surprised if it didn't come out rather clearer after treatment like this as well.

That brings up an obvious second point: preced isn't a particularly great name for the function. It hints at the fact that it's somehow related to precedence, but that's about all we get from the name. Something more descriptive of the real intent would clearly help a lot here.

Use of NULL

NULL is intended to be a null pointer constant--not anything else. It should never be assigned to anything other than a pointer. Right now, you have places where you use it as a char instead, such as:

 Symbol[j-1]=NULL;

Although it's possible for a compiler to define NULL in a way that allows this, it's also possible to define NULL so you'd have to add a cast for this to work, so this affects both style/readability and portability.

Avoid arbitrary limits

If at all reasonable, and I think it probably is, it's generally best to avoid having entirely arbitrary limits on things like the size of the input. If you are going to have an arbitrary limit, at least try to make it large enough that it's unlikely to ever arise in practice (e.g., set it to 8 kilobytes instead of 10 bytes).

Enforce the limits you do have

Any time you see scanf("%s", somestring); you should cringe in fear, bordering on horror. You've just used scanf to re-create gets, a function so terrible it's been removed from recent versions of the standard.

The problem here is that although your string is (in this case) only 10 bytes long, you haven't informed scanf of that fact, so if the user types in more than 9 characters (remember, the string needs a NUL terminator) you'll get undefined behavior. Any time you use the %s conversion with scanf (or cousins like fscanf) you need to include a maximum size allowed:

char buffer[1025];

scanf("%1024s", buffer);

So here we've specified a maximum length of 1024, with a buffer length of 1025, so we have room for the NUL terminator. Result: the code is nice and safe. No matter how much data the user enters, scanf will never write past the end of our buffer.


Painting Fences - optimize for faster performance I am attempting this problem at Codeforces. I'm stuck as it gives a time limit exceeded error for large inputs though the logic I followed is similar to that of the editorial.

I'd be grateful for any pointers that can be useful in improving my logic and/or code performance.

import sys

count = 0

def count_and_split(p):
    h = min(p)
    n = len(p)
    global count
    if h<n:
        count += h
    else:
        count += n
    p = [i-h for i in p]
    e = []
    for i in p:
        if i == 0:
            k = p[:p.index(i)]
            if k:
                e.append(k)
            p = p[(p.index(i)+1):]
    if p:
        e.append(p)
    return e

def main():
    n = int(sys.stdin.readline())
    a = [map(int, sys.stdin.readline().split())]

    for part in a:
        e = count_and_split(part)
        if count>n:
            print n
            exit()
        for new_part in e:
            a.append(new_part)
    print count

if __name__ == '__main__':
    main()

Your logic seems badly organised. Let's try to improve this first :

Making things better organised

At the moment, when you get the input, you use them to define a and n such that, for example, a = [[2, 2, 1, 2, 1]] and n = 5. For the sake of clarity, let's try to isolate the different parts to make things easier to build. We could define a function getting the list [2, 2, 1, 2, 1] from the user and then write a function to handle this list.

Just doing this and moving your code around, here's what we get :

import sys

count = 0

def count_and_split(p):
    h = min(p)
    n = len(p)
    global count
    if h<n:
        count += h
    else:
        count += n
    p = [i-h for i in p]
    e = []
    for i in p:
        if i == 0:
            k = p[:p.index(i)]
            if k:
                e.append(k)
            p = p[(p.index(i)+1):]
    if p:
        e.append(p)
    return e

def get_nb_strokes(l):
    a = [l]
    n = len(l)
    global count
    count = 0
    for part in a:
        e = count_and_split(part)
        if count>n:
            return n
        for new_part in e:
            a.append(new_part)
    return count

def get_list_from_user():
    n = int(sys.stdin.readline())
    return map(int, sys.stdin.readline().split())

def main():
    print(get_nb_strokes(get_list_from_user()))

if __name__ == '__main__':
    main()

It already looks much better doesn't it ? Well, it's not quite over yet.
Create tests :

Here's what your main function could look like as your change the code. You can run this quickly to ensure things seem to be working fine :

def main():
    assert get_nb_strokes([2, 2, 1, 2, 1]) == 3
    assert get_nb_strokes([2, 2]) == 2
    assert get_nb_strokes([1]) == 1

Add tests :

You can easily define new tests, it can also be a good idea to check how your code behaves on bigger inputs :

for i in range(2, 10):
    # fence of constant height
    for h in range(1,3):
        fence = [h] * i
        assert len(fence) == i
        assert get_nb_strokes(fence) == min(h,i)
    # fence of constant width
    for w in range(1,3):
        fence = [i] * w
        assert get_nb_strokes(fence) == min(w,i)
    # fence going up
    fence = range(1, i+1)
    assert get_nb_strokes(fence) == len(fence) == i
    # fence going down
    fence = range(i, 0, -1)
    assert get_nb_strokes(fence) == len(fence) == i
    # fence going up then down
    fence = range(1, i) + range(i, 0, -1)
    assert len(fence) == 2*i -1
    assert get_nb_strokes(fence) == i
    # fence going down then up
    fence = range(i, 0, -1) + range(2, i+1)
    assert get_nb_strokes(fence) == len(fence) == 2*i-1
    # L-shaped fence
    fence = [i] + [1]*i
    assert get_nb_strokes(fence) == 2
    # U-shaped fence
    fence = [i] + [1]*i + [i]
    assert get_nb_strokes(fence) == 3
    # W-shaped fence
    fence = [i] + [1]*i + [i] + [1]*i + [i]
    assert get_nb_strokes(fence) == 4

Don't use global variables

Having global variables makes things impossible to track and pretty hard to test. Let's try to improve this. It seems like the logic in count_and_split related to count could be done easily from the get_nb_strokes functions.

The updated code looks like :

def count_and_split(p):
    h = min(p)
    p = [i-h for i in p]
    e = []
    for i in p:
        if i == 0:
            k = p[:p.index(i)]
            if k:
                e.append(k)
            p = p[(p.index(i)+1):]
    if p:
        e.append(p)
    return e

def get_nb_strokes(l):
    a = [l]
    n = len(l)
    count = 0
    for part in a:
        count += min(min(part),len(part))
        e = count_and_split(part)
        if count>n:
            return n
        for new_part in e:
            a.append(new_part)
    return count

Please note that at the moment, I still don't know how your code works, I've just changed the way it is organised.
A bit of cleanup

Now, we can move e = count_and_split(part) after the check about count as things are independent. We can also get rid of the temporary variable, use extend and rename the function to write :

def get_nb_strokes(l):
    a = [l]
    n = len(l)
    count = 0
    for part in a:
        count += min(min(part),len(part))
        if count>n:
            return n
        a.extend(split(part))
    return count

I'll try to have another loop ASAP.


Always keep https in url I wrote this recently, I thought I'd check if it was the best way to keep my url as an https at all times, any advice would be appreciated

   if($_SERVER["HTTPS"] != "on")
{
    header("Location: https://" . $_SERVER["HTTP_HOST"] . $_SERVER["REQUEST_URI"]);
    exit();
}

If the request is already being served over HTTPS, then it would be a good idea to add a Strict-Transport-Security header to the response. Strict-Transport-Security tells the browser that, as a matter of policy, your site wants all requests to your domain to be made over HTTPS.

Be aware that redirecting an HTTP POST request would cause the browser to make an HTTP GET request. To cause the second request to also be made as a POST, you would need to respond with a a 307 status code.  Of course, the benefit of a 307 redirect of a POST is questionable, as any sensitive POST data will have already been transmitted unencrypted already.


JavaScript "recursion" via setTimeout I understand that because of JavaScript's single-threaded execution, long loops or recursive calls could make the browser unresponsive while they execute.

I thought about simulating recursion using setTimeout with a delay of 0, so as to queue the next call to execute as soon as possible.

Here's an example comparing a standard recursive factorial function with an attempt to do the same thing but using setTimeout:

function rFactorial(num)
{
    if (num === 0) return 1;
    else return num * rFactorial( num - 1 );
}

function tFactorial(num, callback, acc) {
    acc = acc || 1;
    if (num === 0) {
        callback(acc);
    }
    else {
        setTimeout(function() {
            tFactorial(num-1, callback, acc*num);
        }, 0);
    }
}

I tested this using the following test:

for (var x = 1; x < 20; x += 1) {
    tFactorial(x, (function(n) {
        return function(result) {
            console.log(rFactorial(n) === result)
        };
    })(x));
}

This worked for that range of numbers, although for higher numbers the two functions seem to give different results. for example, for 100, sFactorial gives 9.332621544394418e+157, while rFactorial gives 9.33262154439441e+157.

I have two questions:

Is this a good approach? Can anyone suggest any improvements etc?
Can anyone shed any light on why the two functions give the same results for lower numbers but different results for higher numbers?

A few observations :

Numbers in javascript are IEE754 double precision float. All of them. There is no integer type. Which means that only 53 bits are available to describe the integer part. So it doesn't make sense to try to do integer operations on numbers bigger than that. Your numbers are wayyy too big and the results will be undefined. If you want to compute bigger numbers, you'll need to define your own format, you can't use the native numbers of javascript.

Using setTimeout or using a recursion might be amusing solutions but they're not terribly efficient (at least until we have tail call optimization in javascript). You'd better use a simple boring loop.


Convert String to Integer (calculator app) I'm currently learning Swift and today I created my first application, a simple calculator. My main goal was to practice and create something without any help. After a few hours my application was ready for use but of course not perfect. After overlooking my code I noticed that there are areas which I should improve. For example, I think I could simplify this area:

zahlXS = DisplayX.text!
zahlYS = DisplayY.text!
let zahlX = Int(zahlXS)
let zahlY = Int(zahlYS)

My goal is to take the String variable zahlXS and convert it to an  integer variable, so I'm able to calculate with my variables. Please note that I declared zahlXS and zahlYS at the very beginning of my code.

I'm also trying to use the "MVC" concept. In this application, I put my functions add and sub in the ViewController because I first created a struct in a separate file but I just couldn't access the functions in my ViewController. I tried to create a whole class instead of a struct afterwards, but this didn't solve the problem either.

import UIKit
import Foundation

var zahlXS: String = String()
var zahlYS: String = String()

func add(x: Int, y: Int) -> Int {
return x + y
}

func sub(x: Int, y: Int) -> Int {
return x - y
}

class ViewController: UIViewController {

@IBOutlet weak var DisplayX: UITextField!

@IBOutlet weak var DisplayY: UITextField!

@IBOutlet weak var Resultat: UILabel!

@IBAction func performAddition(_ sender: UIButton) {
    zahlXS = DisplayX.text!
    zahlYS = DisplayY.text!
    let zahlX = Int(zahlXS)
    let zahlY = Int(zahlYS)

    let resultAdd = add(x: zahlX!, y: zahlY!)
    Resultat.text = "The result is: \(resultAdd)"

}

@IBAction func performSubtraktion(_ sender: UIButton) {
    zahlXS = DisplayX.text!
    zahlYS = DisplayY.text!
    let zahlX = Int(zahlXS)
    let zahlY = Int(zahlYS)

    let resultSub = sub(x: zahlX!, y: zahlY!)
    Resultat.text = "The result is: \(resultSub)"

}

}

You had the right idea to use to encapsulate the variables and operations in a class object or struct, rather than using the global scope. It was not clear why this did not work for you.

To make things easy you can declare the object in the same file as the view controller.

E.g.

// Declare a struct to encapsulate the x and y variables.
// Perform the add and subtract operations using the instance variables.
// This code can be moved into Arithmetic.swift
struct Arithmetic {
   let x: Int
   let y: Int

   // Create a convenience initialiser for code which can be reused.
   init(x: String, y: String) {
       self.x = Int(x) ?? 0
       self.y = Int(y) ?? 0
   }

   // Use encapsulation to perform the operations.

   func add() -> Int {
       return x + y
   }

   func subtract() -> Int {
       return x - y
   }
}

class ViewController: UIViewController {

    // You correctly defined the IBOutlets as weak to prevent retain cycles.
    // Property names should begin with lower-case.
    @IBOutlet weak var displayX: UITextField!
    @IBOutlet weak var displayY: UITextField!
    @IBOutlet weak var resultat: UILabel!

    @IBAction func performAddition(_ sender: UIButton) {

        // Use a guard condition to unwrap the variables.
        // Doing this avoids crashes when you force unwrap a nil.
        guard let x = displayX.text, let y = displayY.text else {
            return
        }

        let a = Arithmetic(x: x, y: y)
        let r = a.add()
        resultat.text = "The result of addition is: \(r)"
    }

    @IBAction func performSubtraktion(_ sender: UIButton) {
        guard let x = displayX.text, let y = displayY.text else {
            return
        }

        let a = Arithmetic(x: x, y: y)
        let r = a.subtract()
        resultat.text = "The result of subtraction is: \(r)"
    }
}


Which is better practice for this if elif… else statement? If below piece of code has to be restructured to use but one single if-elif block, which is better? 1. or 2.?

A = int(input("Enter an integer(1~30): "))

if A % 6 == 0:
    print("A is even")
    print("A is even and a multiple of 6 (6, 12, 18, 24, 30).")
elif A % 10 == 0:
    print("A is even")
    print("A is even and a multiple of 10 (10, 20).")
elif A % 2 == 0:
    print("A is even")
    print("A is even and a number out of 2, 4, 8, 14, 16, 22, 26, 28.")
elif A % 3 == 0:
    print("A is odd")
    print("A is odd and a multiple of 3 (3, 9, 15, 21, 27).")
else:
    print("A is odd")
    print("A is 1 or a number out of 1, 5, 7, 11, 13, 17, 19, 23, 25, 29.")

if A % 2 == 0 and A % 3 == 0:
    print("A is even")
    print("A is even and a multiple of 6 (6, 12, 18, 24, 30).")
elif A % 2 == 0 and A % 5 == 0:
    print("A is even")
    print("A is even and a multiple of 10 (10, 20).")
elif A % 2 == 0 and A % 3 != 0 and A % 5 != 0:
    print("A is even")
    print("A is even and a number out of 2, 4, 8, 14, 16, 22, 26, 28.")
elif A % 2 != 0 and A % 3 == 0:
    print("A is odd")
    print("A is odd and a multiple of 3 (3, 9, 15, 21, 27).")
else:
    print("A is odd")
    print("A is 1 or a number out of 1, 5, 7, 11, 13, 17, 19, 23, 25, 29.")

This is part of a question in a school assignment and the problem is to reformat some code into the structure of
if
    …
elif
    …
elif
    …
else
    …

There are obviously better structures to avoid multiplicating print("A is even") but the question is to format it in the structure above.
So if the two above are the only options which is better and why?

The following is better IMO. Duplicated "A is odd" / "A is even" strings are omitted from the output. They could be added if required.
if A % 2 == 0:
    if A % 6 == 0:
        msg = "a multiple of 6 (6, 12, 18, 24, 30)"
    elif A % 10 == 0:
        msg = "a multiple of 10 (10, 20)"
    else:
        msg = "a number out of 2, 4, 8, 14, 16, 22, 26, 28"
    print(f"A is even and {msg}.")
else:
    if A % 3 == 0:
        msg = "a multiple of 3 (3, 9, 15, 21, 27)"
    else:
        msg = "a number out of 1, 5, 7, 11, 13, 17, 19, 23, 25, 29"
    print(f"A is odd and {msg}.")

As for which one of the two versions of the provided code is better, I agree with @stefan's answer.


Create HTML listing of source code This is a Python script I made to create a HTML page listing the source files in the current directory.  The script creates a syntax highlighted preview of the files and lists some basic attributes.

I would like to know if this is the best method to generate the HTML code? Is there a web development library that would be more efficient for this task?

What are some ways that I could improve my webpage design?

# -*- coding: utf-8 -*-
import pygments
from pygments import highlight
from pygments.lexers import get_lexer_by_name
from pygments.formatters import HtmlFormatter
from pygments.lexers import guess_lexer, get_lexer_for_filename
from os import listdir, mkdir
from os.path import isfile, join, getsize, getctime, isdir, dirname, abspath
from time import ctime
import glob
from math import ceil

# configuration
mypath = "."
maxlines = 30
items_per_page = 10.0
base_path = dirname(abspath(__file__))+"/"
files_to_ignore = ["pgp", "der", "crt", "asc", "key"]

if isdir("pastebin/") == False:
    mkdir("pastebin/")

onlyfiles = [ f for f in glob.glob(mypath+"/*.*") if isfile(join(mypath,f)) ]
template = """
<!DOCTYPE html>

"""
valid_files = []
for name in onlyfiles:
    if name.split(".")[1:] in files_to_ignore:
        pass
    else:
        try:
            get_lexer_for_filename(name)
            valid_files.append(name)
        except pygments.util.ClassNotFound:
            pass

page_links = ""
for n in range(1, int(ceil(len(valid_files)/items_per_page))):
    page_links+=""" %d """%(n, n)
count=0
page_count=1
output=template
for name in valid_files:
    count+=1.0

    name=name.replace("./", "")
    code = "".join(open(name, "rb").readlines()[:maxlines])
    lexer = get_lexer_for_filename(name)
    formatter = HtmlFormatter(linenos=False, cssclass="source", encoding="utf-8")
    result = highlight(code, lexer, formatter)
    temp="""
"""
    temp+=""
    temp+="""
file-name

"""
    temp+=""" Created on: date-created """
    temp+="""Size: file-size """
    temp+="""

result
"""

    temp=temp.replace("ccs-color-codes", HtmlFormatter().get_style_defs('.source'))
    temp=temp.replace("file-name", name)
    temp=temp.replace("file-path", base_path+name)
    temp=temp.replace("file-size", str(getsize(name)))
    temp=temp.replace("date-created", ctime(getctime(name)))
    temp=temp.replace("result", result)
    output+=temp+"\n"

    if int(count) == int(items_per_page):
        count=0
        output+="""%s"""%(page_links)
        output+=""
        open("pastebin/page%d.html"%(page_count), "w").write(output)
        page_count+=1

There are a ton of good template rendering libraries out there, but I'd leave it to you to figure out if they are better suited to your needs than hand rolled code.

Assuming you still want to write this yourself, I'd look for ways to make the code more completely data driven and also to make it more readable.

On the data driven side, you might want save your design as an html file.  That way you can use whatever styling or graphic layout tools you want to produce the layout. You merely need to leave template placeholders where you want the data to go and apply the substitutions.  Django's templating system offers a great example of what you'd expect: vanilla html data with placeholders where the data's gonna go.

Going data driven will reduce a ton of the code here (since so much of it is just string assembly).  You can do some nice syntactic tricks to clean up things like writing html tags, for example

 tag = lambda t, c : "<%s>%s></%s>" % (t,c,t)

 tag('footer','footer goes here')
 >  footer goes here

And you you could also use a context manager for tag nesting (example here) to keep things neat

Lastly, use string.Template to allow for keyword or dictionary based replacement you can do things like

header = Template("$text</header")
header.subsitute(font='sans-serif',text='hello world')

that would make it easier to clean up the series of temp = temp.replace... lines.  Python is a great language for this sort of thing - it should be easy to cut this down by half or more with built in tools.


Map, which contains biomes, which contain landforms, which contain tiles I am trying to model this:

a Map can have multiple child of type Biomes and no parent
a Biome can have multiple child of type Landforms and a Map as its parent
a Landform can have multiple child of type Tiles and a Biome as its parent
a Tile has no child and a Landform as its parent

I want it to be generic so I can easily add new links to the chain (like adding a new kind of section between Biome and Landform for example). Here is the less ugly solution I have for now :

public class RootSection<T, TChild> : Section<T>
    where T : Section<T>
    where TChild : Section<TChild>
{
    public List<TChild> ChildSection { get; } // duplicate
}

public class MiddleSection<T, TChild, TParent> : Section<T>
    where T : Section<T>
    where TChild : Section<TChild>
    where TParent : Section<TParent>
{
    public List<TChild> ChildSection { get; } // duplicate
    public TParent Parent { get; } // duplicate
}

public class BottomSection<T, TParent> : Section<T>
    where T : Section<T>
    where TParent : Section<TParent>
{
    public TParent Parent { get; } // duplicate
}

public class Section<T>
    where T : Section<T>
{
    List<T> AdjacentSections { get; }
}

public class Map : RootSection<Map, Biome> { } // (T, TChild)
public class Biome : MiddleSection<Biome, Landform, Map> { } // (T, TChild, TParent)
public class Landform : MiddleSection<Landform, Tile, Biome> { } // (T, TChild, TParent)
public class Tile : BottomSection<Tile, Landform> { } // (T, TParent)

As you can see, there is already duplicate code and I can't think of a solution to get rid of this issue. I feel like I am either missing something obvious or over-complexifying the problem. I also feel like this is close to a classic data structure which I ignore the name preventing me from searching for inspiration on the net.

How can I rewrite this code to look cleaner ? Am I right to think it's close to a well known data structure ?

I want it to be generic so I can easily add new links to the chain
  (like adding a new kind of section between Biome and Landform for
  example)

Generics work fine for simple generic data structures (like a List). Your "generic" data structure is actually a very special one which will not be used outside of your model. It is more a try to extract the common parts of your model to a generic structure which may be useful, but based on your question I can not see the value in your case.

In my experience, data stuctures with multiple generic types, which has contraints to other generic types, are hard to understand and make the code more complicated.

In your case, I would just give generics up and write the data structure down as it is:

public class Map
{
    public List<Biome> Biomes { get; } = new List<Biome>();
    public List<Map> AdjacentMaps { get; } = new List<Map>();
}

public class Biome
{
    public Map Map {get; }
    public List<Landform> Landforms { get; } = new List<Landform>();
    public List<Biome> AdjacentBiomes { get; } = new List<Biome>();
}

public class Landform
{
    public Biome Biome {get; }
    public List<Tile> Tiles { get; } = new List<Tile>();
    public List<Landform> AdjacentLandforms { get; } = new List<Landform>();
}

public class Tile
{
    public Landform Landform {get; }
    public List<Tile> AdjacentTiles { get; } = new List<Tile>();
}

Much more readable! The properties has more descriptive names and it needs a minute to extend this hierachical data structure with other types.


Writing if..else construct without long condition checks to increase maintainability Should writing if..else construct in the following manner be flagged for correction in peer code review?

Existing Code

bool userInstanceHasTransactions = userInstance != null && userInstance.Transactions > 0;
if(userInstanceHasTransactions) {
   //Processing logic here
} else {
  //Some other processing here
}

Flagged to change as

if(userInstance != null && userInstance.Transactions > 0) {
   //Processing logic here
} else {
  //Some other processing here
}

I have observed code of this kind in C#, but I believe language really does not mattter for this question.

I noticed there are lot of if..else construct written in this manner. Will too many if..else construct in this manner impact size of the binary file (dll or exe)?

Is it correct to say writing if..else construct in the manner depicted above increases readability?

Replace Temp variables with queries

It's a primary refactoring technique championed by Fowler in his book Refactoring. That as well as the second technique of extracting methods from complex/long code dramatically improves code readability. Please see below for examples:

public void Testc()
{
    // The code that you've suggested as the first option
    // makes the if statement more readable and it should be preserved
    // over the complicated code within the if statement IMO: i.e. if(userInstance != null && userInstance.Transactions > 0)
    // but it introduces the problem of temporary variables - which, when refactoring
    // should probably be replaced with a query, as Martin Fowler suggests,
    // unless you have cogent reasons to preserve them as a temporary variables
    // (i.e. for performance reasons). If that's the case, Fowler suggests
    // to still replace the temporary variable with a query, and further down the line
    // to THEN start thinking about optimisation. This might force you back to using temporary
    // variables.

    // Summary:
    // So Fowler suggests to replace the logic with a query. And Then to
    // start thinking about optimisation if required
    // so it would look something like this:

    if (UserInstanceHasTransactions())
    {
        //Processing logic here
    }
    else
    {
        //Some other processing here
    }
}

public bool UserInstanceHasTransactions()
{
    return userInstance != null && userInstance.Transactions > 0;
}

As Volundi points out sometimes the userInstance might itself be a temporary variable. if that's the case you might have to do something like this:

public bool UserInstanceHasTransactions(userInstance )
{
    return userInstance != null && userInstance.Transactions > 0;
}

And you could call it like this:

if (UserInstanceHasTransactions(userInstance))
{
    //Processing logic here
}
else
{
    //Some other processing here
}

Above all the important thing is to preserve readability, because your code will be read many more times than it is edited. If the userInstance is a temporary variable then you might want to consider refactoring all those instances as a query method and as Fowler suggests, if performance is not a problem.


My implementation of javascript class library I'm reading a book called JavaScript Web Applications. In first chapter, it introduces an implementation of class library. Here's an intermediate version:

var Class = function () {
    var klass = function () {
        this.init.apply(this, arguments);
    }
    klass.prototype.init = function (args) {};

    // Adding class properties
    klass.extend = function (obj) {
        var extended = obj.extended;
        for (var i in obj) {
            klass[i] = obj[i];
        }
        if (extended) extended(klass);
    };

    // Adding instance propertied
    klass.include = function (obj) {
        var included = obj.included;
        for (var i in obj) {
            klass.prototype[i] = obj[i];
        }
        if (included) include(klass)
    };

    return klass;
}

Then it can be used like this:

var Person = new Class;
Person.extend({
    find: function (id) { /* ... */ }
});
var person = Person.find(1);

var Person = new Class;
Person.include({
    save: function () { /* ... */ }
});
var person = new Person;
person.save();

I modified it to that it support non-shared instance properties, and here's my version:

var Class = function (properties) {
    var klass = function () {
        this.init.apply(this, arguments);
    }
    klass.prototype.init = function (args) {
        var self = this;
        if (properties instanceof Array && typeof args === 'object') {
            properties.forEach(function (p) {
                if (typeof args[p] !== 'undefined') {
                    self[p] = args[p];
                }
            });
        }
    };

    // extend and include

    return klass;
}

So it can be used like this:

var Person = new Class(['name', 'age'];
var person = new Person({ name: 'bob', age: '30' });

Is my approach ok?
I also wonder that why Person.name was "" in my approach? I expected it to be undefined.

There is nothing fundamentally wrong about your class strategy, it is a matter of personal style really. The only suggestion I would have is to add Object.prototype.hasOwnProperty guards in the for-in loops, to avoid extending your classes with bogus properties if someone adds things to Object.prototype.

for (var i in obj) {
     if(Object.prototype.hasOwnProperty.call(obj, i)){
         // ...
     }
}

As for the name === '' thing, its just a coincidence since Javascript functions (and therefore your class) always have this property defined:

(function(){}).name     // ""
(function foo(){}).name // "foo"

Some other properties such as "prototype" or "length" should also always be defined and there is nothing you can do about them.


Listener callback functions I wrote codes that listener and callback function like this:

struct MyListener
{
    virtual void callback1(int value) = 0;
    virtual void callback2(string value) = 0;
    ...
};

vector<weak_ptr<MyListener>> _listeners;

void MyClass::fireCallback1(int value)
{
    vector<weak_ptr<MyListener>>::iterator iter = _listeners.begin();
    while (iter != _listeners.end())
    {
        if ((*iter).expired())
        {
            iter = _listeners.erase(iter);
        }
        else
        {
            shared_ptr<MyListener> listener = (*iter).lock();
            listener->callback1(value);
            ++iter;
        }
    }
}

void MyClass::fireCallback2(string value)
{
    vector<weak_ptr<MyListener>>::iterator iter = _listeners.begin();
    while (iter != _listeners.end())
    {
        if ((*iter).expired())
        {
            iter = _listeners.erase(iter);
        }
        else
        {
            shared_ptr<MyListener> listener = (*iter).lock();
            listener->callback2(value);
            ++iter;
        }
    }
}

But as you see, when I add more functions on MyListener struct, I have to add fireCallbackN functions on MyClass, and it has too many duplicated codes.

Is there best way to make simplify these duplicate codes in C++?

Try this:

struct MyListener
{
    virtual void on_data(int value) = 0;    // same function name
    virtual void on_data(string value) = 0; // same function name
    ...
};

template<typename T>
void MyClass::trigger_data_event(T&& value) // pass by universal reference
{
    auto iter = _listeners.begin(); // consider using auto where appropriate

    while (iter != _listeners.end())
    {
        // don't use expired here: it involves concurrency issues if the pointer
        // is invalidated between calling expired and lock

        // instead, just lock() and check the result:

        if(auto p = iter->lock())
        {
            p->on_data(std::forward<T>(value)); // works with both callbacks
            ++iter;
        }
        else
            iter = _listeners.erase(iter);
    }
}

I renamed the APIs you used from callback1/2 to on_data (same name) and unified the implementations of fireCallback1 and fireCallback2.


PHP code for search page Want to show you part of code that I wrote to process search request. User commit search by articles (codes) of products. The goal is to allow user write his search request to textarea element in any possible way: in a row, in a column or separated by any separator (comma, space, semicolon etc.).

<?php
include("db.php"); // Including connection to MySQL

if(!empty($_POST['codes'])){ // Checking user's input from textarea not empty

    $codes = $_POST['codes']; // Moving user's input to variable
    preg_match_all("/\d{6}/", $codes, $codes_array); // Taking all elements that matches regular expression and adding them to the array

    $codes_array = array_unique($codes_array[0]); // Removing duplicates from the array

    if(!empty($codes_array)){ // Checking array not empty

        $codes_list = ""; // Creating new variable

        foreach ($codes_array as $key => $value) { // Processing our array
            $codes_list .= $value . "|"; // Adding values and separator to previously created variable to use it in MySQL query
        }

        $codes_list = rtrim($codes_list, "|"); // Removing last separator from string
        $codes_add = " AND `code` REGEXP '^({$codes_list})$'"; // Creating new variable with part of MySQL query and our elements from the array
    } else {
        $codes_add = ""; // Creating empty variable if our array was empty
    }
}

$query = "SELECT * FROM `table` WHERE `status` = '0'" . $codes_add; // Generating MySQL query string
$result = mysqli_query($link, $query); // Making a request to MySQL
?>

My friend said that it contains vulnerabilities for SQL-injection because I do not filter user's input. But my logic based on a statement that user's input filters at the line with preg_match_all. I'm sure that nothing but elements that consist of 6 digits will be included to the array.

Anyway I want that someone who have an experience with PHP and vulnerabilities took a look at this code and share his opinion about potential risks of using it. Thank You.

Being as big an advocate of prepared statements as I am, I cannot find any way for the SQL injection here, so your code should be safe.

I could only do the usual routine of making this code less bloated:

$query = "SELECT * FROM `table` WHERE `status` = '0' ";

if(!empty($_POST['codes'])) {
    preg_match_all("/\d{6}/", $_POST['codes'], $codes_array);
    $codes_array = array_unique($codes_array[0]);
    $codes_list = implode(",", $codes_array);
    $codes_add = $codes_list ? " AND `code` IN ($codes_list)" : "";
    $query .= $codes_add;
}
$result = mysqli_query($link, $query);

basically, PHP already has a function to glue array values together called implode() and, based on my understanding of the SQL, I decided to change REGEXP for IN. If I am wrong, then you could simply change it back to regexp.


Most efficient solution for USACO: Cow Gymnastics - Python I was trying to figure out this problem, and I did. However, my code is so abhorrently ugly that I want to tear out my eyeballs when I look at it:
with open("gymnastics.in", "r") as fin:
  rounds, cows = [int(i) for i in fin.readline().split()]
  nums = [tuple(map(int, line.split())) for line in fin]

def populatePairs(cows):
  pairs = []
  for i in range(cows):
      for j in range(cows):
          if i != j:
              pairs.append((i+1,j+1))
  return pairs

def consistentPairs(pairs, nums):
  results = []

  for i in pairs:
      asdf = True
      for num in nums:
          for j in num:
              if j == i[1]:
                  asdf = False
                  break
              if j == i[0]:
                  break
          if not asdf:
              break
      if asdf:
          results.append(i)

  return results

pairs = populatePairs(cows)

with open("gymnastics.out", "w+") as fout:
  print(len(consistentPairs(pairs, nums)), file=fout)

I feel like that there should definitely be a better solution that is faster than \$O(n^3)\$, and without the triple nested for-loop with the if-statements trailing behind them, but I cannot, for the love of god, think of a better solution.
Problem synopsis:
Given an \$n\$ by \$m\$ grid of points, find the number of pairs in which number is consistently placed before the other.
Example:
Input:
3 4
4 1 2 3
4 1 3 2
4 2 1 3

Output:
4
Explanation:
The consistent pairs of cows are (1,4), (2,4), (3,4), and (3,1), in which case 4 is consistently greater than all of them, and 1 is always greater than 3.

Suggestions:

Use the variable names from the problem specification instead of inventing your own (just make them lower-case since that's preferred in Python code). Makes it easier to see the connections. Unless yours are much better.
I guess nums is plural so num is a single number, but you can iterate it? Bad name. And wth does asdf mean?
Python prefers snake_case for function names.
To be honest, the lack of any explanation of your method, the name asdf and the highly convoluted code made me give up reading it. But here's my solution, simply counting occurring pairs and then the result is the number of pairs that appeared K times:

from itertools import combinations
from collections import Counter

ctr = Counter()
with open('gymnastics.in') as f:
    k, _ = map(int, next(f).split())
    for session in f:
        cows = session.split()
        ctr.update(combinations(cows, 2))

with open('gymnastics.out', 'w') as f:
    print(list(ctr.values()).count(k), file=f)


Find the nearest point of a given set of points (part 2) This is a continued discussion from here, and I use new a approach (bounding box/or envelop algorithm) to improve the algorithm efficiency. Any code bugs, algorithm time complexity improvement or general code advice is highly appreciated.

Major structure of my code:

assign_box is used to build the bounding box or envelop
find_min is finding min points among all candidate points for a given target point. It first uses a bounding box/envelop solution. If it cannot find an answer, it will use a brute force solution
bruce_force is a brute force solution which is used to check the correctness of find_min
distance is a utility method used to check the distance between two points

import sys
import random
from collections import defaultdict
def assign_box(box_map, points, k):
    x_min = min([p[0] for p in points])
    x_max = max([p[0] for p in points])
    y_min = min([p[1] for p in points])
    y_max = max([p[1] for p in points])
    x_distance = (x_max - x_min) / k
    y_distance = (y_max - y_min) / k
    for p in points:
        i = (p[0]-x_min)/x_distance
        j = (p[1]-y_min)/y_distance
        box_map[(i,j)].append(p)

    return (x_min, x_distance, y_min, y_distance)

def distance(p1, p2):
    return (p1[0]-p2[0]) ** 2 + (p1[1]-p2[1]) ** 2

def find_min(points, box_map, target_point, x_min, x_distance, y_min, y_distance):
    # try 9 neighbour first
    row = (target_point[0] - x_min) / x_distance
    col = (target_point[1] - y_min) / y_distance
    result = sys.maxint
    result_point = None
    for r in range(row-1, row+2):
        for c in range(col-1, col+2):
            if (r,c) in box_map:
                for p in box_map[(r,c)]:
                    d = distance(p, target_point)
                    if d < result:
                        result = d
                        result_point = p
    if result_point:
        return (result, result_point)
    else:
        # try brute force solution
        result = sys.maxint
        result_point = None
        for p in points:
            d = distance(p, target_point)
            if d < result:
                result = d
                result_point = p
        return (result, result_point)

def bruce_force(points, target_point):
    result = sys.maxint
    result_point = None
    for p in points:
        d = distance(p, target_point)
        if d < result:
            result = d
            result_point = p
    return (result, result_point)

if __name__ == "__main__":
    # key is a tuple, (box row index, box col index)
    # value is a list of points fall into it
    box_map = defaultdict(list)
    points = []
    for i in range(100):
        points.append((random.randint(0,10), random.randint(0,10)))
    x_min, x_distance, y_min, y_distance = assign_box(box_map, points, 5)
    target_point = (random.randint(0,10), random.randint(0,10))
    print 'target point, ', target_point
    print 'find min result, ', find_min(points, box_map, target_point, x_min, x_distance, y_min, y_distance)
    print 'brute force result, ',  bruce_force(points, target_point)

Mostly this looks good.

  assign_box is used to build the bounding box or envelope

I benchmarked the speed of assign_box. Please see https://github.com/jhanley634/testing-tools/blob/master/problem/bench/min_of_copied_list.py#L104..L109 . It turns out one can compute a pair of min's / max's slightly faster:

0.017s  scan_manually_with_if
0.019s  scan_with_two_copies
0.030s  assign_box
0.049s  scan_with_key
0.078s  scan_manually_with_min_max

The first two scan approaches are faster than your code.

  find_min

A comment of # try 8 neighbors first would be more natural than 9, since distance won't be less when row & column offsets are zero.

  bruce_force

Maybe this is a Bruce Lee joke. I recommend renaming it def brute_force.

  distance is a utility method ...

I recommend renaming it def distance_squared, as it clearly does not compute Euclidean distance. It's only used in comparisons, and N^2 is monotonic w.r.t. N, so there's no harm done. But you should say what you mean, and mean what you say.