Makefile for C++ OpenGL with GLFW and glad This is a makefile I wrote to compile a simple OpenGL test project which is going to use the libraries GLFW and the C library glad.c.

CC = gcc
CXX = g++

COMPILE_FLAGS = -Wall -ggdb -O3
LINK_FLAGS = -lglfw3 -lopengl32 -lglu32 -lgdi32

glfw = d:/external/glfw-3.1
glfw_inc = $(glfw)/include
glfw_lib = $(glfw)/lib64

glad = d:/external/glad-c
glad_inc = $(glad)/include

INCLUDES = -I$(glfw_inc) -I$(glad_inc)
LIBRARIES = -L$(glfw_lib)

cpp_files = main.cpp
objects = $(cpp_files:.cpp=.o)
headers =

all: main.exe

main.exe: $(objects) glad.o
        $(CXX) $(LIBRARIES) -o main.exe $(objects) glad.o $(LINK_FLAGS)

$(objects): %.o: %.cpp $(headers) makefile
        $(CXX) $(COMPILE_FLAGS) $(INCLUDES) -c -o $@ $<

glad.o: glad.c
        $(CC) $(COMPILE_FLAGS) $(INCLUDES) -c -o glad.o glad.c

Since this is basically the first makefile I wrote, I could surely use some criticism as to what is good style and what isn't. I also wonder if there is a more elegant way to handle the fact that glad is actually a C library and has to be compiled with gcc instead of g++, but g++ is used for everything else.

First of all, good for you for learning how to write a proper makefile.  Few people are any good at it, and it's a really fundamental tool.  Even when nice tools such as CMake are used, it's very useful to understand how to write one from scratch.

With that said, here are some things that may help you improve your makefile:

Prefer Makefile to makefile

The names are equivalent, but since you're using GNU Make, you may as well follow its conventions, and one convention is to name it Makefile instead of makefile.  Either will work, but quoting from the GNU Make man page:

  We recommend 'Makefile' because it appears prominently near the beginning of a directory listing, right near other important files such as 'README'.

Use standard variable names

There are standard names for some of the variables you're using.  For example, use CFLAGS instead of COMPILE_FLAGS and CXXFLAGS which is the same thing, but for invocations of g++ (or more accurately invocations of $(CXX)).  Similarly, use LDFLAGS instead of LINK_FLAGS.

Add INCLUDES to CFLAGS

It's good practice to use variables like INCLUDES to keep things tidy, but it would be better if you added that to CFLAGS:

CFLAGS += $(INCLUDES)

The reason for doing that is in the next point.

Use the implicit rules

GNU Make already has an implicit rule for creating .o files from .c files.  That rule looks like this:

 %.o : %.c
         $(CC) -c $(CFLAGS) $(CPPFLAGS) $< -o $@

The result of this is that if you have defined the flags correctly, you can omit an explicit rule and rely instead on the implicit rule.  In this particular Makefile, it means that you can entirely omit the rule for glad.o.  As long as glad.c is in the current directory, and main.cpp is in the current directory, the built-in rules will understand how to create both glad.o using gcc and main.o using g++.

To expand this a bit, here's what happens when I run make -d to get it to emit more detailed debugging information:

  Considering target file 'main.exe'.
    Considering target file 'main.o'.
     Looking for an implicit rule for 'main.o'.
     Trying pattern rule with stem 'main'.
     Trying implicit prerequisite 'main.c'.
     Trying pattern rule with stem 'main'.
     Trying implicit prerequisite 'main.cc'.
     Trying pattern rule with stem 'main'.
     Trying implicit prerequisite 'main.C'.
     Trying pattern rule with stem 'main'.
     Trying implicit prerequisite 'main.cpp'.
     Found an implicit rule for 'main.o'.
      Considering target file 'main.cpp'.

As you can see, after it has identified a needed target (main.o) it starts looking for implicit rules to create that target.  It finds one eventually because main.cpp exists.  To see the full list in order on your machine, create an empty subdirectory, navigate to it and execute make -d foo.o.  Since foo.o won't exist (it's an empty subdirectory) it will attempt to find an implicit rule that will allow it to create that file and with the -d option, it will tell you all of the things it's trying.  On my machine, that generates 734 lines of output, so there's really quite a number of implicit rules.  There are rules to create object files from C and C++, but also from Fortran, Pascal, Modula, assembly language, and for trying to check out a .o file from various version control systems.

Add LIBRARIES to LDFLAGS

Your main rule (pardon the pun) does this:

$(CXX) $(LIBRARIES) -o main.exe $(objects) glad.o $(LINK_FLAGS)

There are a number of changes I'd make to that line, but one is to consolidate $(LIBRARIES) and $(LINK_FLAGS) into a single variable with a standard name, which is LDFLAGS.

Use predefined variables

Try to avoid repeating hard-coded names in the production rules.  For example, the rule for main.exe could be much simplified to this:

$(CXX) -o $@ $^ $(LDFLAGS)

Add a clean psuedotarget

It's often useful (not least when testing makefiles!) to provide a clean psuedotarget to remove all of the files that were generated, providing a "clean slate" from which to start.  It might be written like this:

.PHONY : clean
clean :
        -rm main.exe $(objects) glad.o

This also makes it less likely that you'll want to have the makefile itself as a dependency, which makes the set of rules cleaner.  This would work even if the first line were omitted, unless there were actually a file named clean.  To tell the makefile that it's not a real file, we use the line .PHONY : clean.

Add glad.o to objects

If you add glad.o to the objects list, you can simplify the rules.

Consider isolating TARGET

For Windows, executables typically end in .exe but not so for most other platforms.  For that reason you could either have a specific variable for the executable extension or keep the target name in a variable:

TARGET = main.exe

Putting it all together

If we take all of these suggestions and apply them, the resulting files looks like this:

CC = gcc
CXX = g++

INCLUDES = -I$(glfw_inc) -I$(glad_inc)
LIBRARIES = -L$(glfw_lib)

glfw = d:/external/glfw-3.1
glfw_inc = $(glfw)/include
glfw_lib = $(glfw)/lib64

glad = d:/external/glad-c
glad_inc = $(glad)/include

CFLAGS = -Wall -ggdb -O3 $(INCLUDES)
CXXFLAGS = -Wall -ggdb -O3 $(INCLUDES)
LDFLAGS = $(LIBRARIES) -lglfw3 -lopengl32 -lglu32 -lgdi32

TARGET = main.exe
cpp_files = main.cpp
objects = $(cpp_files:.cpp=.o) glad.o
headers =

all: $(TARGET)

$(TARGET): $(objects)
        $(CXX) -o $@ $^ $(LDFLAGS)

.PHONY : clean
clean :
        -rm $(TARGET) $(objects)


Signed integer-to-ascii x86_64 assembler macro I have implemented an algorithm which converts a signed integer to its ASCII equivalent string with the corresponding sign as a first byte. I have tested it on several numbers and it worked correctly. But as a newbie I might have not noticed some logic mistakes or flaws.

I am asking for any ideas how to make this more efficient and elegant.

;;;;MACRO SECTION//////////////////////////////////////////////////////////////

%macro Int2ascii 2                  ;1 - number, 2 - string

    mov     r12, %2                 ;move ADDRESS of string

    mov     eax, dword[%1]
    cmp     eax, 0
    jge     %%placePlus             ;check either number is negative or positive

    %%placeMinus:
        mov     byte [r12], "-"     ;fill the first (0th) byte with the sign
        mov     rdi, -1             ;remember the sign
        ;neg     eax                ;negate number and forgot the sign (deprecated: see EDIT 1)
        jmp     %%skipPlacePlus

    %%placePlus:
        mov     byte [r12], "+"
        mov     rdi, 1               ;remember the sign

    %%skipPlacePlus:

    xor     rcx, rcx                ;digit counter here (used implicitely in %%fillLoop)
    mov     ebx, 10

    %%divideLoop:
        cdq                         ;eax -> edx:eax
        idiv    ebx                 ;signed division, reminder is also signed
        imul    rdx, rdi            ;"unsign" the current reminder
        ;div     ebx                ;(deprecated: see EDIT 1)
        push    rdx                 ;push the reminder to stack

        inc     rcx                 ;increment digit counter

        cmp     eax, 0
        jne     %%divideLoop

    mov     rdi, 1                  ;note that the first (0th) byte was number sign (+/-)

    %%fillLoop:
        pop     rax                     ;pop back reminders

        add     al, "0"                 ;ascii code for digit
        mov     byte [r12 + rdi], al    ;fill string
        inc     rdi
        loop    %%fillLoop

    mov     byte [r12 + rdi], 0x00      ;terminate string

%endmacro

Example of output

As @TobySpeight suggested I have tested the above algorithm on numbers: -1, 0, 1, 9, 10, 0x7fffffff, 0x80000000. Output is (gdb session):

(gdb) x/s &sNegOne
0x600377:   "-1"
(gdb) x/s &sZero
0x600368:   "+0"
(gdb) x/s &sPosOne
0x600386:   "+1"
(gdb) x/s &sNine
0x600395:   "+9"
(gdb) x/s &sTen
0x6003a4:   "+10"
(gdb) x/s &sNumber1
0x6003b3:   "+2147483647"
(gdb) x/s &sNumber2
0x6003c2:   "-2147483648"

I have realized that trying to consider only unsigned positive numbers leads to double-word overflow for initially valid double-word number 0x80000000 but invalid after taking absolute value 0x7fffffff + 1 which is no longer signed double-word number. I have overcome it by signed division. The problem here is for signed division reminders are also signed and no more applicable for storing as digits without additional processing which is just multiplying by -1 or 1 depending on sign.

The trick to not counting -0x80000000 as a negative number is to use unsigned division.  IDIV is signed, DIV is unsigned.
One option to consider: If you fill the buffer from the end and output the pointer to the start of the data, then you don't need to relocate things.  Admittedly, this makes it harder to use in some cases.
You might be better using DI (instead of R12) to hold your output address, and STOSB to store a character there.  It not only writes the character, but increments DI.


LeetCode 8: String to Integer (atoi) I'm posting a solution for LeetCode's "String to Integer (atoi)". If you'd like to review, please do. Thank you!
Problem

Implement atoi which converts a string to an integer. The function first discards as many whitespace characters as necessary until the first non-whitespace character is found. Then, starting from this character takes an optional initial plus or minus sign followed by as many numerical digits as possible, and interprets them as a numerical value. The string can contain additional characters after those that form the integral number, which are ignored and have no effect on the behavior of this function. If the first sequence of non-whitespace characters in str is not a valid integral number, or if no such sequence exists because either str is empty or it contains only whitespace characters, no conversion is performed. If no valid conversion could be performed, a zero value is returned.

Note:

Only the space character ' ' is considered a whitespace character.
Assume we are dealing with an environment that could only store integers within the 32-bit signed integer range: [−231,  231 − 1]. If the numerical value is out of the range of representable values, 231 − 1 or −231 is returned.

Example 1:
Input: str = "42"
Output: 42

Example 2:
Input: str = "   -42"
Output: -42
Explanation: The first non-whitespace character is '-', which is the minus sign. Then take as many numerical digits as possible, which gets 42.

