ReactiveCocoa implementation of async function I would like a review of the RAC implementation for the function setGist()
I've posted the code on GitHub for easier reading.

Description:

You create an instance of GistService()
let gistService = GistService()
Then use public functions .resetGist() -> Bool and .setGist(string: String)  -> SignalProducer<(URL: String, gistID: String), ConnectionError>

gistService.setGist(content: content!)
    .on(next: {
        app.userNotification.pushNotification(openURLAction: $0.URL) //this is an NSUserNotification
    })
    .on(error: print)
    .start()

The function uses the Gists API

import Cocoa
import ReactiveCocoa
import SwiftyJSON

enum ConnectionError: ErrorType {

    case Bad(String)
    case Worse(String)
    case Terrible(String)
}

/**
- TODO: Add OAuth2 towards GitHub as a protocol
- TODO: Make it RAC
- TODO: Store gistID in NSUserDefaults
*/
public final class GistService: NSObject {

    //MARK:- Properties
    var userDefaults: NSUserDefaults
    var gistAPIURL: NSURL
    var gistID: String?

    //MARK:- Initialisation

    override init() {
        userDefaults = NSUserDefaults.standardUserDefaults()
        gistAPIURL = NSURL(string: "https://api.github.com/gists")!
        super.init()
    }

    convenience init(apiURL: String) {
        self.init()
        self.gistAPIURL = NSURL(string: apiURL)!
    }

    //MARK:- Public API

    func resetGist() -> Bool {
        //    userDefaults.removeObjectForKey("gistID")
        self.gistID = nil

        return !gistUpdate()
    }

    func setGist(content content: String,
        isPublic: Bool = false, fileName: String = "Casted.swift") // defaults
        -> SignalProducer<(URL: String, gistID: String), ConnectionError> {

            return SignalProducer {sink, disp in

                let gitHubHTTPBody = [
                    "description": "Generated with Cast (cast.lfaoro.com)",
                    "public": isPublic,
                    "files": [fileName: ["content": content]],
                ]

                let request: NSMutableURLRequest
                if self.gistUpdate() {
                    let updateURL = NSURL(string: self.gistAPIURL.path! + self.gistID!)!
                    request = NSMutableURLRequest(URL: updateURL)
                    request.HTTPMethod = "PATCH"
                } else {
                    request = NSMutableURLRequest(URL: self.gistAPIURL)
                    request.HTTPMethod = "POST"
                    request.HTTPBody = try! JSON(gitHubHTTPBody).rawData()
                }

                let session = NSURLSession.sharedSession()
                let task = session.dataTaskWithRequest(request) { (data, response, error) in
                    if let data = data {
                        let jsonData = JSON(data: data)
                        if let url = jsonData["html_url"].string, id = jsonData["id"].string {
                            sendNext(sink, (url, id))
                            sendCompleted(sink)
                        } else {
                            sendError(sink, ConnectionError.Bad("URL or ID Invalid"))
                        }
                    } else {
                        sendError(sink, ConnectionError.Worse(error!.localizedDescription))
                    }
                }
                task.resume()
            }
    }

    //MARK:- Helper functions

    func gistUpdate() -> Bool {
        guard let _ = self.gistID else { return false }
        return true
    }
}

/**
- TODO: Add OAuth2 towards GitHub as a protocol
- TODO: Make it RAC
- TODO: Store gistID in NSUserDefaults
*/

The right way to mark to-dos in Swift is like this:

// TODO: Some note about what you need to do.

Per this Stack Overflow answer, it is the most likely variant that Apple will eventually build into Xcode to automatically generate notes for, but additionally, a script is provided in that answer for generating this warning in the meantime.

public final class GistService: NSObject