Example 3:
Input: str = "4193 with words"
Output: 4193
Explanation: Conversion stops at digit '3' as the next character is not a numerical digit.

Example 4:
Input: str = "words and 987"
Output: 0
Explanation: The first non-whitespace character is 'w', which is not a numerical digit or a +/- sign. Therefore no valid conversion could be performed.

Example 5:
Input: str = "-91283472332"
Output: -2147483648
Explanation: The number "-91283472332" is out of the range of a 32-bit signed integer. Thefore INT_MIN (−231) is returned.

Code
from typing import List
import collections
import itertools
import functools
import math
import string
import random
import bisect
import re
import operator
import heapq
import queue

from queue import PriorityQueue
from itertools import combinations, permutations
from functools import lru_cache
from collections import defaultdict
from collections import OrderedDict
from collections import deque
from collections import Counter

class Solution:
    def myAtoi(self, s):
        s = re.findall(r'^\s*[+-]?\d+', s)
        try:
            MAX, MIN = 2147483647, -2147483648
            res = int(''.join(s))
            if res > MAX:
                return MAX
            if res < MIN:
                return MIN
            return res
        except:
            return 0

if __name__ == "__main__":
    print(Solution().myAtoi("   -42"))

References:

LeetCode 8. String to Integer (atoi)

Leetcode atoi (string to integer)

Nice solution, it's compact and simple to understand. There is little to improve, few suggestions:

Imports: there are a lot of imports, maybe leftovers of previous attempts.
Try Except block: should be around the code that can cause the exception.

Applying the suggestions:
import re

class Solution:
    def myAtoi(self, s: str) -> int:
        MAX, MIN = 2147483647, -2147483648
        s = re.findall(r'^\s*[+-]?\d+', s)
        try:
            res = int(''.join(s))
        except:
            return 0
        if res > MAX:
            return MAX
        if res < MIN:
            return MIN
        return res

Performance
Runtime: 36 ms, faster than 51.56% of Python3 online submissions
Memory Usage: 14.1 MB, less than 32.27% of Python3 online submissions

Regex makes the code compact but is not the fastest approach. Faster solutions iterate through the string character by character.


Eloquent methods to calculate the average rent of apartments I am working on a Laravel 5.1 website where I am counting the average rent of apartments in a specific area. Besides of that I want to get the average rent by amount of rooms in the apartment. The functionality itself isn't anything that hard, but then I want to be able to get the average rent for apartments with e.g. two rooms by a variable.

How would you be able to do this in a cleaner way? I am going to have attributes for up to about 10 rooms, that would mean 20 new methods. Have to be a better way to do this?