It's great that this class is public.  It's the only way to use our classes across frameworks... but none of the properties or methods of this class are marked as public.  So realistically, the class really can't be used outside the framework it's in.
Why is this class final?  What's the specific problem with inheriting from this class?
Why is this class inheriting from NSObject?  That would make sense in Objective-C (where every class must inherit from NSObject), but I don't see what we're getting from NSObject here.  (In fact, later we'll see that inheriting from NSObject actually causes us problems.)
I'm not sure how much I like the name GistService.  Look at the names of Apple classes.  Are any of them "services"?

override init() {
    userDefaults = NSUserDefaults.standardUserDefaults()
    gistAPIURL = NSURL(string: "https://api.github.com/gists")!
    super.init()
}

convenience init(apiURL: String) {
    self.init()
    self.gistAPIURL = NSURL(string: apiURL)!
}

First of all, userDefaults is unused here and should be eliminated.

Second of all, I'm not sure how appropriate is to default to GitHub.  I just don't know at all how appropriate that is.  If someone using the library wants a more convenient way of instantiating with this (or any other) default base URL, they can always extend the class.

Third of all, strings are not URLs and should not be called as such.

Fourth, and absolutely most importantly, you've done this backwards.  You have the wrong init method marked as convenience.  I think that inheriting from NSObject prevents you from marking the zero-argument init has convenience (and if so, all the more reason not to inherit from NSObject).

The designated initializers should be the ones that take more arguments than anything else.  The convenience initializers call those designated initializers.  They are convenient because they take fewer arguments and assume default values for the arguments that the designated initializers require.

So, with all these things in mind... we should start with this init method:

required init(baseURL: NSURL) {
    self.gistAPIURL = baseURL
}

And I'd argue we don't want the zero-argument init at all.  (Why this is required, I'll address in a minute.)

If we really want to provide a means of instantiating with the GitHub url, then that should look something like this:

class func gitHubGist() -> Self {
    let gitHubURL = NSURL(string: "https://api.github.com/gists")!
    return self.init(baseURL: gitHubURL)
}

We want to return Self so that this works with inheritance.  And to do this properly, we have to call self.init.  The only way this works with the Self return type is if the init we're calling is marked as required.

So now, if I want a gist of my own choosing, I use this approach:

let myGist = GistService(baseURL: fooBarGistServiceAPIURL)

And if I want GitHub's, I can conveniently just do this:

let gitHubGist = GistService.gitHubGist()

//MARK:- Public API

This is not how you make methods public.  As I mentioned earlier, our class is public and can be seen outside of the framework... but none of its methods or properties are.  If these methods are intended to be part of the API, we need to start by marking them with public.

But that's not enough.  We should also use AppleDoc style comments so that without having to navigate to your website, or where ever you're keeping documentation, the user can get an overview of what this method or property is or does right there in Xcode.

I highly recommend you check out this NSHipster article on all of what you can do with AppleDocs, but in the meantime, here's a little preview:

The comments I provided in lines 5 through 12 generated the content for the pop-up box (which you get by holding Option and clicking the function name).  This documentation crosses file and framework boundaries.  It's the surest way to make sure your documentation is visible to the people actually using your code.  It's comment visible if you're browsing the source and documentation available when you're using the API.

//    userDefaults.removeObjectForKey("gistID")

If this code is unused, remove it.  Let source control manage the fact that it used to be there.  If you don't remove it, some maintainer will.  And if it doesn't need to be removed, it needs a comment explaining why it needs to stay.

//MARK:- Helper functions

func gistUpdate() -> Bool {
    guard let _ = self.gistID else { return false }
    return true
}

First, if these are "Helper" functions, they should almost certainly be private (they're probably not needed outside this file, are they?).  But problems exist beyond the access level of this method.

What's it even doing?  It's certainly not updating anything.

If I only see the method signature, I see this:

func gistUpdate() -> Bool

And that's about all I see when I come across it in code (where your library uses it).

So, let's look at the actual implementation:

guard let _ = self.gistID else { return false }
return true

This is arguably the most confusing way of writing:

return self.gistID != nil

But this is a really bad method.  We don't need a method for this.  We need optional binding.  If you're not comfortable with optional binding, it's definitely something you need to work on before you venture into anything more complicated in Swift.

So let's take a closer look at where you're calling this method.  Here's a place:

if self.gistUpdate() {
    let updateURL = NSURL(string: self.gistAPIURL.path! + self.gistID!)!
    request = NSMutableURLRequest(URL: updateURL)
    request.HTTPMethod = "PATCH"
}

And this is certainly wrong.  The better approach would start with:

if let gistID = self.gistID {
    // we know gistID is non-nil, and it's already unwrapped...
}

enum ConnectionError: ErrorType {

    case Bad(String)
    case Worse(String)
    case Terrible(String)
}

These classifications for errors are far from sufficient.  The severity of the error should be a sub-property of the classification of the error.  Our errors should be classified into more specific types.  For example:

No Internet.
No response from server.
Invalid credentials.
Invalid path.

Etc.  This is very simplified and probably not a complete list, but hopefully you get the point.  Now, these should probably still be in an enum and still be paired with human-readable string descriptions of the problem, and perhaps a severity indication... but we need to start by classifying the type of error.

Why?

Most importantly, because it will determine how someone using your library will respond to the error.  This will be a far better indicator of the best course of action for responding to the error than some vague severity that you're classifying yourself (and is probably highly subjective generally).


Implementing a set using a linked list My task is to add the specified element to the linked list if it is not already present

/**
* Adds the specified element to this set if it is not already present.
* @param o element to be added to this set.
* @return  true if this set did not already contain the specified element.
* @throws NullPointerException - if the specified element is null and this set  does
  not support null elements
 */

This is my solution

public boolean add(E o)  {

    if (o == null)
        throw new NullPointerException();

    ListNode newNode = new ListNode(o, null);
    if (contains(o))
    {
     return false;
    }
    else {

    head.next=newNode;
    head=newNode;
    return true;
   }
}

Can please anybody check it and correct if there are mistakes, or give hints to correct it?

Let's say that you are starting with a list containing five elements:

$$
\newcommand{ptr}[1]{\overset{\mathtt{#1}}{\longrightarrow}}
\mathtt{head} \ptr{} \fbox{first}  \ptr{next}
                     \fbox{second} \ptr{next}
                     \fbox{rest}   \ptr{next}
                     \fbox{of}     \ptr{next}
                     \fbox{list}   \ptr{next} \mathtt{null}
$$

Then, you add a non-duplicate item called "another".  After executing head.next=newNode:

$$
% "align" allows us to do vertical alignment
% "\\" separates lines
% "&" is an alignment marker. The markers in different rows
% are positioned above each other, and horizontally centered.
\newcommand{ptr}[1]{\overset{\mathtt{#1}}{\longrightarrow}}
\begin{align*}
                                             &\mathtt{newNode} \\
                                             &\quad\downarrow  \\
\mathtt{head} \ptr{} \fbox{first} \ptr{next} &\fbox{another} \ptr{next} \mathtt{null} \\
                                             &\fbox{second}  \ptr{next}
                                              \fbox{rest}    \ptr{next}
                                              \fbox{of}      \ptr{next}
                                              \fbox{list}    \ptr{next}
                                              \mathtt{null}
\end{align*}
$$

You've just orphaned and discarded everything starting from the second element.  Then it gets worse when you set head=newNode.


Project Euler # 22 names scores in Python Using names.txt (right click and 'Save Link/Target As...'), a 46K text file containing over five-thousand
first names, begin by sorting it into alphabetical order. Then working out the alphabetical value for
    each name, multiply this value by its alphabetical position in the list to obtain a name score.

For example, when the list is sorted into alphabetical order, COLIN, which is worth \$3 + 15 + 12 + 9 + 14 = 53\$,
is the 938th name in the list. So, COLIN would obtain a score of \$938 × 53 = 49714\$.

What is the total of all the name scores in the file?

import string

def calculate_score(filename):
    """Reads names and returns total scores."""
    letter_scores = {letter: ord(letter) - 64 for letter in string.ascii_uppercase}
    with open(filename) as names:
        total = 0
        names = sorted(names.read().replace('"', '').split(','))
        for index, name in enumerate(names):
            temp = 0
            for letter in name:
                temp += letter_scores[letter]
            total += temp * (index + 1)
        return total

if __name__ == '__main__':
    print(calculate_score('p022_names.txt'))

The code looks quite good in general. Without a deeper look at the problem, there are twothree things that can easily be improved:

Building the look-up table

You use

letter_scores = {letter: ord(letter) - 64 for letter in string.ascii_uppercase}

which makes use of the ASCII encoding to determine the value of each letter. The following could be considered a little bit more straightforward:

letter_scores = {letter: index for index, letter in enumerate(string.ascii_uppercase, 1)}

This uses enumerate with a starting value of 1 to assign the position in the alphabet.

Iterating over the sorted names

When iterating over the sorted names, you already use enumerate to get index, but since you only use index+1, you can reuse the trick from above to simplify this even further (as @RootTwo rightfully pointed out in his comment).

Computing the score

To compute the score you use a for loop with an intermediate variable:

temp = 0
for letter in name:
    temp += letter_scores[letter]
total += temp * (index + 1)

This can be rewritten using a generator expression and sum:

total += sum(letter_scores[letter] for letter in name) * (index + 1)

It basically allows you to get rid of the additional counter variable.

Going nuts

You can also reapply that generator expression idea to the first loop and end with something like:

def calculate_score(filename):
    """Reads names and returns total scores."""
    letter_scores = {letter: index for index, letter in enumerate(string.ascii_uppercase, 1)}
    with open(filename) as names:
        names = sorted(names.read().replace('"', '').split(','))
        return sum(
            sum(letter_scores[letter] for letter in name) * index
            for index, name in enumerate(names, 1)
        )


Pandas transform series values until condition is met without for loop I have a pandas Series contain 0s and 1s. Now I want to convert all the 0s to 1s which come before the first 1, and rest of the 1s to 0s. I can achieve that using the below code:
import pandas as pd
a = [0,0,1,0,0,1]
df = pd.Series(a)
flag = True
for i in range(df.shape[0]):
  if (df[i]!=1) & flag:
    df[i]=1
  elif flag:
    flag=False
  else:
    df[i]=0
print(df)

Final dataframe:
[1,1,1,0,0,0]

But how can I optimize it? Perhaps by avoiding the for loop?

Use a combination of Series.cummax() and Series.shift()
a = pd.Series([0,0,1,0,0,1])

a.cummax()                               # [0,0,1,1,1,1]

a.cummax().shift(fill_value=0)           # [0,0,0,1,1,1]

Then "invert" the 0's and 1's
df = 1 - a.cummax().shift(fill_value=0)  # [1,1,1,0,0,0]


Looking for efficient python program for this following python script I am looking for a memory efficient python script for the following script. The following script works well for smaller dimension but the dimension of the matrix is 5000X5000 in my actual calculation. Therefore, it takes very long time to finish it. Can anyone help me how can I do that?

def check(v1,v2):
    if len(v1)!=len(v2):
        raise ValueError,"the lenght of both arrays must be the same"
    pass
def d0(v1, v2):
    check(v1, v2)
    return dot(v1, v2)
import numpy as np
from pylab import *
vector=[[0.1, .32, .2, 0.4, 0.8], [.23, .18, .56, .61, .12], [.9, .3, .6, .5, .3], [.34, .75, .91, .19, .21]]
rav= np.mean(vector,axis=0)
#print rav
#print vector
m= vector-rav
corr_matrix=[]
for i in range(0,len(vector)):
    tmp=[]
    x=sqrt(d0(m[i],m[i]))
    for j in range(0,len(vector)):
        y=sqrt(d0(m[j],m[j]))
        z=d0(m[i],m[j])
        w=z/(x*y)
        tmp.append(w)
    corr_matrix.append(tmp)
print corr_matrix

I noticed one of your issues is that you are calculating the same thing over and over again when it is possible to pre-calculate the values before looping. First of all:
x = sqrt(d0(m[i], m[i]))

and:
y = sqrt(d0(m[j], m[j]))

are doing the exact same thing. You are calculating the sqrt() of dot product of the entire length of the vector length^2 times! For your 5000 element vector list, that's 25,000,000 times.
When it comes to iterating over giant mounds of data, try to pre-calculate as much as possible. Try this:
pre_sqrt = [sqrt(d0(elem, elem)) for elem in m]
pre_d0 = [d0(i, j) for i in m for j in m]

Then, to reduce the amount of repetitive calls to len() and range():
vectorLength = len(vector)
iterRange = range(vectorLength)

Finally, this is how your for loops should look:
for i in iterRange:

    tmp = []
    x = pre_sqrt[i]

    for j in iterRange:

        y = pre_sqrt[j]
        z = pre_d0[i*vectorLength + j]

        w = z / (x * y)

        tmp.append(w)

    corr_matrix.append(tmp)

When I timed your implementation vs mine these were the average speeds over 10000 repetitions:

Old: .150 ms
Mine: .021 ms

That's about a 7 fold increase over your small sample vector. I imagine the speed increase will be even better when you enter the 5000 element vector.
You should also be aware that there could be some Python specific improvements to be made. For example, using lists vs numpy arrays to store large amounts of data. Perhaps someone else can elaborate further on those types of speed increases.


Chunk a large set of characters by a specified set of delimiters and a maximum chunk size Problem
I need to be able to grab a subset of characters but ensure that each set doesn't exceed a specified limit and must end with a defined set of characters. For example, say I have an entire book worth of characters, I need to chunk that into a set of 300 characters but ensure that none of those chunks is split mid-sentence.
To put it into pesudo code I basically need:
foreach(var chunk in text.Chunk(x).ButAlsoSplitOnFirst('.', '!', '?', etc...);

My solution
public static class StringExtensions
    {
        public static IEnumerable<string> ChunkWithDelimeters(this string source, int maxChar, params char[] delimeters)
        {
            var sourceSpan = source.AsSpan();
            var items = new List<string>();

            var pointer = 0;
            var lazyPointer = 0;

            while (pointer <= sourceSpan.Length)
            {
                bool foundMatch = false;

                pointer = Math.Min(lazyPointer + maxChar, sourceSpan.Length);

                if (pointer == sourceSpan.Length)
                {
                    items.Add(sourceSpan.Slice(lazyPointer, pointer - lazyPointer).TrimStart().ToString());
                    break;
                }

                for (var j = pointer; j >= lazyPointer; j--)
                {
                    var tempChar = sourceSpan[j];

                    if (delimeters.Contains(tempChar))
                    {
                        foundMatch = true;
                        items.Add(sourceSpan.Slice(lazyPointer, j + 1 - lazyPointer).TrimStart().ToString());
                        lazyPointer = j + 1;
                    }
                }

                if (!foundMatch)
                {
                    items.Add(sourceSpan.Slice(lazyPointer, pointer - lazyPointer).Trim().ToString());
                    lazyPointer = pointer;
                }
            }

            return items;
        }
    }

Benchmarks

source.Length
maxChar
delimiters
Mean (μs)
Error (μs)
StdDev (μs)

7320000
30
'.'
73,622.739
1,412.4035
1,252.0589

439200
30
'.'
4,122.574
68.6392
60.8468

731
30
'.'
5.125
0.0466
0.0436

My question
What is the optimal solution to this problem? Is there any way I can improve the performance of my solution?

Don't you need to test for delimiters here?  From reading your code I assume we want to max chunk but if inside the chunk there are delimiters we want to break that up.  If the last section fits in the entire chunk it's not checking for delimiters
if (pointer == sourceSpan.Length)
{
    items.Add(sourceSpan.Slice(lazyPointer, pointer - lazyPointer).TrimStart().ToString());
    break;
}

Also you could use IndexOfAny instead of the loop at the end
WARNING this code not tested and just used an example
var slice = sourceSpan.Slice(lazyPointer, pointer - lazyPointer);
var contains = slice.IndexOfAny(delimeters);
while (contains > -1)
{
    items.Add(slice.Slice(0, contains + 1).ToString());
    slice = slice.Slice(contains + 1);
    contains = slice.IndexOfAny(delimeters);
}

Now some other points that I think you should split by delimiters first then chunks.  Instead of chunks then delimiters.  If I have a string of  "12345.6789.8.654321" and split by 7.   The first two coming out would be "12345" <-- looks good but then the next chunk would be "6" then chunk after that would be "789","8","6","54321"  if we split by delimiters first then chunked them it would output what I think most people would expect.


Time and Space Complexity of Leetcode Problem #31. Next Permutation The question is as follows: Given a collection of distinct integers, return all possible permutations. Example:

Input: [1,2,3]
Output:
[
  [1,2,3],
  [1,3,2],
  [2,1,3],
  [2,3,1],
  [3,1,2],
  [3,2,1]
]

Here is my backtracking solution for the problem, where I added the num_calls variable to keep track of the number of times that the backtrack function is called recursively.

class Solution:
    def permute(self, nums):
        answer = []
        num_calls = []

        def backtrack(combo, rem):
            if len(rem) == 0:
                answer.append(combo)
            for i in range(len(rem)):
                num_calls.append(1)
                backtrack(combo + [rem[i]], rem[:i] + rem[i + 1:])

        if len(nums) == 0:
            return None
        backtrack([], nums)
        print(len(num_calls))
        return answer

I can't make sense of any of the answers that I have seen thus far for the time and space complexity of this solution.

Some people say its worst case O(n * n!), but looking at the len of num_calls doesn't verify this claim.

For example, for:

test = Solution()
print(test.permute([1, 2, 3]))

length of num_calls = 15, which != n * n! = 3 * (3*2*1) = 18

, for:

test = Solution()
print(test.permute([1, 2, 3, 4]))

length of num_calls = 64, which != n * n! = 4 * (4*3*2*1) = 96.

, and for:

test = Solution()
print(test.permute([1, 2, 3, 4, 5]))

length of num_calls = 325, which != n * n! = 5 * (5*4*3*2*1) = 600

Can someone please explain this in a simplified and easily understandable manner?

The number of calls indeed grows slower than \$n*n!\$. Its growth is only \$O(n!)\$. However, number of calls is not the only source of complexity.

Constructing arguments for the recursive call also takes time, and rem[:i] + rem[i + 1:] is on average linear in terms of n. The total complexity therefore is \$O(n*n!)\$, just as some people say.


Generating bounded & unique random numbers in the Linux kernel I recently needed to generate a series of unique random numbers (non-repeated) within a bounded range inside of the Linux kernel. The code I came up with is below. I'd appreciate any feedback -- again, please keep in mind that it's within the Linux kernel, so the standard C library is out of the question.

#include <linux/kernel.h>
#include <linux/random.h>
#include <linux/slab.h>

static int mod(int, int);
static int _is_repeated_int(int *, int, int);

/* proj2_create_randints - creates a buffer of random integers
 * @dstruct_size: number of random integers
 * @max_range: maximum range of values to generate
 *
 * Allocates a buffer that can hold @dstruct_size random integers. If
 * @max_range is greater than 0, mods all values by @max_value to ensure
 * they fall within the range. If @dstruct_size is greater than @max_range,
 * returns NULL. The buffer returned is guaranteed to have no duplicate values.
 *
 * On successs, it fills the buffer and returns a pointer to the buffer;
 * otherwise returns NULL.
 */
int *proj2_create_randints(int dstruct_size, int max_range)
{
    int *randint;
    int tmp, idx;

    if ((max_range != 0) && (dstruct_size > max_range))
        return NULL;

    randint = kmalloc(dstruct_size * sizeof(*randint), GFP_KERNEL);
    if (randint != NULL) {
        idx = 0;
        while (idx < dstruct_size) {
            tmp = get_random_int();
            if (max_range > 0)
                tmp = mod(tmp, max_range);
            /* Check if 'tmp' is already in array */
            if (!_is_repeated_int(randint, idx, tmp)) {
                randint[idx] = tmp;
                idx++;
            }
        }

    }
    return randint;
}

/* mod - returns x modulo m
 * @x: value to reduce
 * @m: modulo value
 *
 * Guaranteed to be positive.
 */
static int mod(int x, int m)
{
    return (x%m + m)%m;
}

/* _is_repeated_int - checks array against repeated values
 * @array: array of integers to check
 * @size: size of @array
 * @new_value: value to check array for
 */
static int _is_repeated_int(int *array, int size, int new_value)
{
    int i;

    if (size == 0)
        return 0;

    for (i = 0; i < size; i++) {
        if (array[i] == new_value) {
            return 1;
        }
    }

    return 0;
}

What are the valid input ranges? The documentation flags two special cases and the code handles them as special cases, but

I don't see any checks for dstruct_size < 0
If max_range is very large, the implementation of mod is incorrect. Consider e.g. mod(1 << 30, (1 << 30) + 1): the addition overflows into a negative value.

On the assumption that the intention is to guarantee that max_range will be zero or positive, why int instead of uint? Using uint would encode the guarantee of non-negative return values, and would remove the need for the mod helper.

            /* Check if 'tmp' is already in array */
            if (!_is_repeated_int(randint, idx, tmp)) {
                randint[idx] = tmp;
                idx++;
            }

is very inefficient. Consider the worst case: dstruct_size == max_range. The expected number of calls to get_random_int will be quadratic in max_range, and the overall running time of the method will be cubic.

A simple improvement which requires only dstruct_size calls to get_random_int and overall quadratic time is the following (pseudocode):

for (idx = 0; idx < dstruct_size; idx++) {
    tmp = (uint)get_random_int() % (max_range - idx);
    for (cmpIdx = 0; cmpIdx < idx; cmpIdx++) {
        // Discussed below
        if (tmp >= sorted(randint)[cmpIdx]) tmp++;
    }
    randint[idx] = tmp;
}

Obviously it needs minor amendment to handle the case max_range = 0, but that's not too difficult, especially if working with uint.

The big issue is that, as JS1 pointed out in comments, the loop which increments to avoid collisions needs to process the previous numbers in ascending order. There are two ways to do this: either keep a second, temporary, buffer with the numbers sorted; or sort randint as we go and then Fisher-Yates shuffle at the end. One uses twice as much memory; the other uses twice as many calls to get_random_int(). Both use insertion sort, and leave the overall running time as quadratic.

The Fisher-Yates option, however, does allow a fairly tricky elegant insertion.

for (i = 0; i < dstruct_size; i++) {
    tmp = (uint)get_random_int() % (max_range - i);
    for (j = 0; j < i; j++) {
        if (tmp >= randint[j]) tmp++;
        else {
            tmp2 = tmp;
            tmp = randint[j];
            randint[j] = tmp2;
        }
    }
    randint[i] = tmp;
}
// TODO Now Fisher-Yates shuffle

Observe that because randint contains sorted distinct values, once we find the insertion point for tmp we will never again hit the first case. In fact, that means we could optimise the inner loop and assignment to randint[i] as

    for (j = 0; j < i && tmp >= randint[j]; j++, tmp++) {
        // Empty loop body
    }
    for (; j <= i; j++) {
        tmp2 = tmp;
        tmp = randint[j];
        randint[j] = tmp2;
    }

I've followed your code in taking get_random_int() modulo a value, and that's ok if you don't need uniformity. However, in some cases the fact that get_random_int() % m is more likely to be 0 than m-1 (unless m is a power of two) is a problem. In that case you'd want an auxiliary get_uniform_random_int(m).


Pass Cassandra session reference to http handlers I'm building a golang backend using gorilla/mux and gocql. My objective is to create a session to a remote cassandra and share the very same session to a serie of handlers.

I came out with this implementation:

server.go:

package main

import (
    "fmt"
    "net/http"
    "os"

    "github.com/andream16/price-probe-go/api/item/rest"
    "github.com/andream16/price-probe-go/cassandra"
    "github.com/gorilla/mux"
)

func main() {

    fmt.Println("Initializing Cassandra connection . . .")
    s, err := cassandra.InitCassandraClient()
    if err != nil {
        fmt.Println("Unable to set up Cassandra client!")
        os.Exit(1)
    }
    fmt.Println("Successfully initialized Cassandra connection . . .")

    fmt.Println("Starting server . . .")
    router := mux.NewRouter()
    router.HandleFunc("/item", itemrest.ItemHandler(&s))

    . . .

}

I create a new HandleFunc for each route and pass it &s.

And each handler is like:

func ItemHandler(session *gocql.Session) func(w http.ResponseWriter, r *http.Request) { }

Is there a more elegant way to achieve this?

Yes, there definitely is a better way to do this. The easiest way would be to use reciever functions (aka methods) as handler functions. Rather than returning a handler from the ItemHandler func (bad name BTW), simple declare a handler directly.

Here's an example of how you could acchieve this:

type MyServer struct {
    cql *gocql.Session
}

func (s *MyServer) ItemHandler(w http.ResponseWriter, r *http.Request) {
    // use s.cql here
}

func New(cql *gocql.Session) (*MyServer, error) {
    s := &MyServer{
        cql: cql,
    }
    // more stuff here
    return s, nil
}

Then, in your main package, simply write this:

s, err := cassandra.InitCassandraClient()
if err != nil {
    fmt.Println("Unable to set up Cassandra client!")
    os.Exit(1)
}
// assume package name server
srv, _ := server.New(&s)
// now ItemHandler with srv as receiver is the handler
router.HandleFunc("/item", srv.ItemHandler)


Parsec for primes and fibs I decided to spend some time working with parsec. I've written some code to check for primality and for whether something is a Fibonacci number. I'm mainly hoping for feedback on readability, but if you have any performance tips I'd be interested in those as well.

import Text.Parsec
import Text.Parsec.Error
import Control.Applicative hiding ((<|>))

toXTerminatedAs :: Int -> String
toXTerminatedAs = end . parse (manyAccum acc digit) "" . show
  where acc digit cur = concat (replicate 10 cur) ++ toAs digit
        toAs digit    = replicate (fromEnum digit - 48) 'a'
        end (Left _)  = error "I can't deal with negative numbers, sorry"
        end (Right x) = x ++ "x"

isLeft :: Either a b -> Bool
isLeft (Left _) = True
isLeft _        = False

fib :: Int -> Bool
fib = not . isLeft . parse (fibParser "a" "") "" . toXTerminatedAs

fibParser a b = char 'x' <|> (\_ -> fibParser b (a++b)) =<< string a

prime :: Int -> Bool
prime = isLeft . parse (char 'x' <|> primeParser "a") "" . tail . toXTerminatedAs

primeParser x = (\c -> try (many1 (string (c:x)) *> char 'x') <|> primeParser (c:x)) =<< char 'a'

toXTerminatedAs is pretty ugly, but I haven't been able to think of any improvements. As the name/signature implies it turns a number n into n 'a's followed by an x.

prime works for all natural numbers, but it returns True for 0, and I'm not really sure what to do about it besides hard-coding it in somewhere.

Parsers are for parsing

This is probably the strangest primality test I've ever seen. So for those that want to write their own primality test, please don't use this approach.

Almost all your functions can be written without a Parser. toXTerminatedAs is probably the best example:

toXTerminatedAs :: Int -> String
toXTerminatedAs n = replicate n 'a' ++ "x"

That's easy to read, and more likely to be correct. Either way, I assume you know that, so let's get started with the actual review.

Try not to shadow existing names

We return to toXTerminatedAs:

toXTerminatedAs :: Int -> String
toXTerminatedAs = end . parse (manyAccum acc digit) "" . show
  where acc digit cur = concat (replicate 10 cur) ++ toAs digit
        toAs digit    = replicate (fromEnum digit - 48) 'a'
        end (Left _)  = error "I can't deal with negative numbers, sorry"
        end (Right x) = x ++ "x"

We have three digits here. Two of them are function arguments, but the first one is Parsec's digit function. That's misleading. Use d or another name that's not already in use. Alternatively, import Parsec qualified or as P to show the difference more clearly.

Use the standard library

We stay at toXTerminatedAs. fromEnum digit - 48 isn't that clear to someone who doesn't know Enum instances or 48 well. But digitToInt shows its intend very well. It's provided by Data.Char:

import Data.Char

-- ....

toXTerminatedAs :: Int -> String
toXTerminatedAs = end . parse (manyAccum acc digit) "" . show
  where acc d cur     = concat (replicate 10 cur) ++ toAs d
        toAs d        = replicate (digitToInt d) 'a'
        end (Left _)  = error "I can't deal with negative numbers, sorry"
        end (Right x) = x ++ "x"

isLeft is provided by Data.Either, so no reason to implement it yourself.

Also, try to avoid ++ in recursive functions or accumulators, as it turns \$\mathcal O(n)\$ algorithms into \$\mathcal O(n^2)\$ ones.

Encapsulate often used functionality in a function

You often use parse parser "". That's somewhat error prone as you can accidentally forget the file name.

Instead write a small function that takes care of the SourceName for you:

parse' :: Stream s Identity t => Parsec s () a -> s -> Either ParseError a
parse' p = parse p ""

While we're at it, we add a

parseViaX p = isLeft . parse' p . toXTerminatedAs

Add type signatures

parseViaX is missing its type signatures because I don't know the ones of fibParser and primeParser. Always add type signatures to top-level bindings. Not only will it serve as minimal documentation, but it will also make sure that the compiler does not infer too general types or types you didn't expect.

Document complicated expressions

fibParser and primeParser should get some documentation.

Bottom line

Don't mix parsing and arithmetic, at least for arithmetic tests. It's messy and hard to read. Both fib and prime are easy to write without any parsing at all.

If you were going for code obfuscation: kudos to you, that was brilliant.


Simple stack implementation I am not experienced in C++, and wrote this code for implementing a stack data structure using a linked list.

Can you please point out any flaws, errors, stupid mistakes, general improvements etc that can make this code better?

#include <iostream>
using namespace std;

struct Node;

struct Node {
    int data;
    Node* next;
};

class Stack {
public:
    Node* first;
    Node* last;

    Stack() {
        first = 0;
        last = 0;
    }

    ~Stack() {
         while (first != 0) { pop(); }
    }

    Stack& push(int value) {
        Node* temp = new Node;
        temp->data = value;
        temp->next = 0;

        if (first == 0) {
            first = temp;
            last = temp;
        } else {
            last->next = temp;
            last = last->next;
        }

        cout << "Pushed " << value << " on the stack\n";

        temp = 0;
        return *this;
    }

    Stack& pop() {
        if (first == 0) {
            cout << "No nodes to pop.\n";
        } else {
                // only one node left
                if (first == last) {
                cout << "Popped " << first->data << " off the stack\n";
                delete first;
                first = 0;
                last = 0;
                return *this;
            }

            Node* temp = first;
            while (temp->next != last) {
                temp = temp->next;
            }
            cout << "Popped " << last->data << " off the stack\n";
            delete last;
            last = temp;
            last->next = 0;
        }
        return *this;
    }
};

int main() {
    Stack s;
    s.pop();
    s.push(1);
    s.push(2);
    s.pop();
    s.push(3).push(4);
    s.pop().pop();
    return 0;
}

A few comments, wrt style.

#include <iostream>
using namespace std;

You don't need a forward declaration of struct Node.

//struct Node;

You could add a constructor here just like in class. So your initialization
is simpler.

struct Node {
    int data;
    Node* next;
    Node(int d):data(d), next(0){}
};

It may be a good idea to write an abstract class as an interface, and then write
the concrete class to conform to it. Another idea is to try to templatize Node so that the data can be any type. Also look at the stack class in STL

You may also want top() and empty() methods

on logic: If you are going to implement a stack using a linked list, you don't really need to keep a first and a last. Just keep the reference to the top element. On push, create a new node, set its next to the current top, and set it to top. On pop, set the top to the next of current top, and delete the node. Something along the lines of: (may contain bugs.)

struct Node {
    int data;
    Node* next;
    Node(int v, Node* n):data(v),next(n) {}
};

class Stack {
  public:
    Node* top;
    Stack():top(0){}
    ~Stack() {
      while (top != 0) pop();
    }
    Stack& push(int value) {
      top = new Node(value, top);
      return *this;
    }
    Stack& pop() {
      if (!top) throw "No nodes to pop.";
      Node* t = top;
      top = top->next;
      delete t;
      return *this;
    }
};


Hexadecimal number to decimal conversion I am a semi-beginner in C++ and I have a project in C++ to convert a hexadecimal integer into decimal equivalent, but I extended it and now this program can also convert a fractional part.

Logic I have used:

First the whole program is inside a do-while loop so that the user can convert a number as many time as desired.
Another do-while loop checks if the inputted number is valid or invalid. If invalid, it again asks the user for input.
A for loop counts the number of characters before the decimal point (if any).
Another for loop  converts the characters before the decimal point into a decimal number.
Another for loop converts the characters after the decimal point into a decimal fraction and adds it to the original number.
It prints it and asks if the user wants to again run the loop.

I have knowledge on selection statements (if-else), loops (for, while, do-while), operators and other basic things in C++.

Note: This program is compiled with MinGW and CodeBlocks. My school teacher uses TurboC++.

Please provide advice on things which I can understand; don't get too advanced. I want to score full marks on my school project but also want to understand the code.

// This is a C++ project for Unit Test 2 (2015-16). This program can convert a hexadecimal number into decimal equivalent.

#include <iostream>
#include<string.h>
#include<math.h>

using namespace std;
int main()
{
char choice;
cout<<" *** HEXADECIMAL TO DECIMAL CONVERTER ***";
do{                  //The whole program is inside a do-while loop, so that user can convert a number as many times as they want.
char hexd[120];
                                                         //This string will store the hexadecimal number input by the user.
int validn_count=0,length,lcopy,p=1,num,i,countp=0;
double decimal=0;

do{                                 //This do-while loop checks if the inputed number is valid or not. If not valid it again asks to enter the number.
cout<<"\n Enter a hexadecimal number for conversion (0-F): ";
cin>>hexd;
length=strlen(hexd);
for(int i=0;i<length;++i)
{
    if( (hexd[i]>=48 and hexd[i]<=57) or (hexd[i]>=65 and hexd[i]<=70) or      (hexd[i]>=97 and hexd[i]<=102) or hexd[i]==46)
      ++validn_count;
    else
       {
        validn_count=0;
        cout<<"\n ! WARNING! Invalid HEXADECIMAL NUMBER!!!!";
        break;
       }
}

}while(validn_count==0);

for(i=0;i<length;i++)   //This for loop checks how many characters are there before the decimal point (.)
{
    if(hexd[i]==46)
     break;
   else
     ++countp;

}
lcopy=countp;          // Copying the value of countp for usage in the  conversion.

for(i=0;i<countp;++i)  // This for loop converts all the character before    the decimal point (.) into decimal number and stores it in the variable 'decimal'.
 {
  if(hexd[i]>=48 and hexd[i]<=57)
    num=((int)hexd[i]-48);
  else if(hexd[i]>=65 and hexd[i]<=70)
   num=((int)hexd[i]-55);
  else if (hexd[i]>=97 and hexd[i]<=102)
    num=((int)hexd[i]-87);

 decimal=decimal+(num*pow(16,--lcopy));
 }

for(i=(countp+1);i<length;i++) // This for loop converts all the character after the decimal point (.) into decimal number and further stores it in the variable 'decimal'.
    {

    if(hexd[i]>=48 and hexd[i]<=57)
    num=((int)hexd[i]-48);
   else if(hexd[i]>=65 and hexd[i]<=70)
   num=((int)hexd[i]-55);
   else if (hexd[i]>=97 and hexd[i]<=102)
   num=((int)hexd[i]-87);

   decimal=decimal+(num*pow(16,-p));
    ++p;                                    //p is used for the multiply with 16 with negative powers.
   }

   cout<<" \n The decimal equivalent of hexadecimal number "<<hexd<<" is "    <<decimal;
  cout<<"\n Do you want to convert another hexadecimal number into decimal?   (Y/N): ";  // Asks if the user again wants to do a conversion.
 cin>>choice;
 }while(choice=='Y' or choice=='y');
cout<<"\n\n **** THANK YOU FOR USING THIS SOFTWARE ****";
return 0;
}

To take a quick first stab at some style issues:

Follow a consistent indentation style. This makes the code extremely readable, and you will thank yourself later when you come back to your code.

For example:

void foo()
{
  do
  {
    bar();
  } while(0);
}

int main()
{
  foo();
}

Try to break down your program into smaller code blocks that you can reason about them independently, and perhaps even test them independently.

For example, how about start with this template, and fill in the code:

// checks
bool isValid(const std::string &hexInput)
{

}

int64_t convertHexToDecimal(const std::string& hexString)
{

}

int main()
{
    std::string input;
    std::cin >> input;

    while(isValid(input))
    {
        // perform the conversion
        int64_t convertedDecimal = convertHexToDecimal(input);

        // output the conversion
        std::cout << "..." << convertedDecimal <<  std::endl;

        // get new input
        std::cin >> input;
    }
}

Use std::string liberally instead of a character array. I think this is a more c++11 style of going about it. This will allow you to utilize the wealth of STL. This is more recognizable, and reduces the changes of introducing bugs in your hand-rolled while/for loop.

auto found = input.find('.');
if (found != std::string::npos)
{
    std::cout << "Period found at: " << found << '\n';
}

Your way of checking if the input string is hexadecimal is very awkward and unreadable. Whenever you use constants, you should declare them as consts, so you can update them without having to change the code.

A hexadecimal string has characters from one of the following set of characters:

0 1 2 3 4 5 6 7 8 9 a b c d e f A B C D E F and the ., also the . should appear exactly once, and should probably not be the last character of the string. Also, you can use the characters directly for the comparison instead of the ASCII values.

bool isValidHex = true;

// for_each will iterate between the the provided iterators
// for each position, you can pass a lambda function that checks
// the characters
std::for_each(
    str.begin(),
    str.end(),
    [&isValidHex](char c)
          {
            // std::cout << "Checking" << c << std::endl;
            if ((c >= 'a' && c <= 'f') ||
                (c >= 'A' && c <= 'F') ||
                (c >= '0' && c  <= '9'))
            {
                // this is a valid hex.
            }
            else
            {
                isValidHex = false;
            }
          });

If lambdas are not your style, you can also use the new c++11 loop style, which is much more readable.

for (char c : input)
{
    // perform checks here
}

To Be Continued..


Simple peak finder solution - O(log n) I attempted a solution to a seemingly simple problem:

  Peak Finder

  Let input be a list. input[i] is a peak if input[i] > input[i + 1] and input[i] > input[i - 1].

  Implement a recursive algorithm of time complexity \$O(\log n)\$ to find a peak if it exists.

def peakFinder(input):
        n = len(input)
        middleIndex = n/2
        middleIsMax = input[middleIndex] > input[middleIndex + 1] and input[middleIndex] > input[middleIndex - 1]

        # base case
        if middleIsMax:
            return input[middleIndex]

        leftArray = input[:middleIndex]
        rightArray = input[middleIndex:]

        moveRight = input[middleIndex + 1] > input[middleIndex] and input[middleIndex + 1] > input[middleIndex - 1]

        moveLeft = input[middleIndex - 1] > input[middleIndex] and input[middleIndex - 1] > input[middleIndex + 1]

        # recursive case
        if moveRight:
            return peakFinder(rightArray)
        elif moveLeft:
            return peakFinder(leftArray)

    print peakFinder([2, 41, 17, 11, 13, 7])

Are there improvements to be made? Is the time complexity \$O(\log n)\$?

Slicing a list, as in leftArray = input[:middleIndex], creates a copy and takes \$O(n)\$ time. You can avoid that by passing the whole list together with left and right indices as function arguments.
Another problem with the slicing approach is that you need to look at three values to recognize the peak, but the slices can become shorter than three elements.
Python's chained comparison would be handy here:

middleIsMax = input[middleIndex - 1] < input[middleIndex] > input[middleIndex + 1]


How to use pattern matching within curried function I'm learning F# and I'm trying the following exercise (exercise 4.10 of Functional Programming Using F#).

  Declare an F# function prefix: 'a list -> 'a list -> bool when a:equality. The value of the expression prefix[x_0;x_1;...;x_m] [y_0;y_1;...;y_n] is true if m<=n and x_i=y_i for 0<=i<=m, and false otherwise.

I wanted to use pattern matching on the two inputs, but wasn't sure how to do this with a curried function (as opposed to a function taking a 2-tuple). So I wrote:

let rec prefix a b = (function
    | ([], _) -> true
    | (_, []) -> false
    | (x0::xst,y0::yst) -> x0 = y0 && prefix xst yst) (a,b);;

I wasn't very happy with this, because I thought it would be better (clearer and more efficient) if the inner function called itself, so I tried declaring the inner function locally.

let prefix a b =
    let rec prefixt = function
        | ([], _) -> true
        | (_, []) -> false
        | (x0::xst,y0::yst) -> x0 = y0 && prefixt (xst,yst)
    prefixt (a,b);;

Is this the right way to approach such a problem in F#?

You can use the match … with … syntax:

let rec prefix a b =
    match (a, b) with
    | ([], _) -> true
    | (_, []) -> false
    | (x0::xst,y0::yst) -> x0 = y0 && prefix xst yst


Entering and authorizing Twitter credentials I'm fairly new to Ruby, and I've been refactoring this code (it's a silly app, but it's helping me learn) as I learn more and more best practices and features new to Ruby 1.9 (e.g., hashrocket alternative). I figure having more experienced Rubyists give feedback on the short program will help me learn faster. So, with that in mind, is there anything in this code that isn't a best practice, or anything not so clean about it?

#!/usr/bin/env ruby

require 'twitter'

# Enter your creds
creds = {username:       'YourUsername',
         consum_key:     'SomeLongKeyBlahBlah',
         consum_secret:  'AnotherLongKeyButASecretOneThisTime',
         oauth_token:    'OAuthTokenKeyBlahBlah',
         oauth_secret:   'OAuthSecretTokenOfSecrets'}

# Twitter auth
def twitterAuth(creds)
  Twitter.configure do |config|
      config.consumer_key       = creds[:consum_key]
      config.consumer_secret    = creds[:consum_secret]
      config.oauth_token        = creds[:oauth_token]
      config.oauth_token_secret = creds[:oauth_secret]
  end
end

def sendTweet(creds)
  # Make string
  msg = '@' + creds[:username] + ' ' + ARGV.join(' ')
  # Send!
  Twitter.update(msg)
  # Return tweet
  puts 'Your tweet: ' + msg
end

if __FILE__ == $0
  twitterAuth(creds)
  sendTweet(creds)
end

Generally Rubyists define methods in the underscore-lowercase pattern: def twitter_auth, def send_tweet. Additionally most that I've encountered prefer to drop the () from method calls: twitter_authenticate creds; send_tweet creds

Otherwise this looks OK!


Rearranging array - with empty strings in front and non-empty strings at back Given a string array that either contains empty strings or non-empty
  strings, arrange the contents such that all the empty strings are at
  front and non-empty strings at back, retaining their order from
  original array.

  For example, input:

  {"","a","","d","","o","","g",""}
{"d","o","g"}
{"","","",""}

  Output:

  [, , , , , a, d, o, g]
[d, o, g]
[, , , ]

I wrote two implementations below:

In-place re-arrangement

//in-place arrangement with two pointers running towards starting index from back
public static void arrangeString(String...arr) {
    for (int i=arr.length-1, j=i; i>=0 && j >=0; ) {
        boolean needSwap  =false;
        boolean exit = false;
        while (!exit && arr[i].equals("")) {
            i--; needSwap = true;
            if (i==-1) exit = true;
        }
        while (!exit && !arr[j].equals("")) {
            j--; needSwap = true;
            if (j==-1) exit = true;
        }
        if (exit) break;
        if (needSwap) {
            String temp = arr[i];
            arr[i] = arr[j];
            arr[j] = temp;
        }
        i--; j--;
    }
}

Re-arranging using a temp array

public static String[] arrangeString(String...arr) {
    int len = arr.length;
    String[] temp = new String[len];
    for (int i=0, t1=0, j=len-1, t2=len-1; i<len && j>=0; i++, j--) {
        if(arr[i].equals(""))
            temp[t1++] = arr[i];
        else if(!arr[j].equals(""))
            temp[t2--] = arr[j];
    }
    return temp;
}

Please give me suggestions on how to improve them. It'll be great if you can provide an alternative code snippet replacing my code.

Your code snippets are interesting... both of them.

The second is interesting because I feel it is your better-executed system, you used an interesting bi-directional approach, and it is efficient.

The first is interesting because I prefer the in-place solution, but your implementation is kludgey.... and hard to follow.

Temp-array solution

Notes:

use String.isEmpty().
make len final, it's not a big deal, but I find it helps readability.

As an aside, I would seriously consider a simpler temp-array solution, even though it loops twice:

final int len = arr.length;
final String[] temp = new String[len];
int pos = 0;
for (String s : arr) {
    if (s.isEmpty()) {
        temp[pos++] = s;
    }
}
for (String s : arr) {
    if (!s.isEmpty()) {
        temp[pos++] = s;
    }
}
return temp;

The above solution may be slightly slower (would need testing), but it is also clear, and scales in linear time still. It's not horrible.

In-place solution

This solution is just.... messy. I think a better option is to have two pointers..... one to the first non-empty, and the next to the first empty after that....

private static final int findEmptyState(final String[] data, int index, final boolean empty) {
    while (index < data.length && data[index].isEmpty() != empty) {
        index++;
    }
    return index;
}

with the above helper function, you can:

    final int len = arr.length;
    int notempty = findEmptyState(arr, 0, false);
    int empty = findEmptyState(arr, notempty + 1, true);

    while (notempty < len && empty < len) {

        // shift the next empty value in before the not-empty.
        String e = arr[empty];
        System.arraycopy(arr, notempty, arr, notempty + 1, empty - notempty);
        arr[notempty] = e;

        // find the next coordinates.
        notempty = findEmptyState(arr, notempty, false);
        empty = findEmptyState(arr, notempty + 1, true);
    }
    return arr;

Alternatives

A cheating alternative is to rely on the fact that Java sorts are stable... you can do:

Arrays.sort(arr, Comparator.comparingInt(value -> value.isEmpty() ? 0 : 1));
return arr;

which puts empty values first.

Or, as a duplicated solution, you can:

 return Stream.of(arr)
          .sorted(Comparator.comparingInt(value -> value.isEmpty() ? 0 : 1))
          .toArray(sz -> new String[sz]);


Cleaning up calculator program I recently started learning and programming Java based on a series of beginner videos created by a Youtuber (TheNewBoston), who had a tutorial on a calculator. So based on everything else I've previously learned, I have tried to mix it in to create a bit more advanced one.  If you are able to minimize and clean up the code, please give a brief explanation.  I'd like some tips as well.

import java.util.Scanner;

class apples{
    public static void main(String args[]){
        Scanner checker = new Scanner(System.in);
        int selection = 0;
        double fnum, snum, answer;

        System.out.println("Calculator Program Initialized");
        System.out.println("Please select your math operator");
        System.out.println("Enter 1 for Addition");
        System.out.println("Enter 2 for Subraction");

        selection = checker.nextInt();

        if (selection == 1){
            System.out.println("Enter your first number here");
            fnum = checker.nextDouble();
            System.out.println("Enter your second number here");
            snum = checker.nextDouble();
            answer = fnum+snum;
            System.out.println("Answer: " + answer);
            System.out.println("Thank you for using my calculator");
        }else{
            System.out.println("Enter your first number here");
            fnum = checker.nextDouble();
            System.out.println("Enter your second number here");
            snum = checker.nextDouble();
            answer = fnum-snum;
            System.out.println("Answer: " + answer);
            System.out.println("Thank you for using my calculator");
        }
    }
}

class apples{

Naming your class something like apples is always wrong. Java class names start with a Capital letter and are written in CamelCase. Thus, regardless of whether the name itself is good, it should be spelled Apples, not apples.

Also, you need to add a javadoc comment to the class that explains exactly what it does, when to or not use it, and all that.

About the name itself, now. Is Apples really a good summary of what this class is? Looking at your dialogue, probably not; maybe BasicInteractiveCommandlineCalculator would be a better fit? In any case, change the name to something that actually has to do with the class's purpose.

public static void main(String args[]){

Again, no javadoc. Javadoc for a main method should include information about what arguments it takes or how to invoke it. Pretend you were calling it from the command line, exactly what arguments are you passing it? If it doesn't take any arguments, say no args, but at least say something.

Scanner checker = new Scanner(System.in);

Here, checker isn't exactly a great name for this. Call it something shorter, like in.

While your initialization of the scanner is fine, it really ought to be created in a try-with-resources, so you can easily change to use a different input stream as needed, like a file. Instead write:

try(Scanner in = new Scanner(System.in)){

System.out.println("Calculator Program Initialized");
System.out.println("Please select your math operator");
System.out.println("Enter 1 for Addition");
System.out.println("Enter 2 for Subraction");

Doesn't this seem a little wordy for a simple calculator app? You should try talking less, and just say what you mean.

Also, you shouldn't use System.out for permanent output statements. Instead, add another declaration to your try statement. That way, you can easily change the desired stream later, with only minimal modification:

try(Scanner in = new Scanner(System.in); PrintStream out = System.out){

Now, replace the dialog with a much shorter one:

out.println("Enter operation (+ or -):");

Match the input then with in.next(String regex), so it will just wait until there is a valid input.

Handling the input should be way easier, too. Using java 7, we can switch on a string value:

String op = in.next("[+-]");

// ...

switch(op){
case "+": // ...
case "-": // ...
}

If you don't have java 7, then you should still allow the user to input this way, you just need to use if statements instead.

Now, the main problem, though, is in the way it handles the following dialog. Not only is some of the dialog itself duplicated, confusing data with logic, but in the way it computes the answer.

Refactoring the dialog out, and simplifying it, we get much, much neater code:

out.println("x=");
fnum = in.nextDouble();
out.println("y=");
snum = in.nextDouble();

switch(op){
case "+": answer = fnum + snum; break;
case "-": answer = fnum - snum; break;
default: answer = Double.NaN;
}

out.println("x" + op + "y=" + result);

This here is only the most basic level of revisions that ought to be made to the code. An application or command that attempts to do this would be expected to support all standard mathematical operations, and to be able to parse a complete expression passed to it as a string.

I once wrote a program like that, that supported all java operators and every function from java.util.Math. My approach was to use a runtime compiler janino to compile as java source code an expression given to it. (It probably would have been simpler to just use syntax trees though; configuring everything was a nightmare.)


Look back during iterating array I have an array (or string) to parse and I want to examine each element sequentially. This is a perfect case for the iterators / consumers that Rust offers.

The problem arises when I have to access previously parsed data during iteration. For example, I need to subtract the item that was 5 places before the current item.

The best way I know to achieve this using idiomatic Rust is a sliding window Vec that keeps track of the items previously seen, all of which happens inside a fold operation.

A working code extract looks like the following:

fn main() {
   let string = "111222023333"; // the string to parse
   let str_iter = string.trim_left().trim_right().chars();
   let mut last = 1 as usize;
   let mut digit = 0 as usize;
   let mut window: Vec<usize> = Vec::new();
   let target = 5;
   str_iter.fold(1, |mut val, c| {
      digit = c.to_digit(10).unwrap() as usize;
      window.push(digit);
      if window.len() > target
        {last = window.remove(0);}
      if digit == 0
        {val -= last;}
      val
   });
}

I don't really like this as I have to use a Vec to store the values of size target; I could just use indexes like I would in C/C++ but that again feels a bit wrong and not idiomatic at all.

Any ideas on how to idiomatic achieve this without using an additional Vec?

.trim_left().trim_right() is just .trim().
You might as well inline str_iter, since you only use it in one place. At least move it closer to the use-point.
Folds aren't the idiomatic loop primitive in Rust. In this case it would be much nicer to just use for.

This improves compiler analysis, and you find out that digit's initial assignment isn't getting read. You don't need digit outside the loop at all, and thus it doesn't even need to be mutable.

You're using too many type annotations. The one as usize after to_digit is enough, and even if you did need to annotate the others you should use let last: usize = 1 or let last = 1usize. Similarly, let the Vec's type parameter be inferred.
Your brace formatting is not Rusty.
Instead of dealing with window, just run two zipped offset parallel iterators. Indices work fine, too, and aren't unidiomatic: clearer, efficient code is always idiomatic.


Convert list of UTC to Current TimeZone I found myself with a list of UTC timestamps and wanted to convert them into the local time zone.  Using this web post as a starting point, I modified the script to grab a text file containing UTC times (one per line) and loop over each one outputting the results in a nice two column format.

Is there a more efficient method for accomplishing this?  I've seen people mention the .NET method $UTC.ToLocalTime() but I didn't find details on that, so I went with my frankenstein script.

$UTCTime = GC "C:\Scripts\UTC.txt"
$results = '' | Select UTCTime,PST
Foreach ($newtime in $UTCTime){
$strCurrentTimeZone = (Get-WmiObject win32_timezone).StandardName
$TZ = [System.TimeZoneInfo]::FindSystemTimeZoneById($strCurrentTimeZone)
$LocalTime = [System.TimeZoneInfo]::ConvertTimeFromUtc($newtime, $TZ)
$results.UTCTime = $newtime
$results.PST = $LocalTime
 $results
}

Input is: (and powershell might be flexible enough to handle various input formatting, more testing is needed to determine this)

4/19/2015 4:10:03 AM
4/19/2015 9:10:03 AM

Output is:

UTCTime               PST
-------               ---
4/19/2015 4:10:03 AM  4/18/2015 9:10:03 PM
4/19/2015 9:10:03 AM  4/19/2015 2:10:03 AM

This is how I would do it. First, we need a function to convert a UTC string to a DateTime since one doesn't already exist, as you already found out. The function that you found looks a bit complicated to me. I did a search for ".net utc to local time", and I found this: https://stackoverflow.com/questions/179940/convert-utc-gmt-time-to-local-time

Don't forget that .NET code, such as C#, can be translated easily into PowerShell. This is what I came up with, based on the top-rated answer:

function UtcToDateTime($dateStr)
{
    $date = [DateTime]::Parse($dateStr)
    [DateTime]::SpecifyKind($date, [DateTimeKind]::Utc)
}

The result is returned in the last line of that function because SpecifyKind returns a result.

Now we can use that function to transform our UTC date strings into DateTime objects:

$utcTimes = gc C:\Scripts\UTC.txt

$dates = $utcTimes | % { UtcToDateTime $_ }

The % is short for foreach. In English: "for each date in $dates, convert it to a DateTime using our function."

Since you want a two-column output, an easy way to do that is to make a custom objects with two the desired two fields:

$results = $dates |
    % { [pscustomobject]@{UTCTime = $_.ToString(); LocalTime = $_.ToLocalTime().ToString() } }

The @{...} is a hash table. So what that code says is that for each date, convert it to a hash table, and then convert that hash table into a pscustomobject. (That's just an easy way to make custom objects.)

And now we return our results:

$results

Which looks like the following (note that days and months are switched in my country):

UTCTime                  LocalTime
-------                  ---------
19/04/2015 4:10:03 AM    19/04/2015 2:10:03 PM
19/03/2015 9:10:03 AM    19/03/2015 7:10:03 PM

Here's the full script:

function UtcToDateTime($dateStr)
{
    $date = [DateTime]::Parse($dateStr)
    [DateTime]::SpecifyKind($date, [DateTimeKind]::Utc)
}

$utcTimes = gc C:\Scripts\UTC.txt

$dates = $utcTimes | % { UtcToDateTime $_ }

$results = $dates |
    % { [pscustomobject]@{UTCTime = $_.ToString(); LocalTime = $_.ToLocalTime().ToString() } }

$results


Perl hash mapping SVG elements to content elements Is there a more reliable way of getting the content elements for each SVG element?

#-----------------------------------------------------------------------
# Get Element content from SVG specification from:
# http://www.w3.org/TR/2011/REC-SVG11-20110816/single-page.html
# Philip R Brenan at gmail dot com, Appa Apps Ltd, 2013
#-----------------------------------------------------------------------
use feature ":5.16";
use warnings FATAL => qw(all);
use strict;
use Carp;
use Data::Dump qw(dump);

use HTML::TreeBuilder;

my $f = "svgSpecification.html";
open(my $F, "<:encoding(UTF-8)", $f) or die "Cannot open $f for unicode input";

my $t = HTML::TreeBuilder->new->parse_file($F);
my @t;

#-----------------------------------------------------------------------
# Extract relevant nodes
#-----------------------------------------------------------------------

sub list
 {my ($t, $d) = @_; # Element, stack
  if (ref($t))
   {push @$d, $t->tag;
    for($t->content_list)
     {list($_, $d);
     }
    pop @$d;
   }
  elsif (@$d >= 5)
   {my $T = join("_", @$d) ."=". ($t =~ s/[\x{2018}\x{2019}]//gr);
    if
     ($T =~ /html_body_div_div_dl_dd_ul_li_span_a_span/ or
      $T =~ /html_body_div_div_dl_dt/                   or
      $T =~ /html_body_div_div_div_span/ or
      $T =~ /html_body_div_div_dl_dd_ul_li_a_span/ # Linear gradient
     )

     {push @t, $T;
     }
   }
 }

list($t, []);

#-----------------------------------------------------------------------
# Compile content list
#-----------------------------------------------------------------------

my $c;

if (1)
 {my $s = 0;
  my $e;
  for(@t)
   {if ($s == 0 and /html_body_div_div_div_span=([\w_:-]+)/)
     {$e = $1;
      $s = 1;
     }
    elsif ($s == 1 and /html_body_div_div_dl_dt=Content model:/)
     {$s = 2;
     }
    elsif ($s == 2)
     {if (/(?:html_body_div_div_dl_dd_ul_li_span_a_span|html_body_div_div_dl_dd_ul_li_a_span)=([\w_:-]+)/)
       {push @{$c->{$e}}, $1;
       }
      elsif (/html_body_div_div_dl_dt=Attributes:/)
       {$s = 0; $e = undef;
       }
     }
   }
 }

say dump($c);

First a general code review, not specific to your “is there a more reliable way”. Later, I will show a superiour solution. But first, a lengthy rant.

I see you have a very unique coding style. This is not good: Every piece of code has two audiences: The computer, and a maintainer. Everybody can write code a computer understands, but writing for humans is an art. Do the maintainer a favour and write understandable, easy code – it could be you! In fact, your coding style slightly reminds me of the horrors of the “perlish coding style” which is neither perlish, nor recommendable, although completely logical. See here for an example. Do not imitate! Your single-space indent isn't recommendable either.

You have a knack for single letter variable names. Heck, you even have a comment explaining what each variable means: my ($t, $d) = @_; # Element, stack. This is completely and utterly silly, just name them my ($element, $stack) already. Code ought to be self-documenting.

Otherwise, comments explaining what you are doing is suspiciously absent.

When opening a file, always include the reason for failure $! in the error message, or use automatic error handling with the autodie pragma.

UTF-8 is not the default encoding for HTML documents. If your document is guaranteed to be encoded in UTF-8, then opening the file with such a layer is the correct thing. Otherwise, you can pass the filename to the parse_file method. You should check if the return value of parse_file is defined. If not, you'll find a reason for failure in $!.

You can shorten the :encoding(UTF-8) layer as :uft8. Technically, they are slightly different. Most importantly, the latter is more relaxed.

I see how you used a recursive solution for your misnomed list sub, which traverses the document. You should comment this more thoroughly – it took me a moment to realize that if (ref $t), then it is a tag, possibly with childs, otherwise a text node.

You have two stacks: @t and $d. Why do you carry one around as an argument, but not the other? Your code is actually quite hard to grok, because different branches in the tree append elements to the same $d arrayref. If I understand your code correctly, then a tree like

   html
    / \
head   body
       / \
    div   div
    / \   /|\
  h1   p p p p
           |
          em

would produce the flattening $d = [qw/html head body div h1 p div p p em p/].

[type type type] Now I see I don't understand the code correctly – you always pop the last element of the stack after traversing into childs. As each child also pops one element of the stack, each node removes itself. I think this is utterly silly, and it would be better to pass each recursion a copy of the stack. Otherwise, comment this shit! Recursion has the advantage that it is easier to reason about code. You loose this advantage when you use mutable variables.

Another problem is that your search is too wide. First, you want the HTML element. Then only the body. Then all divs. Then another one. Then a definition  list or another div. However, you search the whole tree. This is wasteful.

You have a magic number in that code: 5. Which is wrong, because your regexes can only match starting at positions 6 tags deep! Oh, and all those regexes could be combined into one, which would be more efficient. You can also anchor the match at the beginning of the string with the ^ or \A operators, which lets the regex fail faster. You also might want to assert that your path ends with a = (or, if you heed the advice of the next paragraph, with the end of the string $ or \z).

You combine the element path and the text value by concatenating them with an = in between. Later, you use captures to fiddle that value out again. I would rather use an two-element arrayref.

But why even have the @t stack? If you look closely, you'll see that there is a lot repeated code, e.g. regexes in the else path of list, and the if(1) later. You could just do the processing right there, and save that memory.

Why is there an if(1)? If you, for some reason, want to enter a new scope, just use a plain block:

my $x = 1; { my $x = 2 } say $x; # 1

This is Ok on the statement level. On the expression level, you can use a do {BLOCK} to include more complex calculations. This evaluates to the result of the last statement in the block.

Here is a refactoring of your code. It tries to fix many issues mentioned above.

# tested, works
use strict; use warnings; use 5.016;
use autodie;
use Data::Dump;
use HTML::TreeBuilder;

open my $fh, "<:utf8", "svgSpecification.html";
my $tree_builder = HTML::TreeBuilder->new;
my $document = $tree_builder->parse_file($fh) or die $!;

my (%elements, $element);
my $state = 0;
# state = 0: want element
# state = 1: want dt "Content model"
# state = 2: want value

find($document, {
  body => { div => { div => {
    div => { span => sub {
      return unless $state == 0;
      $element = shift->as_trimmed_text =~ s/\pP//gr;
      $state++;
    }},
    dl => {
      dt => sub {
        my $content = shift->as_trimmed_text;
        if ($state == 1 and $content =~ /Content model:/) {
          $state++;
        } elsif ($content =~ /Attributes:/) {
          $state = 0;
        }
      },
      dd => { ul => { li => {
        span => { a => { span => sub {
          return unless $state == 2;
          push @{ $elements{$element} }, shift->as_trimmed_text =~ s/\pP//gr;
        }}},
        a => { span => sub {
          return unless $state == 2;
          push @{ $elements{$element} }, shift->as_trimmed_text =~ s/\pP//gr;
        }},
      }}},
    },
  }}},
});

dd \%elements;

sub find {
  my ($tag, $path) = @_;
  for my $child ($tag->content_list) {
    next unless ref $child; # skip text contents
    next unless my $nextpath = $path->{$child->tag};
    if (ref($nextpath) eq 'CODE') {
      $nextpath->($child);
    } else {
      find($child, $nextpath);
    }
  }
}

As you see, the actual recursion logic is decoupled from your data extraction. The find function is guided by a hashes-of-hashes data structure, which keeps the search space minimal. The formerly implicit state machine is now documented and quite explicit. No unneccessary regexes are used.

One problem with my solution is that the data structure is extremely verbose. I used unusual formatting to battle insanely deep intendation, and it does have a slightly DSL-esque feeling. However, automatic formatting will destroy this.

Of course, such recursion is absolutely silly in this day and age. Modules like Mojo::DOM and Web::Query allow us to use CSS selectors, which makes this a breeze.

# tested, works
use strict; use warnings; use 5.016;
use Data::Dump;
use File::Slurp;
use Mojo::DOM;

my $contents = read_file "svgSpecification.html", { binmode => ":utf8" };
my $dom = Mojo::DOM->new($contents) or die "Can't parse input";

my %elems;

for my $summary ($dom->find('.element-summary')->each) {

  my $name = $summary->at('.element-summary-name')->all_text;
  $name =~ s/[\pP\s]//g; # remove all punctuation and spaces

  # Getting the correct section is tricky, as it is only identified by text.
  # We get around this by filtering manually for the correct text,
  # then continuing with the next sibling node
  my ($child_headline) = grep { $_->all_text =~ /Content model/ } $summary->find('dt')->each;
  next unless $child_headline;

  my @childs = map { $_->all_text =~ s/[\pP\s]//gr } $child_headline->next->find('.element-name')->each;
  push @{ $elems{$name} }, @childs;
}

dd \%elems;

Both of these solutions have the same speed. The 2nd solution will also put elements without childs into the hash, while the first solution ignores them. My scripts were tested with the SVG 1.1 spec on a single page.


On form submission, upload an image if there is one, then start update event What is the expected behavior?

If there is a file in the state (file != null), then startUploadImage(file) should be executed, followed by startUpdateEvent(eventId, state).
If there is no file (file === null) then only the startUpdateEvent(eventId, state) should be executed.

What is the problem I am encountering right now?

Currently I am struggling with writing a good piece of code, especially when it comes to conditional promises. In the code below you can see that I use some if / else (nested) methodology which leads to tremendously long code.

onSubmit = event => {
        event.preventDefault();
        if (
          (this.state,
          eventNameRegex.test(this.state.event_name) &&
            eventDescriptionRegex.test(this.state.event_description) &&
            priceRegex.test(this.state.ticket_price) &&
            eventDateRegex.test(this.state.event_date))
        ) {
          // check if image changed or not then continue
          this.props.handleLoading(true);

          if (this.state.file != null) {
            startUploadImage(this.state.file).then(response => {
              this.setState({ event_image: response.data });
              return this.props
                .startUpdateEvent(this.props.event.id, this.state)
                .then(() => {
                  this.props.handleLoading(false);
                  this.props.history.push("/events");
                })
                .catch(error => {
                  this.props.handleLoading(false);
                  this.setState({
                    error: error.status,
                    errorMessage: error.data
                  });
                });
            });
          } else {
            return this.props
              .startUpdateEvent(this.props.event.id, this.state)
              .then(() => {
                this.props.handleLoading(false);
                this.props.history.push("/events");
              })
              .catch(error => {
                console.log("error");
                this.props.handleLoading(false);
                this.setState({
                  error: error.status,
                  errorMessage: error.data
                });
              });
          }
        } else {
          this.setState({
            error: 400,
            errorMessage:
              "Please fill in all required fields with correct data. Please try again."
          });
        }
      };

I know nothing of react.js, so please forgive me if I missed something, but in a JS way I'd do it that way :

onSubmit = event => {
        event.preventDefault();
        //start with a dedicated part for testing values
        let fieldError="";
        if(!eventNameRegex.test(this.state.event_name))fieldError.="Name not correct.";
        if(!eventDescriptionRegex.test(this.state.event_description))fieldError.="Description not correct.";
        if(!priceRegex.test(this.state.ticket_price))fieldError.="Price not correct.";
        if(!eventDateRegex.test(this.state.event_date))fieldError.="Date not correct.";
        if(fieldError!=""){
          this.setState({
            error: 400,
            errorMessage:
              fieldError."Please fill in all required fields with correct data. Please try again."
          });
          return false;
        }
        // check if image changed or not then continue
        this.props.handleLoading(true);

        if (this.state.file != null) {
          startUploadImage(this.state.file).then(response => {
            this.setState({ event_image: response.data });
            return myUpdateEvent(this);
          });
        }
        else return myUpdateEvent(this);
      };

//when in doubt use a function ! here's hoping react.js can deal with it
function myUpdateEvent(myEvent){
  return myEvent.props.startUpdateEvent(myEvent.props.event.id, myEvent.state)
    .then(() => {
      myEvent.props.handleLoading(false);
      myEvent.props.history.push("/events");
    })
    .catch(error => {
      console.log("error");
      myEvent.props.handleLoading(false);
      myEvent.setState({
        error: error.status,
        errorMessage: error.data
      });
    });
}

Not tested but you get the gist.


Ubuntu kernel update and removal automation script This is my first bigger than a few lines bash script.

It automates kernel updating and removing on Ubuntu through ukuu (Ubuntu kernel update utility).

I'm not yet familiar with all the bells and whistles of GNU utilities and want to strive for clarity. The only thing I know now is grep -P and everything seems like a nail if you have a hammer. Also this is mostly stitched from examples on StackExchange.

What would you change to make the code more readable or perhaps better in some other ways?

#!/bin/bash

updateInstalledKernels () { # TODO (low priority) modify to not double-count kernels which have two "Installed" records in ukuu --list
    while read I
    do
        INSTALLED_KERNELS=( "${INSTALLED_KERNELS[@]}" "$I" )
    done < <(printf "$AVAILABLE_KERNELS" | grep -oP 'v\d+\.\d+((\.\d+)|(-\w+))*(?=\s+[0-9.]*\s+(Installed|Running)\s*?\z)')
}
containsElement () { # usage: containsElement string element1 element2 ...
  local e match="$1"
  shift
  for e; do [[ "$e" == "$match" ]] && return 0; done
  return 1
}

FALLBACK_KERNELS=( 'v4.15.0-33.36' 'v4.15.0-30.32' ) # array of kernels for fallback
echo "fallback kernels: ${FALLBACK_KERNELS[@]}"
# TODO variable which chooses the number of latest kernels to keep

AVAILABLE_KERNELS=$(ukuu --list)
if ! echo "$AVAILABLE_KERNELS" | grep -oPzq 'Available Kernels.*?\n=+\n\K.*?(Installed|Running).*?\n';
then
    ukuu --install-latest
else
    LATEST_KERNEL=$(echo "$AVAILABLE_KERNELS" | grep -oPz 'Available Kernels.*?\n=+\n\Kv.+?(?=\s.*?\n)')
    echo "Latest kernel ${LATEST_KERNEL} installed. Removing old kernels apart from ${FALLBACK_KERNELS[@]}."
    declare -a INSTALLED_KERNELS
    updateInstalledKernels
    echo "Installed kernels: ${INSTALLED_KERNELS[@]}."
    for INSTALLED_KERNEL in "${INSTALLED_KERNELS[@]}"
    do
            echo "$INSTALLED_KERNEL"
        if ! containsElement "$INSTALLED_KERNEL" "${FALLBACK_KERNELS[@]}"
        then
            if ! echo "$INSTALLED_KERNEL" | grep -q "$LATEST_KERNEL"
            then
                echo "${INSTALLED_KERNEL} is not fallback, nor latest. Removing it..."
                ukuu --remove "$INSTALLED_KERNEL"
            fi
        fi
    done <<< "$INSTALLED_KERNELS"
fi

The only thing I know now is grep -P and everything seems like a nail if you have a hammer.

I hope that's an exaggeration. Otherwise, I don't know how you will understand a review ;-)

Use more here-strings

The script uses here-strings in only one place,
when there are more places where it would be good to use.
For example, instead of this:

LATEST_KERNEL=$(echo "$AVAILABLE_KERNELS" | grep -oPz 'Available Kernels.*?\n=+\n\Kv.+?(?=\s.*?\n)')

This would be better, and eliminate an unnecessary echo call:

LATEST_KERNEL=$(grep -oPz 'Available Kernels.*?\n=+\n\Kv.+?(?=\s.*?\n)' <<< "$AVAILABLE_KERNELS")

Eliminate unnecessary condition

The inner if here can be eliminated:

if ! containsElement "$INSTALLED_KERNEL" "${FALLBACK_KERNELS[@]}"
then
    if ! echo "$INSTALLED_KERNEL" | grep -q "$LATEST_KERNEL"
    then
        echo "${INSTALLED_KERNEL} is not fallback, nor latest. Removing it..."
        ukuu --remove "$INSTALLED_KERNEL"
    fi
fi

You could add "$INSTALLED_KERNEL" to the parameter list of the call to containsElement. Not only the code will be simpler, but it will also eliminate a grep call. (And the echo with the grep, which should have been a here-string anyway, as explained earlier.)

Understand the flags of commands

This call looks strange: grep -oPzq ....
Because of the -q.
Thanks to the -q, the command will produce no output.
Which makes the -o flag unnecessary.
I suggest to review in man grep all the flags you're using,
understand what they do,
and consistently eliminate unnecessary flags.

Naming variables

I and e are too short, they don't help understanding their purpose, and therefore the code.

All capital letters are traditionally used for system variables.
For all local variables in shell scripts,
it's better to use lowercase names to avoid any confusion.

Appending to an array

This statement appends a value to the INSTALLED_KERNELS array:

INSTALLED_KERNELS=( "${INSTALLED_KERNELS[@]}" "$I" )

A simpler way to achieve the same:

INSTALLED_KERNELS+=("$I")

Declare important global variables and constants at the top

Some of the variables are crucial for the behavior of the script,
for example FALLBACK_KERNELS.
What if later you want to change the fallback kernels?
You have to read through the script to find the declaration in the middle.
It would be a lot easier if this was declared at the top,
so you would not need to re-read and re-understand what's going on just to make a simple change.


JavaScript toggle form elements Fellow JS devs, I have come across a fairly simple question which has puzzled me. When the user submits a form I disable all inputs and submit button changes to a loading animation (fa-spinner from fontawesome). Once the form submit is complete then I restore the previous state, and enable everything back.

My question comes to making this code more concise and deliberate in a simple toggle function:

function toggleFormElements(toggle, elArr) {
  if (toggle == 'disable') {
    elArr.btn.innerHTML = '<i class="fas fa-spinner fa-spin"></i>';
    elArr.btn.setAttribute('disabled', 'disabled');

    // inputs
    elArr.fields.forEach((field, i) => {
      field.setAttribute('disabled', 'disabled');
    });
  } else if (toggle == 'enable') {
    elArr.btn.innerHTML = 'Submit';
    elArr.btn.removeAttribute('disabled');

    // inputs
    elArr.fields.forEach((field, i) => {
      field.removeAttribute('disabled');
    });
  }
}

Would it be possible to make this code more readable or simply shorter?

That's not a toggle

Toggle has a very specific meaning and comes with the benefit of not having to supply the new state when called. Your function is not a toggle and should be called something along the lines of  setFormDisableState(formElements, disable = true) or setFormState(formElements, disable)

Argument order

If not using a default parameter use the right to left assignment rule when defining argument order. Eg assignment is right to left for a = b. Your function is assigning a state to elements in the object elArr, the elArr should be to the left of the toggle.

Danger! implied type

elArr is not an array. That is most defiantly a bad name as it implies the incorrect type.

The shortest form.

For all attributes defined in the DOM set the values directly. You only use the attribute functions for attributes not defined by the DOM. Using the longer version to do anything in code adds unneeded noise.

Keep the source DRY

Don't Repeat Yourself (code). The field iterators elArr.fields.forEach are almost identical, with only the state changing.

You have two if statements. You don't need to do the second test. If not disabling then must be enabling.

Alternatives
function toggleFormState(formDesc) { // Desc is short for description
  const newState = ! formDesc.btn.disabled;
  formDesc.btn.innerHTML = newState ? '' : "Submit";
  formDesc.btn.disabled = newState;
  for (const element of formDesc.fields) { element.disabled = newState }
}

Or
function setFormState(formDesc, disable) {
  formDesc.btn.innerHTML = disable ? '' : "Submit";
  formDesc.btn.disabled = disable;
  for (const element of formDesc.fields) { element.disabled = disable }
}


Alerting users with a pop-up window I'm relatively new at programming in general including Python. But I've tried to create a program in Python that alerts users through a pop up window to perform an action every 2 hours.

import time
import ctypes

count = 0

print "This break message was sent on "+time.ctime()
messageBox = ctypes.windll.user32.MessageBoxA
returnValue = messageBox(None, "TextAskingToDoSomething", "WindowTitle", 0x40 | 0x4)
if returnValue == 6:
    messageBox = ctypes.windll.user32.MessageBoxA
    returnValue = messageBox(None, "TextRespondingToAnswerYes", "SWindowTitle", 0x40 | 0x0)
elif returnValue == 7:
    messageBox = ctypes.windll.user32.MessageBoxA
    returnValue = messageBox(None, "TextRespondingToAnswerNo", "WindowTitle", 0x40 | 0x0)

while (count < 4):
    time.sleep(60*60*2)
    print "This break message was sent on"+time.ctime()
    messageBox = ctypes.windll.user32.MessageBoxA
    returnValue = messageBox(None, "TextAskingToDoSomething", "WindowTitle", 0x40 | 0x4)
    if returnValue == 6:
        messageBox = ctypes.windll.user32.MessageBoxA
        returnValue = messageBox(None, "TextRespondingToAnswerYes", "WindowTitle", 0x40 | 0x0)
    elif returnValue == 7:
        messageBox = ctypes.windll.user32.MessageBoxA
        returnValue = messageBox(None, "TextRespondingToAnswerNo", "WindowTitle", 0x40 | 0x0)
    count = count + 1

time.localtime()

It essentially calls a message box with a title and a text in it and if the user clicks one button then another one follow up message box appears. If the user clicks on the other button, then another follow up message appears. This process is repeated every 2 hours for 5 times.

How could I improve this code to be shortened? Could I use a function somehow? Or what would be a way to shorten it and avoid me repeating myself too much (if that is the case)?

You could change count = count + 1 to count += 1 besides that, do you need to repeat messageBox = ctypes.windll.user32.MessageBoxA so much? If it doesn't change, couldn't you just have it once? You could also add variables to shorten repeated lines like making a variable called resetMsgBox and have it be: resetMsgBox = ctypes.windll.user32.MessageBoxA so it's easier to read by having messageBox = resetMsgBox every line instead, I suppose.


Iterator to generate all the words (of a given words) that are one change away I'm trying to create an iterator, that will generate all the words that are one change away.

For example, if my input is "sam", I should get the following words:

"aam", "bam", ..."ram","tam",...."zam","sbm","scm",..."szm","saa","sab",..."san","sao",..."saz".

Notice that there is no "sam" in the output, as it is 0 changes away.

The following function, it(), is a simple function that does this and prints the output:

public class oneChangeAway implements Iterator {
String s;

oneChangeAway(String s)
{
    this.s =s;
}
@Override
public boolean hasNext() {
      return false;
}

@Override
public Object next() {
    return null;
}

@Override
public void remove() {

}

public void it() {
    int len = s.length();
    String temp ;
    for (int i = 0; i < len; i++)
    {
        char[] char_s = s.toCharArray();
        for(char c= 'a'; c<='z'; c++)
        {
            char_s[i] = c;
            temp = new String(char_s);
            if ( temp.equals(s)) continue;
            System.out.println(temp);
        }
    }
}
}

However, now I want an iterator to do this. This is what I came up with:

public class oneChangeAway implements Iterator {
String s;

int l;
int i ;
char c ;
char[] c_s;
oneChangeAway(String s)
{
    this.s =s;
    l =s.length();
    i = 0;
    c = 'a';
    c_s = s.toCharArray();
}
@Override
public boolean hasNext() {
    if ( i == l)
    return false;

    return true;
}

@Override
public Object next() {
    String temp;
    if ( this.hasNext())
    {
        c_s[i] = c;
        c++;
        temp = new String(c_s);
        if( c == ('z' + 1))
        {
            c = 'a';
            c_s[i] = s.toCharArray()[i];
            i++;
        }
        if (temp.equals(s)) return this.next();
        return temp;
    }

    return null;
}

@Override
public void remove() {

}

public void it() {
    int len = s.length();
    String temp ;
    for (int i = 0; i < len; i++)
    {
        char[] char_s = s.toCharArray();
        for(char c= 'a'; c<='z'; c++)
        {
            char_s[i] = c;
            temp = new String(char_s);
            if ( temp.equals(s)) continue;
            System.out.println(temp);
        }
    }
}
}

The following main function calls it:

public class App {
public static void main(String args[])
{

    oneChangeAway ia = new oneChangeAway("sam");

    //ia.it();

    while (ia.hasNext())
    {
        System.out.println(ia.next());
    }
}
}

This seems to work and the output is as intended. However, am I doing it the 'right' way?

am I doing it the 'right' way?

Please excuse the rude answer but: No.

general critic

Java is a class based object oriented language. So you should solve the problem in an object oriented way. This does not nessesarrily mean that your approach needs more classes and objects, but it does mean that you should apply the single responsibility pattern and find better separation of concerns.

Eg.: The responsibility of an Iterator is to give access to individual elements of a collection or stream like data structure. But the creation of this elements is not.

detailed critic

formal issues

naming

The way you choose the names in you approach has some problems.

First of all you should stick to the Java Naming Conventions In particular class names should always start with an upper case letter and the first part in a method name should be a verb.

Your variables should have readable names taken from the problem domain. You have one letter abreviations. It is quite certain that you don't remember the meaning of that one letter variables yourself when you read this in 2 month...

magic numbers

In your approach you use magic numbers, even if they are chars actually. You should rather use constants with proper names like LETTER_WITH_LOWEST_ASCII_VALUE instead of 'a'.

returning literal null explicitly

You should never return a literal null reference explicitly unless it is a valid part of the result space.

Especially you should not indicate an error by returning null. Use exceptions instead. In your case the next() method should throw  NoSuchElementException instead of returning null.

single line blocks

If you omit the curly braces for single line blocks then at least proper indent this line.

explicit use of literal true/false

In your hasNext() method you explicitly return the literal  true/false values. This introduces the possibility to change the methods logic by accident. Why didn't you simply return the result of the compare instead of wrapping it into an if statement?

use of continue

continue and break are the poor mans goto. In 20+ years of coding in Java I never ever used a continue or a break (other than inside a switch  where break is inevitable).
So instead of placing a continue think of how you could write your code without it, which is very simple in your case.

raw types

The Iterator interface supports a generics parameter but your implementation does not use it. Therefore  your next() method is declared to return a Object and the caller of your code must explicitly cast the value to a String relying on you that you don't return something else.

broken OO principles

separation of concerns / single responsibility

As already mentioned the creation of the result collection is out of scope of an Iterators responsibility.

Therefore you should have a separate class or a method in your "main app" that creates some Collection or a Java8-Stream containing the result elements. Then there would be no need to write your own Iterator implementation at all.

other missed best practices

single abstraction layer

single abstraction layer means that a method either calls other methods or does some "low level" operations, but not both. (only loops, and conditionals are allowed in both method types).

You method next() does both: it calls hasNext() and the does some operations to create the next element. The code inside the if block which does the "low level operations" should be moved to a separate method so that hasNext() only calls other methods.


Validating integer or string input I'm at uni and working on an assignment in c# that takes some user input (from they keyboard via console).

The input validation required:

numbers must be within range (range will vary depending on which menu they're in)
strings must not be empty/null
string must be no longer than 30 characters in length

So I've chosen to write just one method (will be a static method of a 'validator' class) that can handle validation for both and am unsure if this is a good idea - having a method that essentially does more than one thing.

The method:

public static bool isValid(string input, string type, int min = 0, int max = 0)
  {
     if (String.IsNullOrEmpty(input))
        return false;
     else
     {
        switch (type.ToLower())
        {
           case "integer":
              int i;
              if (Int32.TryParse(input, out i))
                 return ((i >= min) && (i <= max));
              else
                 return false;
           case "string":
              return input.Length < charLimit; // charLimit defined as 30 above
           default:
              return false;
        }
     }
  }

And a sample call:

if(isValid(userInput, "integer", 0, 5)
{
   // do something awesome
}

I've run some tests to check the edge cases and passing in empty strings etc and it passed them, but for some reason this feels kind of wrong.

Should I separate string and integer validation into separate methods?

You are clearly violating the Single Responsibility Principle (SRP) because the method is doing two things. You should use two methods where each validates one type only.

You will see also that for this task you gain the advantage that the methods will be smaller and easier to read and understand.

A small side note: In .NET methods are named using PascalCase,  so isValid should be IsValid.

Don't omit braces {} although they might be optional. By always using them your code will become less error prone.

Another quick remark about the method in question. You have defined optional parameters min = 0 and max = 0. I would expect if parameters are optional, that they could return true for a bool method. Using this method without specifying min and max will result in false which is unexpected.

Integrating the said points into extension methods would look like so

public static bool IsInRange(this int value, int min, int max)
{
    return min <= value && value <= max;
}

public static bool HasValidLength(this string value, int maximumLength)
{
    if (value == null) { throw new ArgumentNullException("value"); }

    return value.Length < maximumLength;
}

which could then be called through this methods

public static bool IsValidInteger(this string value, int min = int.MinValue, int max = int.MaxValue)
{
   int v = 0;
   return int.TryParse(value, out v) && v.IsInRange(min, max);
}

public static bool IsValidString(this string value, int maximumLength = 30)
{
    return value != null && value.HasValidLength(maximumLength);
}

like e.g

if(userInput.IsValidInteger(0, 5))
{
   // do something awesome
}