public function average_rent($rooms, $timespan = '6 months', $fromDate = null)
{
    if ($fromDate === null) $fromDate = date('Y-m-d');

    $query = DB::select(
        "SELECT COUNT(id) as amt, SUM(rent) AS total_rent
          FROM " . env('DB_PREFIX', '') . "apartments
          WHERE
           rooms = " . $rooms . " AND rent > 0 AND
           formatted_address LIKE '% " . $this->query_params . ", Sverige%' AND
           created_at BETWEEN '" . date('Y-m-d', strtotime('-' . $timespan)) . "' AND '" . $fromDate . "'");

    $avgRent = null;
    $avgAmount = $query[0]->amt;
    if ($avgAmount >= 10) $avgRent = round($query[0]->total_rent / $avgAmount, 0) . ' kr';

    return [$avgRent, $avgAmount];
}

public function getOneRoomAvgRentAttribute()
{
    return $this->average_rent(1)[0];
}

public function getTwoRoomAvgRentAttribute()
{
    return $this->average_rent(2)[0];
}

public function getOneRoomAmtAttribute()
{
    return $this->average_rent(1)[1];
}

public function getTwoRoomAmtAttribute()
{
    return $this->average_rent(2)[1];
}

I am going to have attributes for up to about 10 rooms, that would mean 20 new methods. Have to be a better way to do this?

The only reason you have all these methods in the first place is because it's unclear how to use the average_rent method. If you would just document it properly, you can remove all the other methods that don't actually contain any logic themselves.

I'm unsure what the difference between amount and rent is, but it may make sense to have two methods: getAmount($roomCount) and getAverageRent($roomCount). If both are needed, you could also just query once and save the result in private fields. What you definitely do not want is what you have now, where you add a method for each amount of rooms you expect. That's what method arguments are for.

Security

You are vulnerable to SQL injection. Never put variables into a query directly, always use prepared statements.

Naming

don't shorten variable names, it makes code harder to read. amt should be amount, etc.
getOneRoomAmtAttribute is really hard to read. Someone not familiar with your code will have no idea what it will do. It also draws too much focus to irrelevant words (eg attribute), and less focus to the important parts (amt). Something like getAmount($roomCount) would be better.
it's hard to understand the difference between avgAmount and avgRent from their names. Average amount of what? Money? What's the difference to rent then?
either always use snake_case or always camelCase, but don't mix them.


Swift: arrayToTree() where array contains int and nil I am learning tree on Leetcode. Need to prepare the testing data.

It is easy to convert the array to an organized node, where its elements are integers.

such as [3, 9, 20, 15, 7]

Here is my code:

extension Array where Element == Int{

func arrayToTree() -> TreeNode{
    var nodes = [TreeNode]()
    for num in 0..<self.count{
         nodes.append(TreeNode(self[num]))
    }

    var i = 0
    repeat{
        nodes[i].left = nodes[2 * i + 1]
        if self.count > 2 * i + 2 {
             nodes[i].right = nodes[2 * i + 2]
        }
        i+=1
    }while i < (self.count)/2

    return nodes.first!
  }
}

When the last level contains some nils , such as [3, 9, 20, nil, nil, 15, 7]

Here is the code:

extension Array where Element == Int?{

    func arrayToTree() -> TreeNode?{

        guard self.count > 0 else{
            return nil
        }
        var nodes = [TreeNode?]()
        for num in 0..<self.count{
            if let num = self[num]{
                nodes.append(TreeNode(num))
            }
            else{
                nodes.append(nil)
            }
        }
        var i = 0
        repeat {
            nodes[i]?.left = nodes[2 * i + 1]
            if self.count > 2 * i + 2 {
                nodes[i]?.right = nodes[2 * i + 2]
            }
            i += 1
        } while i < (self.count) / 2

        return nodes.first!
    }

}

How can I refactor this?

Such as combine them together using Swift Generic with some Protocol.

Simplifying func arrayToTree()

This

   var nodes = [TreeNode?]()
   for num in 0..<self.count{
       if let num = self[num]{
           nodes.append(TreeNode(num))
       }
       else{
           nodes.append(nil)
       }
   }

creates a new array by mapping each element in self (an optional Int) to a new element (an optional TreeNode). That can be simplified to

let nodes = self.map { $0.map { TreeNode($0) } }

where the outer Array.map maps the given array to a new array, and the inner Optional.map maps an optional Int to an optional TreeNode.

The

   var i = 0
   repeat {
       // ...
       i += 1
   } while i < (self.count) / 2

loop can be simplified to

for i in 0..<self.count/2 {
    // ...
}

The forced unwrapping

   return nodes.first!

cannot crash – the nodes array cannot be empty at this point. I would still suggest to avoid it since later code changes might break the logic. It also makes it easier for future maintainers of the code to verify its correctness.

Actually the preceding code just results in an empty nodes array if the given list is empty. Therefore we can remove the initial guard and replace it by

guard let first = nodes.first else {
    return nil
}
return first

at the end of the method. This can be further shortened to

return nodes.first.flatMap { $0 }

Putting it together, the function would look like this:

func arrayToTree() -> TreeNode? {

    let nodes = self.map { $0.map { TreeNode($0) } }
    for i in 0..<self.count/2 {
        nodes[i]?.left = nodes[2 * i + 1]
        if self.count > 2 * i + 2 {
            nodes[i]?.right = nodes[2 * i + 2]
        }
    }

    return nodes.first.flatMap { $0 }
}

An alternative implementation

What you have is a way to create a TreeNode, and that is what initializers methods are for. Therefore I would put the code in a

public convenience init?(values: [Int?])

of the TreeNode class instead of an Array extension method. The usage would be

let list = [3, 9, 20, nil, nil, 15, 7]
if let tree = TreeNode(values: list) {
    // ...
}

And the task calls for a recursive implementation:

public convenience init?(values: [Int?], offset: Int = 0) {
    guard offset < values.count, let value = values[offset] else {
        return nil
    }
    self.init(value)
    self.left = TreeNode(values: values, offset: 2 * offset + 1)
    self.right = TreeNode(values: values, offset: 2 * offset + 2)
}

Making it generic

With the above changes it is now easy to replace Int by an arbitrary value type:

public class TreeNode<T> {
    public var val: T
    public var left: TreeNode?
    public var right: TreeNode?
    public init(_ val: T) {
        self.val = val
        self.left = nil
        self.right = nil
    }

    public convenience init?(values: [T?], offset: Int = 0) {
        guard offset < values.count, let value = values[offset] else {
            return nil
        }
        self.init(value)
        self.left = TreeNode(values: values, offset: 2 * offset + 1)
        self.right = TreeNode(values: values, offset: 2 * offset + 2)
    }
}


Using a factory pattern many times This is my first question on this website and I hope it goes well.

I have created a Kotlin factory pattern and I am using it very frequently. the code I use it consists of functions inside a class. This is not a static class however, it does not mutate any field inside this class.

When I look at this code, I can tell there is a pattern, so I thought I could improve it. That's why I am asking this question, for ideas & thoughts on how to improve the structure. The code is the following

class Client() {

    fun dispatchFollowEvent(followerID: Int, destination: Server, callback: (success: Any?) -> Unit) {
        val packet: FollowUserPacket = this.factory.makePacket(PacketType.FOLLOW_USER) as FollowUserPacket
        packet.payload = FollowUserPacket.FollowUserPayload(this, followerID)
        sendToServer(packet, this, destination, callback)
    }

    fun dispatchRegisterEvent(destination: Server) {
        val packet: RegistrationPacket = this.factory.makePacket(PacketType.REGISTRATION) as RegistrationPacket
        packet.payload = RegistrationPacket.RegistrationPayload(this)
        sendToServer(packet, this, destination)
    }

    fun dispatchUploadEvent(image: String, destination: Server) {
        val packet: UploadImagePacket = this.factory.makePacket(PacketType.UPLOAD_IMAGE) as UploadImagePacket
        packet.payload = UploadImagePacket.UploadImagePayload(this, image)
        sendToServer(packet, this, destination)
    }

    fun dispatchListUsersEvent(destination: Server, callback: (usersIDs: Any?) -> Unit) {
        val packet: ListUsersPacket = this.factory.makePacket(PacketType.LIST_USER_IDS) as ListUsersPacket
        packet.payload = ListUsersPacket.ListUsersPayload(this)
        sendToServer(packet, this, destination, callback)
    }

    fun dispatchGetFollowRequestsEvent(destination: Server, callback: (requests: Any?) -> Unit) {
        val packet: GetFollowRequestsPacket = this.factory.makePacket(PacketType.GET_FOLLOW_REQUESTS) as GetFollowRequestsPacket
        packet.payload = GetFollowRequestsPacket.GetFollowRequestsPayload(this)
        sendToServer(packet, this, destination,  callback)
    }
}

The class is simplified just so that it can be easier to read. I'm sure that all of you can see a pattern in this code, but it's beyond my knowledge to know how to make it more structured.

Do not worry about the functionality, the focus of this question is about improving the structure.

The factory code

class PacketFactory : Serializable {

    @Throws(Exception::class)
    fun makePacket(type: PacketType): Packet {
        return when (type) {
            PacketType.REGISTRATION -> RegistrationPacket()
            PacketType.UPLOAD_IMAGE -> UploadImagePacket()
            PacketType.LIST_USER_IDS -> ListUsersPacket()
            PacketType.FOLLOW_USER -> FollowUserPacket()
            PacketType.GET_FOLLOW_REQUESTS -> GetFollowRequestsPacket()
            else -> throw Exception("Type is not provided for the factory!")
        }
    }

}

The sendToServer function

fun sendToServer(
        payload: Packet,
        sender: Client,
        receiver: Server,
        callback: ((data: Any?) -> Unit)? = null
): Unit {
    // Code that sends payload to receiver and then invokes the callback
}

First of all, a client-server relationship is kind of a 1:1 relation. That means that the client should probably have a connection to a server (or, if abstracted, a server) to talk with. Currently you are using a server parameter, but I would argue that this should be a field. This will also free one parameter of every method. Furthermore, currently it seems allowed to call just any server from the client class. First register and then call a totally different server.

Furthermore, if such a generic & relatively static parameter is present then it should probably be the first parameter. Currently the location of the server parameter seems to change per function, and that makes reading the functions harder.

The dispatchFollowEvent seems to be used to "follow a user", but that's only clear to me after reading the code. Similarly we have an upload method that seemingly uploads an image. Why not just followUser or uploadImage? It's a client, we expect it to "dispatch" and that it creates an "event" at the server.

I'm not sure why you would use a factory in above code. The makePacket code is really just an intermediate to the specific packets that you need anyway. So now you go: "packet specific method -> packet enum -> packet specific method". Sure you can do that, but why should you?

A more grievous thing is that your factory method seems to create partial packets without a payload. That means that packets are initially in an invalid state. The whole idea is that you manufacture concrete products inside a factory.

Another minor issue is the send part of the sendToServer method. A send method generally doesn't receive any data back. If you have a callback handler, why not call it call instead? You can leave the server part out of it maybe, that a client calls a server seems kind of logical.

Unfortunately I have to leave it at this, not just because it is late, but also because I don't think I've seen enough of your code / architecture to refactor the given code. It's not terrible code or anything, but it seems rather over-engineered and especially the naming suffers from it.


Processing a collection of objects, one at a time, with an asynchronous method. I currently use setInterval and a wait flag to process this collection. Is there a cleaner way?

var wait = false;
var processInterval = setInterval(function(){
    if(!wait){
        var currentVideo = videos.shift();

        if(currentVideo){
            wait = true;

            validateSongById(currentVideo.videoId, function(result){
                wait = false;
                if(result){
                    clearInterval(processInterval);
                    callback(currentVideo);
                    return;
                }
            });
        }
    }
}, 200);

You're basically there, I would say. If the validateSongById function works as  expected, it'll call its callback when it's done, and you then do it again for the next video.

function processVideos(videos, callback) {
    var results = [], i = 0;
    function processNext() {
        if(i < videos.length) {
            validateSongById(videos[i].videoId, function (result) {
                results.push(result);
                i++;
                processNext();
            });
        } else {
            callback(results); // when called, all the videos have been processed
        }
    }
    processNext(); // start the processing
}

With this, you call processVideos with the videos array and a callback. The callback is called with a new array of all the results.

Alternatively, you can attach the result to the video-object directly, so the video and the result are tied together:

function processVideos(videos, callback) {
    var i = 0;
    function processNext() {
        if(i < videos.length) {
            validateSongById(videos[i].videoId, function (result) {
                videos[i].result = result;
                i++;
                processNext();
            });
        } else {
            callback(); // when called, all the videos have been processed
        }
    }
    processNext(); // start the processing
}


Reduce function on custom data structure using Cats Semigroup This is for purely pedagogical purposes only. So, within the contrived data structure and its constraints, I'm interested in knowing how might one improve that reduce function and whether I've properly used the Semigroup typeclass correctly here.

object ComplexData {
  import cats.Semigroup
  import cats.instances.all._

  case class Element(name: String, value: Int, els: List[Element] = Nil) {

  def reduce[A : Semigroup](z: A)(f: Element => A): A = {
      Semigroup[A].combine(f(this),
        els match {
          case Nil => z
          case _ => els.map(_.reduce(z)(f)).reduce((a, b) => Semigroup[A].combine(a, b))
        })
      }
    }

    val e1 = Element("Zero", 0,
      List(
        Element("One", 1,
          List(Element("Two", 2,
            List(
             Element("Three", 3),
             Element("Four", 4))))),
        Element("Five", 5,
          List(
           Element("Six", 6),
           Element("Seven", 7),
           Element("Eight", 8,
             List(
               Element("Nine", 9),
               Element("Ten", 10)))))))

   e1.reduce(0)(_.value)
   //res0: Int = 55

   e1.reduce("")(_.name + " ")
   //res1: String = Zero One Two Three...

   e1.reduce(0)(_.els.length)
   //res2: Int = 10

   e1.reduce("")(e => e.name + " -> " + e.value + ", ")
   //res3: String = One -> 1, Two -> 2, Three -> 3, ...
 }

Specifically:

While it works, not excited by the use of view bounds given that they are long since
deprecated (attempting to use A <: Semigroup[A]in the function signature did not compile), do I really need an implicit definition here of the semigroup if I wanted to go this way?
That pattern match seems accidentally complex, even given my constraints there's probably a more elegant or at least more straightforward way to do that, yes?
If I used aMonoid[A]instead of a semigroup I could get rid of thezparameter and provide aZero[A]orEmpty[A], I think - is that the preferred way to go?

Yes, using a Monoid can simplify what you want:

def foldMonoidal[A : Monoid](f: Element => A): A =
      Monoid.combine(f(this), Monoid.combineAll(els.map(f)))
...

e1.foldMonoidal(_.value)

One issue with that though is that you lose the information of the nested structure: You may as well have just a list of (name, value).

Any nested data structure can have a fold operation defined for it, in a way that keeps knowledge of its structure. In your case, this would be:

def fold[A](f: (Element, List[A]) => A): A =
  f(this, els.map(_.fold(f)))
...
e1.fold[Int]((el, list) => el.value + list.sum)

You can see this allows more freedom, e.g. you may want to sum the value with the average of the nested elements, which you couldn't do with the monoidal solution above.

e1.fold[Double]((el, list) => el.value + list.sum / list.size)

Or for pretty-printing:

e1.fold[String]((el, strEls) => s"(${el.name} -> ${el.value}, ${strEls.mkString("[", ",", "]")}")
// (Zero -> 0, [(One -> 1, [(Two -> 2, [(Three -> 3, [],(Four -> 4, []]],(Five -> 5, [(Six -> 6, [],(Seven -> 7, [],(Eight -> 8, [(Nine -> 9, [],(Ten -> 10, []]]]


Resource locker in Qt The idea is to lock resources in C# or Java using Qt:

lock(obj){/*process with locked obj*/}`

Now I see the problem with deleting obj under lock().

resourcelocker.h

#ifndef RESOURCELOCKER_H
#define RESOURCELOCKER_H

#include <QObject>
#include <QHash>
#include <QSemaphore>

class ResourceLocker : public QObject
{
    Q_OBJECT
public:

    friend class ResourceWatcher;
    explicit ResourceLocker(QObject *parent = 0);
    ~ResourceLocker();

    bool lock();

private:
    static QHash<QObject*,QSemaphore*> resources;
    QSemaphore * sem;
    QObject * expectedParent;
    bool doubleLock;

signals:

public slots:

};

#define _LOCK(object) for (ResourceLocker locker((object)); locker.lock(); )

#endif // RESOURCELOCKER_H

resourcelocker.cpp

#include "resourcelocker.h"
#include <QMutex>
#include <QMutexLocker>
#include <QSemaphore>
#include <QDebug>

QHash<QObject* ,QSemaphore*> ResourceLocker::resources;

class ResourceWatcher: public QObject
{
public:
    explicit ResourceWatcher(QObject * parent):
        QObject(parent)
    {
        //qDebug()<<"creating watcher";
    }

    ~ResourceWatcher()
    {
        QSemaphore * sem = ResourceLocker::resources.value(parent(),NULL);
        if (sem->available()>0)
        {
            //unlocked
            ResourceLocker::resources.remove(parent());
            delete sem;
        }
        else
        {
            //locked
            ResourceLocker::resources.remove(parent());
        }
        //qDebug()<<"removing sem";

    }
};

ResourceLocker::ResourceLocker(QObject *parent) :QObject(),expectedParent(parent),doubleLock(false)
{
}

bool ResourceLocker::lock()
{
    static QMutex internalMutex;
    {
        QMutexLocker locker(&internalMutex);

        if (doubleLock)
            return false;
        doubleLock = true;
        //qDebug()<<&expectedParent;

        sem = resources.value(expectedParent,NULL);
        if (sem == NULL)
        {
            //qDebug()<<"Crearting sem";
            sem = new QSemaphore(1);
            resources.insert(expectedParent,sem);
            new ResourceWatcher(expectedParent);
        }
    }
    //qDebug()<<"acquiring";
   sem->acquire();
   return true;
}

ResourceLocker::~ResourceLocker()
{
    QMutex internalMutex;
    QMutexLocker locker(&internalMutex);
    //qDebug()<<"releasing";
    sem->release();
    if (!resources.values().contains(sem))
        delete sem;
}

Usage:

_LOCK(smth)
    {
        //prcoess with smth locked

    }

There reason languages like Java has that feature is because they don't have RAII like C++ does.

I don't quite see how that is better than simply:

// Note: Braces for scope. "mutex" could optionally be declared static, or as a class member.
{
    QMutexLocker lock(&mutex);

    /* code */
}


LINQ method that selects an item based on previous one my requirement is to implement a way to filter a collection of elements based on a value of previous one.
For example to only select a number that is larger than the previous one. I don't currently know of any LINQ method that would allow me to do such a thing, so I decided to create one.

This is a code I have so far:

        public static IEnumerable<T> WherePrevious<T>(this IEnumerable<T> collection, Func<T, T, bool> predicate)
    {
        if (collection == null) throw new ArgumentNullException(nameof(collection));
        if (predicate == null) throw new ArgumentNullException(nameof(predicate));

        T previous = default(T);
        bool firstIteration = true;

        foreach (var item in collection)
        {
            if (firstIteration)
            {
                previous = item;
                firstIteration = false;
                yield return item;
            }
            else
            {
                if (predicate(previous, item))
                {
                    yield return item;
                }

                previous = item;
            }
        }
    }

Are there any ways to improve my code in terms of performance, readability or more generalization to make?

For example I'm not sure if it's clear to the caller of the method what the predicate inputs actually are

Linq Zip came to mind in order to map the items in pairs and then apply the predicate.

public static IEnumerable<T> WherePrevious<T>(this IEnumerable<T> collection, Func<T, T, bool> predicate) {
    if (collection == null) throw new ArgumentNullException(nameof(collection));
    if (predicate == null) throw new ArgumentNullException(nameof(predicate));
    if(!collection.Any()) yield break;

    yield return collection.First();

    var items = collection
        .Zip(collection.Skip(1), (previous, item) => (previous, item))
        .Where(zip => predicate(zip.previous, zip.item))
        .Select(zip => zip.item);

    foreach(var item in items)
        yield return item;
}

The following test was used to demonstrate the example given in the original question.

[TestClass]
public class FilterTest {
    [TestMethod]
    public void WherePreviousTest() {
        //Arrange
        var numbers = new[] { 1, 5, 8, 7, 12, 8, 5 };
        var expected = new[] { 1, 5, 8, 12 };

        //Act
        var actual = numbers.WherePrevious((first, second) => second > first).ToArray();

        //Assert
        actual.ShouldAllBeEquivalentTo(expected);
    }
}

The body of the created extension method could have been done on its own, but creating the extension method allowed for a cleaner approach.

While I would say it is easier to read IMO, technically you will be enumerating the list more than once, which makes this not as efficient as your one pass through.

That then lead me to rethink my approach. After reviewing the source code for Enumerable.Zip and realizing your approach had the right idea I refactored the code to use the enumerator of the collection.

public static IEnumerable<T> WherePrevious<T>(this IEnumerable<T> collection, Func<T, T, bool> predicate) {
    if (collection == null) throw new ArgumentNullException(nameof(collection));
    if (predicate == null) throw new ArgumentNullException(nameof(predicate));
    return WherePreviousIterator(collection, predicate);
}

private static IEnumerable<T> WherePreviousIterator<T>(IEnumerable<T> collection, Func<T, T, bool> predicate) {
    using (var e = collection.GetEnumerator()) {
        if (e.MoveNext()) {
            var previous = e.Current;
            yield return previous;
            while (e.MoveNext()) {
                var item = e.Current;
                if (predicate(previous, item))
                    yield return item;
                previous = item;
            }
        }
        yield break;
    }
}

Which actually performed not that much better than the previous suggestion using Zip when run against the same test above.


Python 3 adventure game in modules I have been learning Python 3 for about a year now. I have been looking for a project to do instead of just going through tutorials and completing some Code Wars challenges. One website suggested that a text game would be something good to work on. So, I tried to come up with a story line and did a basic map design. But, as soon as I started coding I realized that I had better just try something simple first to learn how to break things up so the game could be manageable as it got bigger.

The three small files listed below are what I have so far. You should be able to just copy and paste the code, and then run it. (To run through all the possibilities/options you want to try "N", "S", "E", "e" and then "W".)

I quickly found that using modules is essential. For this simple project I am using a separate file for storage of health and wealth.

I have tried to use descriptive names for variables and functions. I think the "page_01.py" should have a more descriptive name. But, this first little project is basically an experiment for learning. I would also use comments for a bigger game. For now, I think the variable names and function names will allow you to easily figure out what is going on in the code listed below.

Here is main.py

import page_01
import storage

page_01.instructions()
print("You have " + str(storage.health) + " health points and " + str(storage.wealth) + " coins.")

action = input("What do you want to do now?")
page_01.player_move(action)

Here is page_01.py

import storage

def instructions():
  print("You can move N, S, E or W.")

def ask_player_for_move():
  react = input("\nWhat is your next move?")
  player_move(react)

def player_update():
  print("\nYour health is now: " + str(storage.health))
  print("You now have " + str(storage.wealth) + " coins.\n")

def player_move(action):
  if action == "N":
    print("You stepped on a bear trap.")
    storage.health -= 10
    player_update()
    ask_player_for_move()
  elif action == "S":
    print("You tripped over a pengiun!")
    print("But, you found a small bag of 15 coins.")
    storage.health -= 3
    storage.wealth += 15
    player_update()
    ask_player_for_move()
  elif action == "E":
    print("Err...I wouldn't go that way if I were you!")
    ask_player_for_move()
  elif action == "W":
    print("You ride off into the sunset")
    player_update()
  else:
    print("That is not a valid entry.")
    ask_player_for_move()

Here is storage.py

health = 100

wealth = 10

Style

There is an official standard Python style guide called PEP 8. This is highly recommended reading. It gives guidelines to help writing code that is both readable and consistent. The Python community tries to follow these guidelines, more or less strictly (a key aspect of PEP 8 is that it provides guidelines and not strict rules to follow blindly).

It deals with various aspects of the code style: naming conventions, indentation convention, etc.

You'll find various tools to try to check whether your code is PEP 8 compliant and if is it not, to try and fix this:

pycodestyle package (formerly known as pep8) to check you code
pep8online to check your code with an online tool
autopep8 package to fix your code automatically
Also, this is also checked by various linters: pylint, pyflakes, flake8, etc.

In your case, the major point is that indentation should be 4 space. Also, you should not have trailing whitespaces.

Modules

Splitting the code into small modules can be a good idea. However, I find page_01 to be a pretty weird name. Also, the way the code is splitted in different modules does not seem to be perfect to me. The way you use storage suggests that is should probably be a data structure (a class?).

For the time being, let's move everything back to a single file:

health = 100
wealth = 10

def instructions():
    print("You can move N, S, E or W.")

def ask_player_for_move():
    react = input("\nWhat is your next move?")
    player_move(react)

def player_update():
    print("\nYour health is now: " + str(health))
    print("You now have " + str(wealth) + " coins.\n")

def player_move(action):
    if action == "N":
        print("You stepped on a bear trap.")
        health -= 10
        player_update()
        ask_player_for_move()
    elif action == "S":
        print("You tripped over a pengiun!")
        print("But, you found a small bag of 15 coins.")
        health -= 3
        wealth += 15
        player_update()
        ask_player_for_move()
    elif action == "E":
        print("Err...I wouldn't go that way if I were you!")
        ask_player_for_move()
    elif action == "W":
        print("You ride off into the sunset")
        player_update()
    else:
        print("That is not a valid entry.")
        ask_player_for_move()

instructions()
print("You have " + str(health) + " health points and " + str(wealth) + " coins.")

action = input("What do you want to do now?")
player_move(action)

Functions

player_update does not update the player, maybe print_player would be more relevant.

Similarly, I'd expect ask_player_for_move to be a function returning the move asked to the user.

In both cases, it seems like the functions could be used in the "main" part instead of duplicating the code.

health = 100
wealth = 10

def show_intructions():
    print("You can move N, S, E or W.")

def ask_player_for_move():
    react = input("\nWhat is your next move?")
    player_move(react)

def print_player():
    print("\nYour health is now: " + str(health))
    print("You now have " + str(wealth) + " coins.\n")

def player_move(action):
    global health
    global wealth
    if action == "N":
        print("You stepped on a bear trap.")
        health -= 10
        print_player()
        ask_player_for_move()
    elif action == "S":
        print("You tripped over a pengiun!")
        print("But, you found a small bag of 15 coins.")
        health -= 3
        wealth += 15
        print_player()
        ask_player_for_move()
    elif action == "E":
        print("Err...I wouldn't go that way if I were you!")
        ask_player_for_move()
    elif action == "W":
        print("You ride off into the sunset")
        print_player()
    else:
        print("That is not a valid entry.")
        ask_player_for_move()

show_intructions()
print_player()
ask_player_for_move()

Hidden recursion

Your game loop relies on a hidden recursion: player_move calls ask_player_for_move which calls player_move. This makes things heard to follow but also will add another level of function calls in the stack. This can be an issue if you need a backtrace because you'll get something like:

  File "new_main.py", line 37, in player_move
    ask_player_for_move()
  File "new_main.py", line 12, in ask_player_for_move
    player_move(react)
  File "new_main.py", line 37, in player_move
    ask_player_for_move()
  File "new_main.py", line 12, in ask_player_for_move
    player_move(react)
  File "new_main.py", line 37, in player_move
    ask_player_for_move()
  File "new_main.py", line 12, in ask_player_for_move
    player_move(react)
  File "new_main.py", line 37, in player_move
    ask_player_for_move()
  File "new_main.py", line 12, in ask_player_for_move
    player_move(react)
  File "new_main.py", line 37, in player_move
    ask_player_for_move()
  File "new_main.py", line 12, in ask_player_for_move
    player_move(react)
  File "new_main.py", line 37, in player_move
    ask_player_for_move()
  File "new_main.py", line 12, in ask_player_for_move
    player_move(react)
  File "new_main.py", line 43, in player_move
    ask_player_for_move()
  File "new_main.py", line 12, in ask_player_for_move
    player_move(react)
  File "new_main.py", line 43, in player_move
    ask_player_for_move()
  File "new_main.py", line 11, in ask_player_for_move
    react = input("\nWhat is your next move?")
KeyboardInterrupt

but the other issue is that you could end up reaching the recursion limit.

You could write something like:

health = 100
wealth = 10
finished = False

def show_intructions():
    print("You can move N, S, E or W.")

def ask_player_for_move():
    return input("\nWhat is your next move?")

def print_player():
    print("\nYour health is now: " + str(health))
    print("You now have " + str(wealth) + " coins.\n")

def player_move(action):
    global health
    global wealth
    global finished
    if action == "N":
        print("You stepped on a bear trap.")
        health -= 10
        print_player()
    elif action == "S":
        print("You tripped over a pengiun!")
        print("But, you found a small bag of 15 coins.")
        health -= 3
        wealth += 15
        print_player()
    elif action == "E":
        print("Err...I wouldn't go that way if I were you!")
    elif action == "W":
        print("You ride off into the sunset")
        print_player()
        finished = True
    else:
        print("That is not a valid entry.")

def game():
    global finished
    show_intructions()
    print_player()
    while not finished:
        action = ask_player_for_move()
        player_move(action)

game()

Then you could move the call of game() behind an if __name__ == "__main__": guard.


Sort numbers in ascending order using Python This is Python code to sort the numbers in ascending order (It's my school task). How does it look?

It works, but I think that this can be improved.

# How many number we have to sort
num = int(input("How many figures : "))
storage = []
result = []

# Creating an array of users numbers
for i in range(1,num+1):
    a = int(input("Enter value" + str(i) + " : "))
    storage.append(a) # user enter

# Sorting the array
for m in range(len(storage)):
    b = min(storage)
    storage.remove(b)
    result.append(b) # user get
j = ' '.join(str(i) for i in result)
print(j)

You could combine your input to a single line to improve ease of use. For example,

storage = input("Enter values separated by spaces:")
storage = [int(x) for x in storage.split()]

This way you have the entire list of input and can avoid having to have the user enter the number of input, and avoid having to declare the num variable at all.

However, you probably also want to include some form of input validation or throw a meaningful error as right now if your user was to enter non-integers, your program would simply crash and output a vague error that the user would probably have a difficult time understanding.

try:
    storage = [int(x) for x in test.split()]
except ValueError:
    print("Non-integers in input!")

Alternatively, you could check if all the values are numeric and if not, have the user re-enter their input.

As for your sorting algorithm, if you don't want to use Python's implemented sort() or sorted(), you should research more efficient algorithm's such as quick sort or even implement the bubble sort that you learned.

Currently, your min_sort algorithm finds the minimum value in the list, which is O(n) then removes that element from the list (separately from the search) which is again O(n). This is extremely wasteful as you may end up searching through the entire list again and again (n times), so it would be better to use a more efficient sorting algorithm or at least recognize that we don't need to pass through the entire list twice on each iteration, just pass through the list once and keep track of the minimum value. You could do this by writing your own function like:

def find_remove_min(nums):
    """Returns the minimum number and the list without the min number"""
    if nums:
        min_index = 0
        for i in range(1, len(nums)):
            if nums[i] < nums[min_index]:
                min_index = i
        return nums[min_index], nums[:min_index] + nums[min_index+1:]

Then you could do something like,

while storage:
    min_num, storage = find_remove_min(storage)
    result.append(min_num)

which would be more readable and efficient imo.


Pascal triangle algorithm is good but fails I have created a function which returns the number that corresponds to the given row and column from the pascal triangle. I have used the formula for this:

  n! / r! * (r-n)!

The code:

def pascal(c: Int, r: Int): Int = {

    def factorial(number: Int): Int = {

      def loop(adder: Int, num: Int): Int = {
        if( num == 0) adder
        else loop(adder * num, num - 1)

      }

      loop(1,number)
    }

    factorial(r) / (factorial(c) * factorial(r-c));
}

I received 180 points out of 190 for this function because it returns an arithmetic exception for a given case (they don't give me the case). How could I improve this code and how can I make it better?  Most importantly, how can I spot the bug?

The code appears to be correct, though the explanatory formula you posted is wrong.

Your factorial function is more complex than it needs to be.  Furthermore, adder is a poor variable name, since the result is multiplicative rather than additive.  The traditional recursive definition is:

def factorial(n: Int): Int = {
    if (n == 0) 1
    else n * factorial(n - 1)
}

The main problem with your approach is that you work with large numbers in both the numerator and the denominator.  If your program is generally working, then your problem is probably arithmetic overflow.  An Int would only handle values up to 231 - 1 correctly.

Therefore, your best bet is to use an entirely different way to construct Pascal's Triangle, as suggested by @janos.


Binary to Decimal Conversion App I have created a binary to decimal calculator that needs to follow this spec:

Arrays may not be used contain the binary digits entered by the user
Determining the decimal equivalent of a particular binary digit in the sequence must be calculated using a single mathematical function, for example the natural logarithm. It's up to you to figure out which function to use.
User can enter up to 8 binary digits in one input field
User must be notified if anything other than a 0 or 1 was entered
User views the results in a single output field containing the decimal (base 10) equivalent of the binary number that was entered
User can enter a variable number of binary digits

My code is:

binaryForm = document.getElementById('formComplete');
numberInput = document.getElementById('numberInput');
numberInputButton = document.getElementById('updateNumberBtn');
binaryInput = document.getElementById('binaryInput');
decimalValue = document.getElementById('decimalOutput');

numberInputButton.addEventListener('click', function () {
    var numberInputValue = numberInput.value;
    binaryInput.setAttribute('maxLength', numberInputValue);
});

binaryInput.addEventListener('keydown', function (e) {

    if (e.keyCode !== 48 && e.keyCode !== 49 && e.keyCode !== 13 && e.keyCode !== 8) {
        e.preventDefault();
        alert('Only 0 and 1 is allowed');
    }

});

binaryForm.addEventListener('submit', function (e) {
    e.preventDefault();

    if (binaryInput.value == 0) {
        alert('Empty');
    } else {
        var userInput = binaryInput.value;
        var total = 0;
        for (var i = 0; i < userInput.length; i++) {
            var binaryDigit = parseInt(userInput[i]);
            var calcInput = (total * 2) + binaryDigit;
            total = calcInput;
        }
        decimalValue.innerHTML = total;
    }
});
body {
    background: #2e2e2e;
    color: #fff;
    font-family: Heebo, sans-serif;
}

h1,
h2,
h3,
h4 {
    font-weight: 900;
}

input {
    display: block;
    margin: 0 auto;
    width: 100%;
    max-width: 300px;
    height: 60px;
}

.form-body {
    min-height: 100vh;
    display: flex;
    flex-direction: column;
    justify-content: center;
    text-align: center;
}

#numberInput,
#binaryInput {
    margin-bottom: 1rem;
    border: 0.125rem solid rgba(255,255,255,0.25);
    background: rgba(255,255,255,0.05);
    color: #fff;
    font-size: 2rem;
    text-align: center;
    font-family: Heebo, sans-serif;
    font-weight: 300;
}

#updateNumberBtn,
#convertBtn {
    margin-bottom: 1rem;
    border: 0.125rem solid rgba(0,128,0,0.75);
    color: #fff;
    background: rgba(0,128,0,0.75);
    text-transform: uppercase;
}

#updateNumberBtn:hover,
#convertBtn:hover {
    background: rgba(0,128,0,0.25);
    cursor: pointer;
}

Binary to Decimal Calculator

  Set max number of bits (Max 12 - Default 8)
  <input id="numberInput" type="number" value="8" min="1" max="12">
  <input id="updateNumberBtn" type="button" value="Set Number of Bits">
  <form action="" id="formComplete">
      Enter a binary value
      <input id="binaryInput" type="text" size="12" maxlength="8">
      <input id="convertBtn" type="submit" value="Convert to Decimal">
  </form>

Decimal Value:

Are there areas that I could improve performance-wise or by making the app easier to operate, such as the part where I am targeting the keys that the user can input.

You should use the directive "use strict" that will place the JavaScript context into strict mode. This will throw errors for some common bad practices.

Always declare variables as const, var, or let. If you are in strict mode you will get an error if you don't.

Don't use alerts or prompts as there is no way to know if they are actually displayed (clients can turn them off) and they are very annoying.

The key event properties KeyboardEvent.keyCode and KeyboardEvent.charCode have been depreciated and you should not use them. Use KeyboardEvent.code or KeyboardEvent.key instead

Rather than filter the input via the keybpoard events, listen to the input's keyup and change events, removing bad characters automatically. Use a CSS rule to unhide a warning and a JavaScript setTimeout to hide it again
Filtering keyboard events means you need to check many keys that are valid (left, right, backspace, delete, etc...) which is just unneeded complexity.

Don't wait for the user to click "Convert to Decimal", display the output automatically. This makes it a lot friendlier to use.

JavaScript can convert binary strings to decimal for you using parseInt. The second argument is the radix (AKA base) of the number that is being parsed.

If you are just setting text (no HTML) use the elements textContent rather than innerHTML

Rewrite
The rewrite is following the points you have set-out in your question. I have not implemented how your code differs from these points.
The rewrite uses

a RegExp to test and filter the input.
a ternary expression to create the decimal value as parseInt will return NaN for empty strings. The ternary checks if the string is empty evaluating to "" or if there is a number the ternary evaluates to the decimal value.
HTMLInputElement.focus (inherited from HTMLElement) to focus the input element when loaded.

I have modified the HTML and CSS to fit the snippet window a little better.

"use strict";
const binaryInput = document.getElementById("binaryInput");
const decimalOutput = document.getElementById("decimalOutput");
const inputWarning = document.getElementById("inputWarning");
binaryInput.addEventListener('change', update);
binaryInput.addEventListener('keyup', update);
binaryInput.focus();

const WARNING_TIME = 2000; // in ms
var warningTimer;
function hidWarning() {
   inputWarning.classList.add("hideWarning");
}
function showWarning() {
    clearTimeout(warningTimer);
    warningTimer = setTimeout(hidWarning, WARNING_TIME);
    inputWarning.classList.remove("hideWarning");
}
function update() {
    var value = binaryInput.value;
    if (/[^01]/g.test(value)){
        binaryInput.value = value = value.replace(/[^01]/g, "");
        showWarning();
    }
    decimalOutput.textContent = value === "" ? "" : parseInt(value, 2);
}
body {
    background: #2e2e2e;
    color: #fff;
    font-family: Heebo, sans-serif;
}
.form-body {
    display: flex;
    flex-direction: column;
    text-align: center;
}
input {
    margin: 0 auto;
    width: 100%;
    max-width: 300px;
    border: 0.125rem solid rgba(255,255,255,0.25);
    background: rgba(255,255,255,0.05);
    color: #fff;
    font-size: 2rem;
    text-align: center;
    font-weight: 300;
}

.inputWarning { color: #F88 }
.hideWarning { display: none }

Binary to Decimal Calculator

  Enter a binary value [Only valid digits 0 and 1 permitted]
  <input id="binaryInput" type="text" size="8" maxlength="8">

Decimal Value:


PHP allow template to use HTML only I have a place where i have to include a HTML template.
The HTML is written by employees only but i dont wanna be an idiot and include it without masking|checks :)

It should allow HTML tags only without any attributes.

So no <a href... links and no <div style=... divs or w/e.

My test script:

$string
    = '

    Foo

Bar

    Baz&nbsp;Mmmpf

someting

someting more

even more

    Foo

Foo BarBaz

';

$htmlWhitelist = [
    'u',
    'i',
    'p',
    'strong',
    'ul',
    'li',
];
// replace allowed tags with placeholders
// that not get changed by htmlspecialchars()
foreach ($htmlWhitelist as $tag) {
    $string = str_replace(
        ["<{$tag}>", "</{$tag}>"],
        ["{OPEN}{$tag}{OPEN}", "{CLOSE}{$tag}{CLOSE}"],
        $string
    );
}
// htmlspecialchars() on everything
$string = htmlspecialchars($string);
// put back the allowed tags
foreach ($htmlWhitelist as $tag) {
    $string = str_replace(
        ["{OPEN}{$tag}{OPEN}", "{CLOSE}{$tag}{CLOSE}"],
        ["<{$tag}>", "</{$tag}>"],
        $string
    );
}

I cannot imagine anything could go wrong with this but would like to ask you guys if i missed someting.

I did some testing and research on this, AFAIK your script is safe.

However, you should also be aware of how you retrieve the input. For example, what if the String contained a ", followed by executing PHP code. This would be an even worse vulnerability than malicious client-side code.

This post states old versions of IE may be vulnerable if your char-set is UTF-7, which it probably isn't.

If <div style...> is entered, it will be escaped. Therefore <div> and <span> should be included.

I believe every tag can have an onload=script, (E.G <u onload="script">), so only allowing tags by themselves is a good idea. (As you are already doing)

I suggest adding div, span, b, br and many other tags to the whitelist.

Edit: Since you mentioned you use PHP to echo the output, I also suggest testing using PHP functions or variables  in the input, such as $_SERVER['REMOTE_ADDR'].

Will the output show the result of that variable, or the literal text?


Static utility functions to turn arrays into SQL statements I have an object that houses only static functions. The functions are stand alone and are not dependent on the state of the object.

I hear that static functions are a nightmare in terms of testing, an anti-pattern if you will. I get it, the state of the object could be impossible to predict.. But these are stateless.

My question is, Should I place these functions into a class of their own or am I ok by leaving them as static methods?

class Utility {

public static function arrayToInsertStatement($table, array $data) {
    $sql = "INSERT INTO %s (%s) VALUES (:%s)";
    return sprintf($sql, $table, implode(', ', array_keys($data)), implode(', :', array_keys($data)));
}

public static function arrayToSelectStatment($table, array $fields, $whereStatement) {
    $sql = "SELECT %s FROM %s %s";
    if ($whereStatement) {
        $whereStatement = sprintf(" WHERE %s", $whereStatement);
    }
    return sprintf($sql, implode(',', $fields), $table, $whereStatement);
}

public static function arrayToUpdateStatement($table, array $data, $whereStatement = null) {
    $t_sql = "UPDATE %s SET %s%s";
    $sets = self::arrayToKeyedArray($data);
    if ($whereStatement) {
        $whereStatement = sprintf(" WHERE %s", $whereStatement);
    }
    return sprintf($t_sql, $table, implode(',', $sets), $whereStatement);
}

public static function arrayToWhereStatement($data, $joinType = 'AND') {
    $joiner = sprintf(" %s ", $joinType);
    return implode($joiner, self::arrayToKeyedArray($data));
}

public static function arrayToOrderBy(array $data) {
    $o = [];
    foreach ($data as $field => $direction) {
        $directionBool = substr(strtolower($direction), 0, strlen($direction)) === substr('descending', 0, strlen($direction));
        $o[] = sprintf($field . ' %1$s', ($directionBool ? 'DESC' : 'ASC'));
    }
    return implode($o, ',');
}

public static function arrayToKeyedArray($data) {
    return array_map(function($value) {
        return sprintf('%1$s = :%1$s', $value);
    }, array_keys($data));
}

It's not so much about the state than it is about coupling. When you test a method, you want to control its dependencies; when the method under test calls a static method, you can't control that dependency and thus, the method under test is coupled with the implementation of that static method, so you might as well consider that static method inlined into the method under test, as far as testing goes.

So, you've called this class Utility. Like the Helpers and Managers of this world, a class called Utility is pretty much an incentive to become a dumping ground for just about anything that's, well, a "utility". Name things after what they're responsible for, and thank yourself later: in this case, a SqlStatementBuilderUtility would make it much clearer what the intent is, and then the class would be much less prone to grow some bubbleSort or sendMail "utility" members in the future.

Side note, the indentation is off; it's like the class-level scope doesn't exist, the class and its members are all at the same indentation level, and that doesn't look right.

I don't think static is the main concern here - indeed, these methods look like good candidates for being static. The problem is what they're being used for. It looks like it's building SQL statements out of whatever values are thrown at it, which obfuscates the actual queries your application is sending to the database server, and raises a flag about sql-injection.


Reading configuration from a file for video rendering I haven't done anything similar so I got nothing to go on. The point was to load everything from the file and I thought a struct would be a good choice to be the handler to store the information.

The loading part is, however, not really something that is to my satisfaction as I find it very primitive and illusive. I try to walk around issues as I don't really know how to handle them if anything goes awry.

    //Set Config Value from File
bool ConfigValue(std::string contents, std::string setting, unsigned int& MySettings){
    int pos1 = NULL, pos2 = NULL;
    pos1 = contents.find(setting);
    pos2 = contents.find('\n', pos1);
    if (pos1 == -1)
        return false;
    else
        pos1 += setting.length();
    contents = contents.substr(pos1, pos2 - pos1);
    for (int i = 0; i < contents.length();i++){
        if (!(contents[i] >= '0' && contents[i] <= '9' || contents[i] == ' '))
            return false;
    }
    MySettings = std::stoi(contents);
    return true;
}
//Reads ConfigFile with some safe measures if it's wrongly written for some reason
Config ConfigFile(){
start:
    video::E_DRIVER_TYPE renderer = video::EDT_DIRECT3D9;
    //Set Default if file doesn't contain them.
    Config MySettings;
    std::string line;
    fstream input("config.ini", fstream::in);

    if (input){
        //Read config file to buffer
        std::stringstream buffer;
        buffer << input.rdbuf();
        std::string contents(buffer.str());

        if (!ConfigValue(contents, "width=", MySettings.Width) || //Set Width
            !ConfigValue(contents, "height=", MySettings.Height) || //Set Height
            !ConfigValue(contents, "fullscreen=", MySettings.Fullscreen) || //Set Fullscreen
            !ConfigValue(contents, "renderer=", MySettings.renderer))  //Gets renderer value
            goto makeDefault;

        //Set renderer according to value obtained
        switch (MySettings.renderer){
        case 0: MySettings.renderer = video::EDT_DIRECT3D8;
            break;
        case 1: MySettings.renderer = video::EDT_DIRECT3D9;
            break;
        case 2: MySettings.renderer = video::EDT_OPENGL;
            break;
        default: goto makeDefault;
        }
    }
    else{
    makeDefault:
        input.close();
        input.open("config.ini", fstream::out);
        input << "//renderer options - 0: D3D8, 1: D3D9, 2: OpenGL\nwidth=640\nheight=480\nfullscreen=0\nrenderer=1"; //Set Default Resolution;
        goto start;
    }

    return MySettings;
}

Config:

struct Config{
    unsigned int renderer;
    unsigned int Width;
    unsigned int Height;
    unsigned int Fullscreen; //should be bool

};

As you can see, I use goto a lot as a kind of safety net. If anything ever goes awry (in my detection), I reset everything. However, the tests I do to make sure it's correct is something I would like reviewed.

The config in its current state is supposed to contain this:

//renderer options - 0: D3D8, 1: D3D9, 2: OpenGL
width=640
height=480
fullscreen=0
renderer=1

The first line is just an instruction for the renderer as it's kinda hard to guess it. Nothing else needs any instructions so they are normal. It's always supposed to be "WhatItIs=Value". (Worth noting is that Fullscreen is a bool, as 0 and 1 works the same I see no reason to mess with conversion).

Any suggestion on this is wanted as I really find pretty much all of this code to be extremely bad even though it works. But as I got nothing to compare to it, I can only guess.

I see a number of things that could help you improve your code.

Make sure you include all required headers

The necessary headers are not included in the question which makes it a bit harder to review than it needs to be.  The standard ones that are required appear to be these:

#include <iostream>
#include <string>
#include <fstream>
#include <sstream>

Pass file names as parameters

Rather than hardcoding a filename twice (leading to the potential error of not having them match!), pass it as a parameter instead.  This reduces the hidden linkages in the code and makes it easier to see what's happening.

Be careful with signed versus unsigned

The code currently contains this:

for (int i = 0; i < contents.length();i++){
    if (!(contents[i] >= '0' && contents[i] <= '9' || contents[i] == ' '))
        return false;
}
MySettings = std::stoi(contents);

However, on some platforms, size_t is unsigned, so the loop is comparing an unsigned and a signed number.  I'd avoid that problem by declaring i to be of type size_t.

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL.  See this answer for why and how it's useful.  Once you do that, you'll see that you have been abusing NULL and using it as an integer value instead of just a pointer.  For example, the code includes this:

int pos1 = NULL, pos2 = NULL;

In that context, both of those should be 0 because the variables are not pointers.

Use namespaces when required

The std namespace is used in some locations within the code, but not all.  In particular, in this line:

fstream input("config.ini", fstream::in);

Needed to be converted to this to compile on my machine:

std::fstream input("config.ini", std::ios::in);

Create a default constructor

In the case there is no config file, you'd like to have some default settings.  The better way to do that is to have a default constructor for Config.  I'd write it like this:

Config() : renderer{1}, Width{640}, Height{480}, Fullscreen{0} {}

Create a stream inserter

At some point, you will undoubtedly want to write the configuration file.  To do that, I'd recommend writing a stream inserter like this:

friend std::ostream& operator<<(std::ostream& out, const Config& cfg) {
    return out << "//renderer options - 0: D3D8, 1: D3D9, 2: OpenGL"
        "\nwidth=" << cfg.Width <<
        "\nheight=" << cfg.Height <<
        "\nfullscreen=" << (cfg.Fullscreen ? 1 :0 ) <<
        "\nrenderer=" << cfg.renderer << '\n';
}

Eliminate goto

The above two suggestions allow you to eliminate the start label and the associate goto by writing the else clause like this:

else{
    std::ofstream out(filename);
    out << MySettings;
}

Use standard algorithms to simplify

The code contains ConfigValue which returns a bool indicating whether the configuration was updated.  However, a more generic way to handle this might instead be to write a function to take a line and extract a name, value pair.  Here's how I'd write such a function:

std::pair<std::string, int> value(std::string line) {
    std::pair<std::string, int> val;
    // remove any comments
    auto n = line.find("//");
    if (n != std::string::npos) {
        line.erase(n);
    n = line.find('=');
    if (n != std::string::npos) {
        line[n] = ' ';  // replace '=' with space
        std::stringstream ss(line);
        ss >> val.first >> val.second;
    }
    return val;
}

Write a stream extractor

In the same spirit as writing the stream inserter, we can write a stream extractor.  Using the value function above as a member function of Config, it could be this:

friend std::istream& operator>>(std::istream& in, Config& cfg) {
    Config temp;
    std::string line;
    int fields = 0xF;
    for (std::string line; fields && std::getline(in, line); ) {
        auto pair=temp.value(line);
        if (pair.first == "width") {
            temp.Width = pair.second;
            fields &= ~0x01;
        } else if (pair.first == "height") {
            temp.Height = pair.second;
            fields &= ~0x02;
        } else if (pair.first == "renderer") {
            temp.renderer = pair.second;
            fields &= ~0x04;
        } else if (pair.first == "fullscreen") {
            temp.Fullscreen = pair.second;
            fields &= ~0x08;
        }
    }
    if (fields == 0) {
        std::swap(temp, cfg);
    } else {
        in.setstate(std::ios::failbit);
    }
    return in;
}

Note that unlike the current code, this code only updates the default if all four values were successfully read.  If you don't want that, you can just omit all references to fields in this function.

Make the object take care of itself

Since all of this code is associated with the Config struct, why not just actually make it a C++ class and have it take care of its own initialization?  For that, we could replace the ConfigFile function with a constructor:

Config(const std::string& filename)
: renderer{1}, Width{640}, Height{480}, Fullscreen{0}
{
     std::ifstream input(filename);
    input >> *this;
    if (!input) {
        std::ofstream out(filename);
        out << *this;
    }
}

Test function

I created a sample main to drive this code.  It looks like this:

int main()
{
    Config cfg("configreader.test");
    std::cout << cfg;
}

Naturally, you'll want to add more error and range checking but I hope this inspires you to rework your code into something more object-oriented.


Reporting test results as JSON and XML I have two functions now with very similar functionality, and would like to merge them into one more generic function. The only difference between those two functions is that one handles json input file and the other xml input file.
I could use if statements to achieve this, but I find it cumbersome, other suggestions would be much appreciated!

def update_json_type_report(self, testcase_id, did_any_test_fail, test_case_steps):
    print "Adding results to test case id: %s" % testcase_id

    fail_or_pass = '(/) ' # initialize test to pass

    expected_result = {
        "result":""
    }

    for i, step in enumerate(test_case_steps):
        for tests in self.parsed_report_file["suites"]:
            for test in tests["tests"]:
                tmp_result = ""
                if test["name"] == test_case_steps[i]['step']['raw'] and test["state"]:
                    if "error" in test:
                        fail_or_pass = '(x) '
                        did_any_test_fail = 3
                        tmp_result += fail_or_pass + test["error"] + '\n'
                    else:
                        # can have many tests per step, append them all in one string adding '\n' at end of each line
                        tmp_result += fail_or_pass + test["name"] + '\n'

                    break
            #import pdb; pdb.set_trace()
            expected_result['result'] = tmp_result
            if tmp_result:
                self.jira_obj.send_put('xray', 'test/%s/steps/%s' % (testcase_id, test_case_steps[i]['id']), expected_result)

    return did_any_test_fail

def update_xml_type_report(self, testcase_id, did_any_test_fail, test_case_steps):
    print "Adding results to test case id: %s" % testcase_id

    fail_or_pass = '(/) ' # initialize test to pass

    expected_result = {
        "result":""
    }

    for i, step in enumerate(test_case_steps):
        for xml_testsuite in self.parsed_report_file:
            for xml_testcase in xml_testsuite.iter('testcase'):
                tmp_result = ""
                if xml_testcase.attrib["name"] == test_case_steps[i]['step']['raw'] and "hook" not in xml_testcase.attrib["name"]:
                    if xml_testcase.find('system-err') is not None:
                        fail_or_pass = '(x) '
                        did_any_test_fail = 3
                        tmp_result += fail_or_pass + xml_testcase.find('system-err').text + '\n'
                    else:
                        # can have many tests per step, append them all in one string adding '\n' at end of each line
                        tmp_result += fail_or_pass + xml_testcase.attrib["name"] + '\n'

                    break

            expected_result['result'] = tmp_result
            if tmp_result:
                self.jira_obj.send_put('xray', 'test/%s/steps/%s' % (testcase_id, test_case_steps[i]['id']), expected_result)

    return did_any_test_fail

It looks like you need to abstract out how you get the element that contains the test cases and then how you determine errors from the nodes in the element.

for test in self.parsed_report_file["suites"]["tests"]:

and

for xml_testcase in xml_testsuite.iter('testcase'):

get you the parent element to start the loop and then you need to determine whether the items are in error. I think it would make things easier if you did some of the refinement when you get the initial set of elements rather than doing more filtering in the loops. Since in the JSON case you only care about nodes matching test["name"] == test_case_steps[i]['step']['raw'] and test["state"], do that filtering when you grab the parent. Same for the XML, can you use XPath or similar to filter down to xml_testcase.attrib["name"] == test_case_steps[i]['step']['raw'] and "hook" not in xml_testcase.attrib["name"] before you start?

Once you have all that, the Jira-specific handling of errors (self.jira_obj.send_put) can be abstracted out to its own function that accepts the id, test step id and result object.


Implementing a Trie in Python Question:

  Implement a trie with insert, search, and startsWith methods.

Source:

  https://leetcode.com/problems/implement-trie-prefix-tree/description/

The Trie object will be instantiated and called as such:

obj = Trie()
obj.insert(word)
param_2 = obj.search(word)
param_3 = obj.startsWith(prefix)

Code:
I have a TrieNode class and a Trie class. I'm not sure if I can make my code more pythonic. I ran the code on leetcode and it passed all the testcases.

class TrieNode:

    def __init__(self):
        self.children = [0]*26
        self.end = False

    def put(self, ch):
        self.children[ord(ch)-ord('a')] = TrieNode()

    def get(self, ch):
        return self.children[ord(ch)-ord('a')]

    def contains_key(self, ch):
        return self.children[ord(ch)-ord('a')] != 0

    def set_end(self):
        self.end = True

    def is_end(self):
        return self.end

class Trie:

    def __init__(self):
        """
        Initialize your data structure here.
        """
        self.root = TrieNode()

    def insert(self, word):
        """
        Inserts a word into the trie.
        :type word: str
        :rtype: void
        """
        curr = self.root
        for i in range(len(word)):
            if not curr.contains_key(word[i]):
                curr.put(word[i])
            curr = curr.get(word[i])

        curr.set_end()

    def search(self, word):
        """
        Returns if the word is in the trie.
        :type word: str
        :rtype: bool
        """
        curr = self.root
        for i in range(len(word)):
            if not curr.contains_key(word[i]):
                return False
            curr = curr.get(word[i])

        return True if curr.is_end() else False

    def startsWith(self, prefix):
        """
        Returns if there is any word in the trie that starts with the given prefix.
        :type prefix: str
        :rtype: bool
        """
        curr = self.root
        for i in range(len(prefix)):
            if not curr.contains_key(prefix[i]):
                return False
            curr = curr.get(prefix[i])

        return True

for with range and len

This is one of the most common things to be seen in python code to someone that isn't very familiar to python, however it's not Pythonic and almost always the wrong way to do it.

Firstly if you are not using the index, so there is no need to iterate base on the index. Consequently this can be replaced with a normal for over a collection, in your case, a string.

So this:

for i in range(len(word)):
    if not curr.contains_key(word[i]):
        curr.put(word[i])
    curr = curr.get(word[i])

Should become this:

for letter in word:
    if not curr.contains_key(letter):
        curr.put(letter)
    curr = curr.get(letter)

Note that the index was not necessary at all, and the code actually becomes clearer.

Even in cases where the index of the element is needed for something, you can use enumerate which gives you a tuple for each element, holding both the index and the element:

for index, letter in enumerate(word):

Which is still more readable and remains Pythonic

Keys stored in array

Even though it is reasonable to store keys in a array it is actually simpler to use a dictionary. Doing so avoids having to convert a character to value in the range 0-26 and still makes it simple to check whether it exists.

So instead of doing self.children = [0]*26 you would do self.children = {}. Most of the TrieNode methods would have to be rewritten accordingly:

def put(self, ch):
    self.children[ch] = TrieNode()

def get(self, ch):
    return self.children[ch]

def contains_key(self, ch):
    return ch in self.children

Note that they are all so small and simple that i would go as far as removing them and using curr.children directly in both in insert, search and startsWith.

i.e (insert):

curr = self.root
for letter in word:
    if letter not in curr.children:
        curr.children[letter] = TrieNode()
    curr = curr.children[letter]

While using curr.children makes sense in this simple example, it may not be the case if there is a lot more logic in TrieNode. Each case is unique in a way, and what may be fine in one scenario may be bad in another.

The same can be said about the is_end method that merely returns end. Following this last step, you could go a step further and also use end directly as there isn't much to encapsulate. On top of that only Trie uses TrieNode.

On a side note, in cases where you need a getter, you should use the @property decorator which would be far more appropriate and Pythonic.

camelCase

I know the methods stubs were provided in the problem description, however, camelCase is not the typical naming convention for python  methods. The preferred style for this is snake_case, and as such, startsWith should be renamed to starts_with.

Feel free to take a look at the related PEP, PEP 8 -- Style Guide for Python Code


Counting the triangles that can be formed from segments of given lengths Problem Statement:

  Given an unsorted array of positive integers. Find the number of
  triangles that can be formed with three different array elements as
  lengths of three sides of triangles.

  Input:  The first line of the input contains T denoting the number of testcases. First line of test case is the length of array N and
  second line of test case are its elements.

  Output: Number of possible triangles are displayed to the user.

  Constraints:

  \$1 \le T \le 200\$
  \$3 \le N \le 10^7\$
  \$1 \le \mathrm{arr}[i] \le 10^3\$

  Example:

  Input:

  2
3
3 5 4
5
6 4 9 7 8

  Output:

  1
10

Algorithm:

The efficient \$O(n^2)\$ algorithm for solving this problem has been explained here.

  Let \$a\$, \$b\$ and \$c\$ be three sides. The below condition must
  hold for a triangle (Sum of two sides is greater than the third side)
  i) \$a + b > c\$ ii) \$b + c > a\$ iii) \$a + c > b\$

  Following are steps to count triangle.

  Sort the array in non-decreasing order.
  Initialize two pointers i and j to first and second elements respectively, and initialize count of triangles as 0.
  Fix i and j and find the rightmost index k (or largest arr[k]) such that arr[i] + arr[j] > arr[k]. The number of
  triangles that can be formed with arr[i] and arr[j] as two sides
  is k – j. Add k – j to count of triangles.

  Let us consider arr[i] as \$a\$, arr[j] as \$b\$ and all
  elements between arr[j+1] and arr[k] as \$c\$. The above mentioned
  conditions (ii) and (iii) are satisfied because arr[i] < arr[j] <
  arr[k]. And we check for condition (i) when we pick k.
  Increment j to fix the second element again.

  Note that in step 3, we can use the previous value of k. The reason is simple, if we know that the value of arr[i] + arr[j-1] is
  greater than arr[k], then we can say arr[i] + arr[j] will also be
  greater than arr[k], because the array is sorted in increasing
  order.
  If j has reached end, then increment i. Initialize j as i + 1, k as i+2 and repeat the steps 3 and 4.

Code Implementation (in C++):

This is my implementation of the algorithm in C++ (using vectors instead of raw arrays):

#include <iostream>
#include <vector>
#include <algorithm>
#include <iterator>

using namespace std;
int triangles (vector<int> &V, int N);

int main () {
    int T;
    cin >> T;
    vector<int> V;
    for (int i = 0; i < T; i++)
    {
        int N;
        cin >> N;
        for (int j = 0; j < N; j++)
        {
           int temp;
           cin >> temp;
           V.push_back(temp);
        }
        cout << triangles (V, N) << endl;
        V.clear();
    }
    return 0;
}

int triangles (vector<int> &V, int N)
{
    int sum = 0;
    sort(V.begin(), V.end());
    for (int i = 0; i <= N-3; i++)
    {
        int k = i + 2;
        for (int j = i + 1; j <= N-2; j++)
        {
            while (k < N && V[k] < V[i] + V[j])
                k++;
            sum += k-j-1;
        }
    }
    return sum;
}

Problem:

The code gives the perfectly correct output for any input, however, when I submit the code on GeekForGeeks Practice, it says

  Your program took more time than expected (Time Limit Exceeded).
  Expected Time Limit < 2.656sec. Hint: Please optimize your code and submit again.

At this point, I'm not sure how to optimize my code. Any ideas? Do I necessarily need to use an array instead of a vector in order to speed up the program?

The statement V.push_back(temp); can be inefficient, as the vector V may need to be reallocated multiple times.

Use std::vector::reserve(N) to ensure sufficient space exists in the vector before reading in the data to avoid multiple reallocations.

  \$3 \le N \le 10^7\$
  \$1 \le \mathrm{arr}[i] \le 10^3\$

With \$10^7\$ pigeons and \$10^3\$ holes, many holes can have over 10,000 pigeons!  You might want to consider an array of counts indexed by side length.  No sort required; it is automatically sorted.  But your algorithm will need considerable reworking.


Microsoft logo animation I made an animation of the Microsoft logo. It's available as a CodePen doodle, also. Would anyone be so kind as to check my style and say if there is anything I could've improved? (I am sure there is.)

$(document).ready(function(){
    $(".logo").click(function(){
      $(this).toggleClass("turnOffAnimation");
      $(".brand").toggleClass("turnOffAnimation");
    });
});
body {
  background: hsl(30, 20%, 20%);
  color: #fff;
  font-family: 'Open Sans', sans-serif;
}

.boxes {
  animation: logo 4s infinite;
  position: absolute;
}

.box {
  animation: scaling 1s infinite;
  height: 50px;
  width: 50px;
}

.brand {
  animation: fadein 2s ease 3s forwards;
  display: inline-block;
  font-size: 36px;
  left: 40%;
  margin: 30px 0 0 20px;
  opacity: 0;
  position: absolute;
  top: 40%;
  z-index: 0;
}

.intro {
  text-align: center;
}

.logo {
  animation: moveLeft .5s linear 3s forwards;
  display: inline-block;
  left: 50%;
  position: absolute;
  top: 40%;
  z-index: 1;
}

.turnOffAnimation {
  animation-name: none;
}

#red {background: #f65314;}
#green {background: #7cbb00;}
#yellow {background: #ffbb00;}
#blue {background: #00a1f1;}

#animateRed {animation-delay: 3s;}
#animateGreen {animation-delay: 2s;}
#animateYellow {animation-delay: 1s;}
#animateBlue {animation-delay: 0s;}

@keyframes fadein {
  from {opacity: 0; left: 40%}
  to {opacity: 1; left: 50%;}
}

@keyframes logo {
  0%, 100% {left: 0px; top: 0px; transform: rotate(0deg)}
  25% {left: 50px; top: 0px; transform: rotate(180deg)}
  50% {left: 50px; top: 50px; transform: rotate(0deg)}
  75% {left: 0px; top: 50px; transform: rotate(-180deg)}
}

@keyframes moveLeft {
  from {left: 50%}
  to {left: 40%}
}

@keyframes scaling {
  0%, 100% {transform: scale(1)}
  50% {transform: scale(.5)}
}

}

<link href='http://fonts.googleapis.com/css?family=Open+Sans' rel='stylesheet' type='text/css'>

  Microsoft

Points of improvement:

The animation moves a bit faster than a traditional logo animation would.
When the window is too small, the brand and the boxes overlap.

There's an extraneous bracket at the end of your CSS file:

@keyframes scaling {
  0%, 100% {transform: scale(1)}
  50% {transform: scale(.5)}
}

}
^ <- Here

If you're not against one line CSS rules, you could convert the following:

.turnOffAnimation {
  animation-name: none;
}
.intro {
  text-align: center;
}


Position update loop and checking collision I'm writing a laser connection game. This is my first game on Windows Phone, and I've never used C# and XNA before.

The part of my code that could be improved is Update() (where I make my sprites move).

public void Update(GameTime gameTime)
{
    TouchPanelCapabilities touchCap = TouchPanel.GetCapabilities();
    if (touchCap.IsConnected)
    {
        TouchCollection touches = TouchPanel.GetState();
        if (touches.Count >= 1)
        {
            Vector2 PositionTouch = touches[0].Position;

            for (int i = 0; i < ListDragObj.Count(); i++)
            {
                if (ListDragObj[i].Shape.Contains((int)PositionTouch.X, (int)PositionTouch.Y))
                {
                    //Permit to avoid several Obj to moove in the same time
                    if (!OnlyOneSelected(ListDragObj))
                    {
                        ListDragObj[i].selected = true;
                        TamponPosition = ListDragObj[i].Position;
                    }
                }
                else
                {
                    ListDragObj[i].selected = false;
                }

                if (touches[touches.Count - 1].State == TouchLocationState.Released)
                {
                    if (gm.Check_Tile((int)(PositionTouch.X / 10), (int)(PositionTouch.Y / 10)))
                    {
                        ListDragObj[i].Update(PositionTouch);
                    }
                    else
                    {
                        ListDragObj[i].Update(TamponPosition);
                    }
                }
            }
        }
    }
}

I'm looking for the selected Sprite (in the List ListDragobj), and then I move it.
The last couple if/else is where I check collisions.

I was wondering how I could improve that position Update() method.

You can reduce some of the indent by checking for the negative condition and bailing out early. E.g.:

TouchPanelCapabilities touchCap = TouchPanel.GetCapabilities();
if (!touchCap.IsConnected) { return; }

TouchCollection touches = TouchPanel.GetState();
if (touches.Count == 0) { return; }

... your other code goes here ...

PositionTouch: Standard naming convention for local variables is camelCase. Also the names reads slightly weird - touchPosition would be better.
On every iteration through your ListDragObj method you check if (touches[touches.Count - 1].State == TouchLocationState.Released). You can store that in a local variable before the loop:

var touchReleased = touches[touches.Count - 1].State == TouchLocationState.Released;

for (...)
{
    ...
    if (touchReleased)

You don't actually use the index in this loop: for (int i = 0; i < ListDragObj.Count(); i++). A more idiomatic C# way would be to use a foreach in this case:

foreach (var dragObject in ListDragObj)
{
    ...
}

And replace all uses of ListDragObj[i] with dragObject.
This:

for (int i = 0; i < ListDragObj.Count(); i++)
{
    if (ListDragObj[i].Shape.Contains((int)PositionTouch.X, (int)PositionTouch.Y))
    {
        //Permit to avoid several Obj to moove in the same time
        if (!OnlyOneSelected(ListDragObj))

looks suspiciously like an O(n^2) algorithm - assuming that OnlyOneSelected iterates over the list and counts how many are selected. You should consider keeping track of how many items are selected in your class. This could be achieved by adding two methods:

void SelectedObject(DragObj dragObject) { ... }
void DeselectObject(DragObj dragObject) { ... }

which modify the selected property and adjust the selected count.
This

if (gm.Check_Tile((int)(PositionTouch.X / 10), (int)(PositionTouch.Y / 10)))

looks like it computes a tile index from the position - assuming tiles are 10x10. If this assumption is correct then you should change Check_Tile to accept a position. It should then calculate the appropriate tile index by itself. Otherwise what happens if you ever want to change your tile size from 10x10 to something else? You would need to find every place in your code where you make this implicit assumption.


Is this a correct way to utilize classes and methods in JAVA? I am currently learning JAVA. I barely have any coding experience. I wrote a program which asks a user for a sentence. It then swaps the words of the sentence (first with last, second with second to last, etc.). Program will then output the new sentence.

I know that this is not the most optimal way this task could be solved so please ignore that.
My question is: "Do I use classes and methods correctly? If not, how would I adjust them in this code? In particular, I feel my main method looks weird (the way parameters are added to the methods)."

The code compiles and works as intended. My question is more in regards to style.

I add the code, in 2 classes

main class:

package com.test;

public class ND2_Task1 {

    public static void main(String[] args) {

        ND2_Task1_Core run = new ND2_Task1_Core();

        run.getInput(); // gets userSentence
        run.getWordCount(run.getUserSentence());
        run.checkValidity(run.getWordCount());
        run.toArray(run.getUserSentence()); // transforms String into an array
        run.reverseArray(run.getSeperatedSentence());
        run.showResult(run.getSeperatedSentence());

    }

}

2nd. class:

package com.test;

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

public class ND2_Task1_Core {

    private static String userSentence;
    private static int wordCount;
    private static String[] seperatedSentence;
    private static String[] reversedSentence;

    public void getInput() {
        System.out.println(
                "Please enter a sentence.\nFirst word will be swapped with the last one.\nSecond word will be swapped with second to last\netc.");
        setUserSentence(new Scanner(System.in).nextLine());

    }

    public void getWordCount(String userSentence) {
        wordCount = 1;
        for (int i = 0; i < userSentence.length(); i++) {
            if (userSentence.charAt(i) == ' ') {
                wordCount++;
            }
        }

    }

    public void checkValidity(int wordCount) {
        if (wordCount < 2) {
            System.out.println(
                    "You have entered an insufficient number of words. Enter at least 2 words for program to swap them");
            System.out.println("Exiting now");
            System.exit(0);
        }

    }

    public void toArray(String userSentence) {
        setSeperatedSentence(userSentence.split(" "));

    }

    public void reverseArray(String[] seperatedSentence) {
        reversedSentence = new String[seperatedSentence.length];

        reversedSentence = seperatedSentence;

        for (int i = 0; i < reversedSentence.length / 2; i++) {
            String temp = reversedSentence[i];
            reversedSentence[i] = reversedSentence[reversedSentence.length - i - 1];
            reversedSentence[reversedSentence.length - 1 - i] = temp;
        }

    }

    public void showResult(String[] reversedSentence) {
        System.out.println(Arrays.toString(reversedSentence));

    }

    public String getUserSentence() {
        return userSentence;
    }

    public void setUserSentence(String userSentence) {
        ND2_Task1_Core.userSentence = userSentence;
    }

    public String[] getSeperatedSentence() {
        return seperatedSentence;
    }

    public void setSeperatedSentence(String[] seperatedSentence) {
        ND2_Task1_Core.seperatedSentence = seperatedSentence;
    }

    public int getWordCount() {
        return wordCount;
    }

    public void setWordCount(int wordCount) {
        ND2_Task1_Core.wordCount = wordCount;
    }

}
```

You seem to be under the impression that efficient code and good design are two separate things.  A good design will help you write efficient code.

In this particular case, you need to learn about separation of concern.  Specifically what is the user concerned about and what should be handled by the class?

The user basically just wants the words in the supplied string swapped.  Everything else should be handled inside the class.

In the class itself, you don't really need all those properties and helper methods.  The main purpose of the code can easily be summed in one function.  I would suggest a utility class(StringUtils?) with a public static function to swap the words and return a string.

The solution could look like this:

Main.java

import java.util.Scanner;

class Main {
  public static void main(String[] args) {
    String sentence;
    System.out.println(
        "Please enter a sentence.\nFirst word will be swapped with the last one.\nSecond word will be swapped with second to last\netc.");
    try (Scanner sc = new Scanner(System.in)) {
      sentence = sc.nextLine();
    }
    String swapped = StringUtils.swapWords(sentence);
    System.out.println(swapped);
  }
}

StringUtils.java

public class StringUtils {

  public static String swapWords(String input) {
    String[] usersWords = input.split(" ");
    if(usersWords.length < 2){
      inputError();
    }
    String[] words = new String[usersWords.length];
    int mid = (words.length / 2) + 1;
    for (int front = 0, back = words.length - 1; front < mid; ++front, --back) {
      words[front] = usersWords[back];
      words[back] = usersWords[front];
    }
    return String.join(" ", words);
  }

  private static void inputError(){
            System.out.println(
                    "You have entered an insufficient number of words. Enter at least 2 words for program to swap them");
            System.out.println("Exiting now");
            System.exit(0);
  }
}