Pattern that adds a different value to the same attribute for many classes The purpose of this question is to get some advice for improving the following code in order to make it less repetitive.

.parent-div a {
  border-bottom: 10px solid;
  .blue & {
    border-color: $blue;
  }
  .red & {
    border-color: $red;
  }
  .green & {
    border-color: $green;
  }
  .pink & {
    border-color: $pink;
  }
  .yellow & {
    border-color: $yellow;
  }
  .purple & {
    border-color: $purple;
  }
  // here comes again all the styles but for each hovered anchor
 .blue &:hover {
    border-color: darken($blue, 10%);
  }
 .red &:hover {
     border-color: darken($red, 10%);
  }
}

You can minimize your SASS code using @each and nesting for :hover

.parent-div a {
  border-bottom: 10px solid;

  @each $color in blue, red, green, yellow, pink, purple { // Iterate through colors
    .#{$color} & {
      border-color: $color;

      &:hover { // Nested inside
        border-color: darken($color, 10%);
      }
    }
  }
}

which compiles to the following output

Codepen Demo

.parent-div a {
  border-bottom: 10px solid;
}
.blue .parent-div a {
  border-color: blue;
}
.blue .parent-div a:hover {
  border-color: #0000cc;
}
.red .parent-div a {
  border-color: red;
}
.red .parent-div a:hover {
  border-color: #cc0000;
}
.green .parent-div a {
  border-color: green;
}
.green .parent-div a:hover {
  border-color: #004d00;
}
.yellow .parent-div a {
  border-color: yellow;
}
.yellow .parent-div a:hover {
  border-color: #cccc00;
}
.pink .parent-div a {
  border-color: pink;
}
.pink .parent-div a:hover {
  border-color: #ff8da1;
}
.purple .parent-div a {
  border-color: purple;
}
.purple .parent-div a:hover {
  border-color: #4d004d;
}

  Test

    Test

    Test

    Test

    Test

    Test

    Test

But the above code seems to carry little logic. I think you were looking to nest the color classes after parent element like this:

.parent-div a {
  border-bottom: 10px solid;

  @each $color in blue, red, green, yellow, pink, purple { // Iterate through colors
     &.#{$color} {
      border-color: $color;

      &:hover { // Nested inside
        border-color: darken($color, 10%);
      }
    }
  }
}

which would be logically correct and with better HTML and CSS syntax:

Codepen Demo

.parent-div a {
  border-bottom: 10px solid;
}
.parent-div a.blue {
  border-color: blue;
}
.parent-div a.blue:hover {
  border-color: #0000cc;
}
.parent-div a.red {
  border-color: red;
}
.parent-div a.red:hover {
  border-color: #cc0000;
}
.parent-div a.green {
  border-color: green;
}
.parent-div a.green:hover {
  border-color: #004d00;
}
.parent-div a.yellow {
  border-color: yellow;
}
.parent-div a.yellow:hover {
  border-color: #cccc00;
}
.parent-div a.pink {
  border-color: pink;
}
.parent-div a.pink:hover {
  border-color: #ff8da1;
}
.parent-div a.purple {
  border-color: purple;
}
.parent-div a.purple:hover {
  border-color: #4d004d;
}

  Test

  Test

  Test

  Test

  Test

  Test

  Test

  Test


Using datetime to obtain the time of the next lecture Often before the lecture starts, and occasionally in the break I like to display a timer with a countdown until the lectures starts/ resumes while playing some lofi-hip hop in the background. While the university students seems to enjoy this, I am ashamed of the code running the timer.
So my lecture schedules is as follows
Monday:    10:15 -- 12:00
Wednesday: 12:15 -- 14:00
Thursday:  08:15 -- 10:00

I am interested in finding when my next lecture is in iso format. I did this using the code below and it works fine. E.g if the current time is 16:00 on a Thursday, the output should be Monday and 10:15. I commented out the actual timer, as it is not important.

Is there a better way / cleaner python code to obtain the date and time for my next lecture?

My attempt (which works, albeit a bit ugly) is as follows:
from datetime import datetime, timedelta
from subprocess import call

# Format (day, hour, minute)
Lectures = [(0, 10, 15), (), (2, 12, 15), (3, 8, 15)]

def get_next_lecture(now=datetime.now()):

    next_lecture = now.replace(minute=15, second=0, microsecond=0)

    # monday = 0, tuesday = 1, ...
    current_day = datetime.today().weekday()
    current_hour = now.hour
    lecture_day = current_day

    correct_day = False
    while not correct_day:
        # If the day is tuesday, increment to wednesday
        if current_day == 1:
            lecture_day = current_day + 1
            lecture_hour = Lectures[lecture_day][1]
            lecture_minute = Lectures[lecture_day][2]
            now += timedelta(days=1)
            correct_day = True
            # if the day is friday, increment to monday
        elif current_day == 4:
            lecture_day = 0
            lecture_hour = Lectures[lecture_day][1]
            lecture_minute = Lectures[lecture_day][2]
            now += datetime.today() + timedelta(days=3)
            correct_day = True
        else:
            # If it is not monday or friday, I have a lecture
            # checks if the lecture is in the future, if else increment day and try again
            if now.hour < Lectures[lecture_day][1]:
                if now.minute < Lectures[lecture_day][2]:
                    lecture_hour = Lectures[lecture_day][1]
                    lecture_minute = Lectures[lecture_day][2]
                    correct_day = True
                else:
                    current_day += 1
                    now += timedelta(days=1)
            else:
                current_day += 1
                now += timedelta(days=1)

    next_lecture = now.replace(
        hour=lecture_hour, minute=lecture_minute, second=0, microsecond=0
    )

    return next_lecture

def launch_timer(time):
    call(["termdown", time.isoformat()])

if __name__ == "__main__":

    next_lecture = get_next_lecture()
    print(lecture)
    # launch_timer(next_lecture)

Your algorithm seems good, but the while loop, and lecture_hour and lecture_minute variables make your code a lot more complicated.
If we KISS then a simple algorithm is to just remove () from Lectures and iterate through it, since it is sorted.
The first lecture that is after the current time is the lecture we want.
This is nice and simple:
import datetime

LECTURES = [(0, 10, 15), (2, 12, 15), (3, 8, 15)]

def _get_next_lecture(now):
    today = (now.weekday(), now.hour, now.minute)
    for lecture in LECTURES:
        if today < lecture:
            return lecture

def get_next_lecture(now=None):
    if now is None:
        now = datetime.datetime.now()
    day, hour, minute = _get_next_lecture(now)
    return (
        now.replace(hour=hour, minute=minute, second=0, microsecond=0)
        + datetime.timedelta(day - now.weekday())
    )

From here we can see if the weekday is 4-6 then _get_next_lecture will return nothing and so will error.
This is easy to solve, we just return the first lecture with +7 days.
def _get_next_lecture(now):
    today = (now.weekday(), now.hour, now.minute)
    for lecture in LECTURES:
        if today < lecture:
            return lecture
    day, hour, minute = LECTURES[0]
    return day + 7, hour, minute

With only 3 lectures there's not much point in optimizing further. However if you have more, here is some food for thought:

You can use bisect to find where to insert into in \$O(\log n)\$ time.

You can change LECTURES into a 7 item list with the weekday as the index and the lectures as the value (always as a list). From here you just find the date using either of the above algorithms.
This would look like your Lectures. But with a list for each day.
This has either \$O(d)\$ or \$O(\log d)\$ time where \$d\$ is the maximum amount of lectures in a day.

Test code
def replace(date, changes):
    day, hour, minute = changes
    return date.replace(hour=hour, minute=minute) + datetime.timedelta(days=day)

def test(tests, bases, fn):
    for base in bases:
        date = base.replace(second=0, microsecond=0) - datetime.timedelta(days=base.weekday())
        for test, exp in tests:
            try:
                output = fn(replace(date, test))
            except Exception as e:
                print(f'❌ {test=}, {exp=}')
                print('  ', e)
                continue
            expected = replace(date, exp)
            try:
                assert output == expected
            except AssertionError:
                print(f'❌ {test=}, {exp=}')
                print('  ', date, output, expected)
            else:
                print(f'✔ {test=}, {exp=}')

TESTS = [
    [(0, 0, 0), (0, 10, 15)],
    [(0, 10, 10), (0, 10, 15)],
    [(0, 10, 15), (2, 12, 15)],
    [(0, 10, 20), (2, 12, 15)],
    [(1, 12, 20), (2, 12, 15)],
    [(1, 13, 20), (2, 12, 15)],
    [(2, 10,  0), (2, 12, 15)],
    [(2, 10,  14), (2, 12, 15)],
    [(2, 12,  15), (3, 8, 15)],
    [(3, 8,  15), (7, 10, 15)],
]
BASES = [
    datetime.datetime.now(),
    datetime.datetime(2020, 9, 1),
    datetime.datetime(2020, 10, 1) - datetime.timedelta(days=1),
    datetime.datetime(2020, 12, 1),
    datetime.datetime(2021, 1, 1) - datetime.timedelta(days=1),
]

test(TESTS, BASES, get_next_lecture)


Logic in a card matching game controller I have a card matching game with a model of:

Card
Deck
PlayingCard
PlayingCardDeck

And another model called CardMatchingGame.

Now, I had a task to add a button to the view for new game, which deals the cards again and setting the labels to 0 (#flips and score).

I added this method:

- (IBAction)newGame:(UIButton *)sender {

    self.flipsCount = 0;
    self.game = nil;
    for (UIButton *button in self.cardButtons) {
        Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:button]];
        card.unplayble = NO;
        card.faceUp = NO;
        button.alpha = 1;
    }

    self.notificationLabel.text = nil;
    [self updateUI];
}

And this is the whole viewcontroller:

#import "CardGameViewController.h"
#import "PlayingCardsDeck.h"
#import "CardMatchingGame.h"

@interface CardGameViewController ()

@property (weak, nonatomic) IBOutlet UILabel *flipsLabel;
@property (weak, nonatomic) IBOutlet UILabel *notificationLabel;
@property (weak, nonatomic) IBOutlet UILabel *scoreCounter;
@property (strong, nonatomic) IBOutletCollection(UIButton) NSArray *cardButtons;

@property (strong, nonatomic) CardMatchingGame *game;
@property (nonatomic) int flipsCount;

@end

@implementation CardGameViewController

//creating the getter method that creates a new card game.
-(CardMatchingGame *) game {

    if (!_game) _game = [[CardMatchingGame alloc] initWithCardCount:self.cardButtons.count usingDeck:[[PlayingCardsDeck alloc] init]];
    return _game;
}

//creating a setter for the IBOutletCollection cardButtons
-(void) setCardButtons:(NSArray *)cardButtons {

    _cardButtons = cardButtons;
   [self updateUI];
}

//creating the setter for the flipCount property. Whick is setting the flipsLabel to the right text and adding the number of counts.
-(void) setFlipsCount:(int)flipsCount {

    _flipsCount = flipsCount;
    self.flipsLabel.text = [NSString stringWithFormat:@"Flips: %d", self.flipsCount];

}

-(void) updateUI {

    for (UIButton *cardButton in self.cardButtons) {
        Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:cardButton]];
        [cardButton setTitle:card.contents forState:UIControlStateSelected];
        [cardButton setTitle:card.contents forState:UIControlStateSelected|UIControlStateDisabled];
        cardButton.selected = card.isFaceUp;
        cardButton.enabled = !card.unplayble;
        if (card.unplayble) {
            cardButton.alpha = 0.1;
        }

        //updating the score
        self.scoreCounter.text = [NSString stringWithFormat:@"Score: %d", self.game.score];

        //if notification in CardMatchingGame.m is no nil, it will be presented
        if (self.game.notification) {

        self.notificationLabel.text = self.game.notification;

        }
    }
}

//Here I created a method to flipCards when the card is selected, and give the user a random card from the deck each time he flips the card. After each flip i'm incrementing the flipCount setter by one.
- (IBAction)flipCard:(UIButton *)sender {

    [self.game flipCardAtIndex:[self.cardButtons indexOfObject:sender]];
    self.flipsCount++;
    [self updateUI];
}

//cleaning the history using the cleanHistory method in CardMatchingGame.m and creating a new game by setting geme to nil
- (IBAction)newGame:(UIButton *)sender {

    self.flipsCount = 0;
    self.game = nil;
    for (UIButton *button in self.cardButtons) {
        Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:button]];
        card.unplayble = NO;
        card.faceUp = NO;
        button.alpha = 1;
    }

    self.notificationLabel.text = nil;
    [self updateUI];
}

@end

Could you please tell me if this is a logical solution? I'm a very new programmer and critiques are important to me.

You certainly get points for using IBOutletCollection(UIButton). But, it's unclear why you want to replace this array with a new one? If you're going to do that it should be this view controller that creates the new buttons and adds them as subviews. Also, if you're going to create the buttons in code then I wouldn't expect the first set to be defined in the XIB.

I'm assuming (based on your property definitions) that you're using ARC too.

This section of code can be better by maintaining and incrementing an index instead of interrogating the array you're looping over for the index:

for (UIButton *cardButton in self.cardButtons) {
    Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:cardButton]];

Also, in the updateUI button loop you don't need to set the title multiple times to the same thing and the loop should be completed before you modify the label text.

You want to know about logic, but there is basically no logic in this class. What this class does have is a bit of confusion about whether it owns the buttons and associated cards or if some other class does. The only comments that can be made really relate to efficiency rather than logic.


Liquid navigation tree generator This include was written for a Jekyll site I'm building. It generates nested HTML unordered lists to a given depth dynamically by parsing page urls. It works fine, but the code feels messy and it takes Jekyll upwards of 20 seconds to build the site (up from ~3 seconds before this navigation was implemented).

Basically the code goes through each page in the site, makes sure the page is at the right depth by examining how many levels are represented in the url (separated by depth), and outputs a link to the page if the page is indeed at the right length and its url contains the base url of its parent page.

nav.html include:

{% comment %} Init call with current_depth, max_depth. Both are zero-based. {% endcomment %}

{% assign current_lvl_url_length = include.current_depth | plus: 2 %}
{% assign next_lvl_url_length = current_lvl_url_length | plus: 1 %}
{% assign max_url_length = include.max_depth | plus: 2 %}
{% assign base_url = include.base_url %}

{% if include.base_url == nil %}
    {% assign base_url = '/' %}
    {{ base_url }}
{% endif %}

{% for node in site.pages %}
    {% assign node_url_parts = node.url | split: '/' %}
    {% assign filename = node_url_parts | last %}

    {% if node_url_parts.size == current_lvl_url_length and node.url contains base_url and filename != 'index.html' and filename contains 'html' %}

            {{node.title}}

            {% if next_lvl_url_length <= max_url_length %}
                {% assign node_base_url = node.url | replace: '.html' | append: '/' %}
                {% assign is_submenu = false %}
                {% for subnode in site.pages %}
                    {% assign subnode_url_parts = subnode.url | split: '/' %}
                    {% if subnode_url_parts.size == next_lvl_url_length and subnode.url contains node_base_url %}
                    {% assign is_submenu = true %}
                    {% break %}
                    {% endif %}
                {% endfor %}

                {% if is_submenu %}

                        {% assign next_depth = include.current_depth | plus: 1 %}
                        {% include nav.html current_depth=next_depth max_depth=include.max_depth base_url=node_base_url old_base_url=base_url %}

                        {% comment %} Reverse recursion state: {% endcomment %}

                        {% assign current_lvl_url_length = current_lvl_url_length | minus: 1 %}
                        {% assign next_lvl_url_length = next_lvl_url_length | minus: 1 %}
                        {% assign base_url = include.old_base_url %}
                        {% if include.old_base_url == nil %}
                            {% assign base_url = '/' %}
                        {% endif %}

                {% endif %}
            {% endif %}

    {% endif %}
{% endfor %}

header.html where nav.html is initially called:

    {{ site.title }}

        <svg viewBox="0 0 18 15">
          <path fill="#424242" d="M18,1.484c0,0.82-0.665,1.484-1.484,1.484H1.484C0.665,2.969,0,2.304,0,1.484l0,0C0,0.665,0.665,0,1.484,0 h15.031C17.335,0,18,0.665,18,1.484L18,1.484z"/>
          <path fill="#424242" d="M18,7.516C18,8.335,17.335,9,16.516,9H1.484C0.665,9,0,8.335,0,7.516l0,0c0-0.82,0.665-1.484,1.484-1.484 h15.031C17.335,6.031,18,6.696,18,7.516L18,7.516z"/>
          <path fill="#424242" d="M18,13.516C18,14.335,17.335,15,16.516,15H1.484C0.665,15,0,14.335,0,13.516l0,0 c0-0.82,0.665-1.484,1.484-1.484h15.031C17.335,12.031,18,12.696,18,13.516L18,13.516z"/>
        </svg>

{% include nav.html current_depth=0 max_depth=2 %}

Put a lot of work into refactoring this code, trying to maximize for readability and simplicity in an effort to increase maintainability. I'm posting my results here since I know creating recursive, dynamic, no-plugin navigation is an issue Jekyll users often face, and some are likely to land here searching for a solution.

nav.html

{% comment %} Init call with base_url (optional) and max_depth. {% endcomment %}

{% if lvl_base_url == nil %}
  {% assign lvl_base_url = '/' %}
  {% if include.base_url %}
    {% assign lvl_base_url = include.base_url %}
  {% endif %}
{% endif %}

{% assign current_lvl_url_length = lvl_base_url | split: '/' | size %}
{% if lvl_base_url == '/' %}
  {% assign current_lvl_url_length = 1 %}
{% endif %}

{% capture links %}
  {% for node in site.pages %}
    {% assign node_url_length = node.url | split: '/' | size | minus: 1 %}
    {% assign filename = node.url | split: '/' | last %}
    {% if node_url_length == current_lvl_url_length and node.url contains lvl_base_url and filename != 'index.html' and filename contains 'html' %}

        {{ node.title }}
        {% if include.max_depth > current_lvl_url_length | plus: 1 %}
          {% assign lvl_base_url = node.url | replace: '.html' | append: '/' %}

          {% comment %} Include can access parent state; hence no variables passed explicitly: {% endcomment %}
          {% include nav.html %}

          {% comment %} Reverse state changes made by child: {% endcomment %}
          {% assign current_lvl_url_length = current_lvl_url_length | minus: 1 %}
          {% assign directory_to_remove = lvl_base_url | split: '/' | last | append: '/' %}
          {% assign lvl_base_url = lvl_base_url | replace: directory_to_remove %}
        {% endif %}

    {% endif %}
  {% endfor %}
{% endcapture %}

{% assign links = links | strip %}

{% if links.size > 0 %}

{{ links }}

{% endif %}

header.html [example usage]

    {{ site.title }}

        <svg viewBox="0 0 18 15">
          <path fill="#424242"
                d="M18,1.484c0,0.82-0.665,1.484-1.484,1.484H1.484C0.665,2.969,0,2.304,0,1.484l0,0C0,0.665,0.665,0,1.484,0 h15.031C17.335,0,18,0.665,18,1.484L18,1.484z"/>
          <path fill="#424242"
                d="M18,7.516C18,8.335,17.335,9,16.516,9H1.484C0.665,9,0,8.335,0,7.516l0,0c0-0.82,0.665-1.484,1.484-1.484 h15.031C17.335,6.031,18,6.696,18,7.516L18,7.516z"/>
          <path fill="#424242"
                d="M18,13.516C18,14.335,17.335,15,16.516,15H1.484C0.665,15,0,14.335,0,13.516l0,0 c0-0.82,0.665-1.484,1.484-1.484h15.031C17.335,12.031,18,12.696,18,13.516L18,13.516z"/>
        </svg>

      {% if page.url contains '/academy' %}
        {% include nav.html base_url='/academy/' max_depth=3 %}
      {% elsif page.url contains '/college' %}
        {% include nav.html base_url='/college/' max_depth=3 %}
      {% else %}
        {% include nav.html max_depth=2 %}
      {% endif %}


Msbuild batch - if any build fails than skip the last one I have this batch that builds some projects using msbuild.

What I want to do is to skip building my-last.proj if any of the above builds fail: my-proj1.proj to my-proj5.proj using environment variables.

I'm sure that there is another cleaner way to do this, but so far I just can't figure it out.

msbuild /target:Win32 my-proj1.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED32=1
)
msbuild /target:X64 my-proj1.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED64=1
)

msbuild /target:ManagedWin32 my-proj1.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED32=1
)
msbuild /target:ManagedX64 my-proj1.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED64=1
)

msbuild /target:Win32 my-proj2.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED32=1
)
msbuild /target:X64 my-proj2.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED64=1
)

msbuild /target:Win32 my-proj3.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED32=1
)
msbuild /target:Win32 my-proj4.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED32=1
)
msbuild /target:Win32 my-proj5.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED32=1
)

IF NOT "%FAILED32%" == "1" (
msbuild /target:setup32 my-last.proj
)

msbuild /target:X64 my-proj3.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED64=1
)
msbuild /target:X64 my-proj4.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED64=1
)
msbuild /target:X64 my-proj5.proj
IF NOT ERRORLEVEL 0 (
    set /p BUILDFAILED64=1
)

IF NOT "%FAILED64%" == "1" (
msbuild /target:setup64 my-last.proj
)

Does anyone have any ideas?

You probably want to localize any environment changes.

You should initialize your variables to a known state at the beginning, otherwise you could get the wrong result.

You have a lot of redundant code that can be eliminated by using a CALLed subroutine

You can use the || operator to conditionally execute code if the previous command failed. I find it simpler than using IF ERRORLEVEL.

You can save a bit of typing by storing code in a variable to be used as a simple macro.

@echo off
setlocal
set "failed32="
set "failed64="
set "build=call :build"

%build% Win        32 my-proj1
%build% X          64 my-proj1
%build% ManagedWin 32 my-proj1
%build% ManagedX   64 my-proj1
%build% Win        32 my-proj2
%build% X          64 my-proj2
%build% Win        32 my-proj3
%build% Win        32 my-proj4
%build% Win        32 my-proj5

if not defined failed32 msbuild /target:setup32 my-last.proj

%build% X          64 my-proj3
%build% X          64 my-proj4
%build% X          64 my-proj5

if not defined failed64 msbuild /target:setup64 my-last.proj
exit /b

:build  target  size  proj
msbuild /target:%1%2 %3.proj || set failed%%2=1
exit /b


Running a master query and a query of queries The following is code I'm running to FIRST, SECOND, THIRD and so on until SEVENTH with a master query and a query of query.

As it's clear that I'm using the master query for each of FIRSTCONN, SECONDCONN etc., I want to have only one master query and the rest subqueries.

Is this an efficient way to do so?
How can I make my code efficient?

<!--- QoQ for FIRSTCONN --->
<!--- Master Query --->
<cfquery datasource = "XX.XX.X.XX" name="master1">
SELECT DATE(Timedetail) as FIRSTCONN, COUNT(Timedetail) as FIRSTOccurances, EVENTS
FROM  MyDatabase
WHERE EVENTS = "FIRST"
GROUP BY FIRSTCONN ;
</cfquery>

<!--- Detail Query --->
<!--- <cfdump var = "#master#"> --->
<cfquery dbtype="query" name="detail1">
SELECT  *
FROM master1
WHERE FIRSTCONN  >= <cfqueryparam value="#form.startdate#" cfsqltype="cf_sql_varchar">
AND FIRSTCONN  <  <cfqueryparam value="#dateAdd('d', 1,form.enddate)#" cfsqltype="cf_sql_varchar">;
</cfquery>

<!--- QoQ for SECONDCONN --->
<!--- Master Query --->
<cfquery datasource = "XX.XX.X.XX" name="master2">
SELECT DATE(Timedetail) as SECONDCONN, COUNT(Timedetail) as SECONDOccurances, EVENTS
FROM  MyDatabase
WHERE EVENTS = "SECOND"
GROUP BY SECONDCONN ;
</cfquery>

<!--- Detail Query --->
<!--- <cfdump var = "#master#"> --->
<cfquery dbtype="query" name="detail2">
SELECT  *
FROM   master2
WHERE  SECONDCONN  >= <cfqueryparam value="#form.startdate#" cfsqltype="cf_sql_varchar">
AND    SECONDCONN  <  <cfqueryparam value="#dateAdd('d', 1,form.enddate)#" cfsqltype="cf_sql_varchar">;
</cfquery>

You could use a UNION to run one master query

<cfquery datasource = "XX.XX.X.XX" name="master1">
SELECT DATE(Timedetail) as FIRSTCONN, COUNT(Timedetail) as FIRSTOccurances, EVENTS
FROM  MyDatabase
WHERE EVENTS = "FIRST"
GROUP BY FIRSTCONN
UNION
SELECT DATE(Timedetail) as SECONDCONN, COUNT(Timedetail) as SECONDOccurances, EVENTS
FROM  MyDatabase
WHERE EVENTS = "SECOND"
GROUP BY SECONDCONN;
</cfquery>

and then in your QoQ reference the correct column name

<cfquery dbtype="query" name="detail1">
SELECT  *
FROM master1
WHERE FIRSTCONN  >= <cfqueryparam value="#form.startdate#" cfsqltype="cf_sql_varchar">
AND FIRSTCONN  <  <cfqueryparam value="#dateAdd('d', 1,form.enddate)#"     cfsqltype="cf_sql_varchar">
AND EVENTS = 'FIRST';

<cfquery dbtype="query" name="detail2">
SELECT  *
FROM   master1
WHERE  SECONDCONN  >= <cfqueryparam value="#form.startdate#"     cfsqltype="cf_sql_varchar">
AND    SECONDCONN  <  <cfqueryparam value="#dateAdd('d', 1,form.enddate)#" cfsqltype="cf_sql_varchar">
AND EVENTS = 'SECOND';
</cfquery>


State machine for handling Telnet sequences for an FTP server I've asked Code Climate to generate metrics for the ftpd Ruby gem.  It correctly identified the God class; I know what to do about that.  But one of the smaller classes has me stumped.  This is telnet.rb:

# -*- ruby encoding: us-ascii -*-

module Ftpd

  # Handle the limited processing of Telnet sequences required by the
  # FTP RFCs.
  #
  # Telnet option processing is quite complex, but we need do only a
  # simple subset of it, since we can disagree with any request by the
  # client to turn on an option (RFC-1123 4.1.2.12).  Adhering to
  # RFC-1143 ("The Q Method of Implementing TELNET Option Negiation"),
  # and supporting only what's needed to keep all options turned off:
  #
  # * Reply to WILL sequence with DONT sequence
  # * Reply to DO sequence with WONT sequence
  # * Ignore WONT sequence
  # * Ignore DONT sequence
  #
  # We also handle the "interrupt process" and "data mark" sequences,
  # which the client sends before the ABORT command, by ignoring them.
  #
  # All Telnet sequence start with an IAC, followed by at least one
  # character.  Here are the sequences we care about:
  #
  #     SEQUENCE             CODES
  #     -----------------    --------------------
  #     WILL                 IAC WILL option-code
  #     WONT                 IAC WONT option-code
  #     DO                   IAC DO option-code
  #     DONT                 IAC DONT option-code
  #     escaped 255          IAC IAC
  #     interrupt process    IAC IP
  #     data mark            IAC DM
  #
  # Any pathalogical sequence (e.g. IAC + \x01), or any sequence we
  # don't recognize, we pass through.

  class Telnet

    # The command with recognized Telnet sequences removed

    attr_reader :plain

    # Any Telnet sequences to send

    attr_reader :reply

    # Create a new instance with a command that may contain Telnet
    # sequences.
    # @param command [String]

    def initialize(command)
      telnet_state_machine command
    end

    private

    module Codes
      IAC  = 255.chr    # 0xff
      DONT = 254.chr    # 0xfe
      DO   = 253.chr    # 0xfd
      WONT = 252.chr    # 0xfc
      WILL = 251.chr    # 0xfb
      IP   = 244.chr    # 0xf4
      DM   = 242.chr    # 0xf2
    end
    include Codes

    def telnet_state_machine (command)
      @plain = ''
      @reply = ''
      state = :idle
      command.each_char do |c|
        case state
        when :idle
          if c == IAC
            state = :iac
          else
            @plain << c
          end
        when :iac
          case c
          when IAC
            @plain << c
            state = :idle
          when WILL
            state = :will
          when WONT
            state = :wont
          when DO
            state = :do
          when DONT
            state = :dont
          when IP
            state = :idle
          when DM
            state = :idle
          else
            @plain << IAC + c
            state = :idle
          end
        when :will
          @reply << IAC + DONT + c
          state = :idle
        when :wont
          state = :idle
        when :do
          @reply << IAC + WONT + c
          state = :idle
        when :dont
          state = :idle
        else
          raise "Unknown state #{state.inspect}"
        end
      end
    end

  end
end

Code Climate does not like the complexity of #telnet_state_machine.  I agree, but I don't know how to reduce the complexity without also making the state machine harder to follow.  State machines never seem to be all that readable as it is.  What would you suggest?

Note: If you want to try some refactoring, this class has rspec test coverage.  Just do "git clone" of the ftpd project.

As suggested by @m_x, This solution uses a parser of sorts, driven by the StringScanner built-in class.  This is very compact, pretty readable, and gets rid of the state machine altogether:

Some methods to handle actions:

def accept(scanner)
  @plain << scanner[1]
end

def reply_dont(scanner)
  @reply << IAC + DONT + scanner[1]
end

def reply_wont(scanner)
  @reply << IAC + WONT + scanner[1]
end

def ignore(scanner)
end

A list of telnet sequences:

# Telnet sequences to handle, and how to handle them

SEQUENCES = [
  [/#{IAC}(#{IAC})/, :accept],
  [/#{IAC}#{WILL}(.)/m, :reply_dont],
  [/#{IAC}#{WONT}(.)/m, :ignore],
  [/#{IAC}#{DO}(.)/m, :reply_wont],
  [/#{IAC}#{DONT}(.)/m, :ignore],
  [/#{IAC}#{IP}/, :ignore],
  [/#{IAC}#{DM}/, :ignore],
  [/(.)/m, :accept],
]

And the parser that uses them:

# Parse the the command.  Sets @plain and @reply

def parse_command(command)
  @plain = ''
  @reply = ''
  scanner = StringScanner.new(command)
  while !scanner.eos?
    SEQUENCES.each do |regexp, method|
      if scanner.scan(regexp)
        send method, scanner
        break
      end
    end
  end
end


Read a single int from an optional .properties file I am no newcomer to Java. One thing that confounds me is why it is so messy to load values from .properties files.

I have an application where, if the .properties file is found, then the value if found, should be used. And in any other circumstance, to use the default. (In the correct deployment, the server is run with sufficient permissions to engage with the socket listener 443, whereas in the development environment or any other environment where a person goes to the trouble to insert the .properties file, another port will be used).

The location of the file is com/foo/bar/webserver.properties in the in the same directory as the class file com/foo/bar/WebServer.class.

The content of webserver.properties is simply:

 listen=4444

Since this such a terribly simple function I would like to know if the community of reviewers sees a more elegant/succinct/secure/complete/correct way to load the value. I feel like I must be overlooking something basic for what feels like, to me, should be a one-liner more like the imaginary API:

    // set int value for key "listen", or else default value 443
    int port = Properties.loadProperties( "webserver.properties" ).getInt( "listen", 443 );

And here is my real code for review:

    int port;
    try ( InputStream webserverProperties = WebServer.class.getResourceAsStream( "webserver.properties" ) ) {
        if ( webserverProperties == null ) {
            port = 443;
        } else {
            Properties p = new Properties();
            p.load( webserverProperties );
            String listen = p.getProperty( "listen", "443" );
            port = Integer.parseInt( listen );
        }
    }
    catch ( NumberFormatException e ) { port = 443; }
    finally {}

you could at least get rid of the if by combining the exceptions:

int port;
try ( InputStream webserverProperties = WebServer.class.getResourceAsStream( "webserver.properties" ) ) {
        Properties p = new Properties();
        p.load( webserverProperties );
        String listen = p.getProperty( "listen", "443" );
        port = Integer.parseInt( listen );
}
catch ( NumberFormatException|NullPointerException e ) { port = 443; }
finally {}

or ignore exceptions at all:

int port= 443;
try ( InputStream webserverProperties = WebServer.class.getResourceAsStream( "webserver.properties" ) ) {
        Properties p = new Properties();
        p.load( webserverProperties );
        String listen = p.getProperty( "listen", "443" );
        port = Integer.parseInt( listen );
}
catch ( Exception e ) {}
finally {}


Retrive relation on a polymorphic model I have a Game model that is "static". Containing game information and such, and users can comment on all games.

Games model

public function comments()
{
    return $this->morphMany(Comment::class, 'commentable');
}

And then I have a Comment model. This has a User relation and a Comments relation.

public function commentable()
{
    return $this->morphTo();
}

public function user()
{
    return $this->belongsTo(User::class);
}

What I want to do is to fetch the 10 games sorted by the latest comment top, be able to display who made the comment, and when.

I have this query that seems to do the job, but it makes an absurd amount of queries. Can I somehow keep the queries to a minimum?

    $games = Game::join('comments', 'games.id', '=', 'comments.commentable_id')
    ->where('comments.commentable_type', Game::class)
    ->latest('comments.created_at')
    ->groupBy('games.id')
    ->take(10)
    ->withCount('comments')
    ->get()->each(function($games){
        $games->comment = $games->comments()->orderBy('created_at', 'desc')->first();
        $games->user = User::find($games->comment->user_id);
        return $games;
    });

You didn't specify which version of Laravel is used, so I am going to presume it is 5.x. If that is incorrect, please notify me and update your post with such relevant information

I am not sure exactly how to reduce the queries for comments but the suggestion below should allow remove the loop of queries for the user on each game.

Eager Loading

(Nested) Eager Loading - utilizing the with() method - can be used to populate the user property for each item in the collection:

$games = Game::join('comments', 'games.id', '=', 'comments.commentable_id')
->where('comments.commentable_type', Game::class)
->latest('comments.created_at')
->groupBy('games.id')
->take(10)
->with('comment.user')
->withCount('comments')

Variable naming

The lambda/closure passed to each() accepts a parameter named $games. Isn't it actually a representation of a game? If so, $game would be more appropriate. Additionally, other developers reading the code may be confused because $games is also used for the assignment of the collection.


Credit card validator using Luhn Algorithm I'm looking for feedback or tips to maybe make my code more readable or faster or just general tips to help get myself started with app making with Python.

import sys

# check for valid input
if len(sys.argv) == 2:
    try:
        int(sys.argv[1])
        if(len(str(sys.argv[1])) == 16):
            pass
        else:
            print("Not 16 digits!")
            sys.exit()
    except ValueError:
        print('Not an integer!')
        sys.exit()
else:
    print('Not enough or too many command line arguments! \n Proper use \"python Check.py <credit card number here> \" ')
    sys.exit()

def main():
    # put the digits into a list
    number = convertToList(sys.argv[1])
    sum = cardCheck(number)
    if (sum%10 == 0):
        print('Valid Card!')
    else:
        print('Invalid Card!')

#converts initial passed int variable to list
def convertToList(num):
    numStr = str(num)
    numList = []
    for digit in numStr:
        numList.append(int(digit))
    return (numList)

def cardCheck(digitList, count = 0):
    sum = 0
    #if digit is every second digit multiply by 2
    if(count%2 == 0 & count < 15):
        digitList[count] = (digitList[count] * 2)
        #if is 2 digit number after multiplication
        if(digitList[count] >= 10):
            digitList[count] = addDigits(digitList[count])
            cardCheck(digitList, count + 1)
        else:
            cardCheck(digitList, count + 1)
    #progresses program
    elif(count < 15):
        cardCheck(digitList, count + 1)
    else:
        return 0
    for digits in digitList:
        sum += int(digits)
    return sum

#resolve 2 digit number conflict by adding the digits of the number and returning it
def addDigits(num):
    list = str(num)
    sum = 0
    for digits in list:
        sum += int(digits)
    return sum

if __name__ == '__main__':
        main()

For making your code faster, we can choose more Pythonic ways in some parts of it.
First, let's take a look at convertToList function. The goal of this function is to split digits of a number to a list of int values. I want to follow your algorithm and do this by converting the number to the str and then splitting it.

I want to do this by "List Comprehension":

def convert_to_list(num):
    result = [int(x) for x in str(num)]
    return result

We made some changes here. First of all, I changed the name of the function from camelCase (convertToList) to snake_case (convert_to_list) because according to the Python style guide, it is the better way. You can read more about the Python style guide at PEP8.

Next change is I replaced all your code with a single line list comprehension. The first advantage, we have written less code. Less code means probably fewer bugs.

But the second advantage here is this code is so much faster. How much? I have written a simple script for it; the result is that on average, the second version is 1.7 times faster.

Now let's move on and take another look at addDigits function. I want to choose pythonic way again here:

def sum_of_digits(number):
    num_list = convert_to_list(number)
    return sum(num_list)

Like the previous time, I changed the name. I think this name is clearer and everyone could tell what this code does.

For converting the number to a list of digits, I used convert_to_list function instead of writing the whole code again. We are using functions to avoiding duplication, so I think it's a bad idea to write the same code here. For calculating the sum of digits in a list, I strongly recommend you that always use built-in function sum. It's faster, you don't need to write new code and every Python programmer can tell what you are doing at first glance. This code is somehow 1.2 times faster than previous.

Now let's go to the beginning of your code. We want to parse command line parameters and be sure that the input is correct.

Even though we only call those codes once, I think it is a great favor to the readability of the code to move those lines in a separate function.

from re import search

def get_input_from_cmd(args_list):
    if len(args_list) != 2:
        raise Exception("You should enter a 16-digit number as input argument")
    return args_list[1]

def is_input_valid(input_str):
    return bool(search(r"\d{16}", input_str))

I separated your code into two functions. The first function gets argv list as an input parameter and if its length is equal to 2, returns the second parameter. Else, it will raise an Exception. There are a lot of people out there who are against exceptions and I agree with most of their reasons. But when we want our program to stop when a bad input came in, I think using exceptions is the best way.

The second function simply uss search function of re module. It checks that the input string only contains 16 digits. If that assumption will be true, True will be returned. Otherwise, the False value is what you get.

Now you can change your main function like this:

def main():
    input_string = get_input_from_cmd(sys.argv)
    if is_input_valid(input_string):
        digits_list = convert_to_list(input_string)
        card_checking_sum = card_check(digits_list)
        if card_checking_sum % 10 == 0:
            print('Valid Card!')
        else:
            print('Invalid Card!')
    else:
        print("Invalid Card number")

What we do is if card number is not a 16-digit number, code in the last else will execute. Otherwise, codes in the first if will run. That looks nicer to me.

So now let's go to the last function, the cardCheck.

def card_check(digits_list, count=0):
    result = 0
    if count % 2 == 0:
        digits_list[count] *= 2
        if digits_list[count] >= 10:
            digits_list[count] = sum_of_digits(digits_list[count])
    if count < 15:
        card_check(digits_list, count + 1)
    else:
        return 0
    result += sum(digits_list)
    return result

There were some problems in your code that I tried to fix. First, you don't need to put if conditions in parentheses. In python, don't need means you should not.

Second, if you run the same code in if and else; you should take that part of code away from if statement. That is what I did with card_check(digits_list, count + 1) line. That line was repeated needlessly.

In the end, for logical operations, you should use operators like and and or, not & and |.

Here is the full code. I hope that helps you:

from re import search
import sys

def get_input_from_cmd(args_list):
    if len(args_list) != 2:
        raise Exception("You should enter a 16-digit number as input argument")
    return args_list[1]

def is_input_valid(input_str):
    return bool(search(r"\d{16}", input_str))

def convert_to_list(num):
    result = [int(x) for x in str(num)]
    return result

def sum_of_digits(number):
    num_list = convert_to_list(number)
    return sum(num_list)

def card_check(digits_list, count=0):
    result = 0
    if count % 2 == 0:
        digits_list[count] *= 2
        if digits_list[count] >= 10:
            digits_list[count] = sum_of_digits(digits_list[count])
    if count < 15:
        card_check(digits_list, count + 1)
    else:
        return 0
    result += sum(digits_list)
    return result

def main():
    input_string = get_input_from_cmd(sys.argv)
    if is_input_valid(input_string):
        digits_list = convert_to_list(input_string)
        card_checking_sum = card_check(digits_list)
        if card_checking_sum % 10 == 0:
            print('Valid Card!')
        else:
            print('Invalid Card!')
    else:
        print("Invalid Card number")

if __name__ == '__main__':
        main()


Display date and/or time The code below takes an input and based on the input gives out a time, date or both. Please will you review me for this and tell me how I can improve this? By the way, I only started learning python.

import datetime

x = datetime.datetime.now()
s = '/'
c = ':'
print("Welcome to Date and Time!")
input = int(input("Please choose one of these options below and type it in the prompt: \n 1 - To know the date (DD : MM : YY) \n 2 - To know the time (Hours : Minutes : Seconds ) \n 3 - To know both date and time.\n"))

if input == 1 :
    print(str(x.day) + s + str(x.month) + s + str(x.year) )
elif input == 2 :
    print(x.strftime('%H') + c + x.strftime('%M') + c + x.strftime('%S'))
elif input == 3 :
    print(str(x.day) + s + str(x.month) + s + str(x.year) + ' ' + x.strftime('%H') + c + x.strftime('%M') + c + x.strftime('%S'))
else:
    print("You didn't type in 1, 2 or 3!")

My first time here providing code review on stackexchange. I randomly clicked a link and arrived here. Anyway, here's how I'd improve your code:

#!/usr/bin/env python

import datetime

DATE_FORMAT = "%d/%m/%y"
DATE_FORMAT_FOR_HUMANS = "DD/MM/YY"
TIME_FORMAT = "%H:%M:%S"
TIME_FORMAT_FOR_HUMANS = "Hour:Minute:Second"

def prompt_for_user_input():
    return int(
        input(
            """Please choose one of these options below and type it in the prompt:
1 - To know the date ({DATE_FORMAT_FOR_HUMANS})
2 - To know the time ({TIME_FORMAT_FOR_HUMANS})
3 - To know both date and time. ({DATE_FORMAT_FOR_HUMANS} {TIME_FORMAT_FOR_HUMANS})
"""
        )
    )

def print_datetime_from_input(user_input, datetime_value):
    formatted_date = get_formatted_date(datetime_value)
    formatted_time = get_formatted_time(datetime_value)

    if user_input == 1:
        print(formatted_date)
    elif user_input == 2:
        print(formatted_time)
    elif user_input == 3:
        print(formatted_date, formatted_time)
    else:
        print("You didn't type in 1, 2 or 3!")

def get_formatted_time(datetime_value):
    return datetime_value.strftime(TIME_FORMAT)

def get_formatted_date(datetime_value):
    return datetime_value.strftime(DATE_FORMAT)

def main():
    now = datetime.datetime.now()

    print("Welcome to Date and Time!")

    user_input = prompt_for_user_input()
    print_datetime_from_input(user_input, now)

if __name__ == "__main__":
    main()

And here's why:

It's good practice to separate logic into smaller functions as it makes it easier to run tests against smaller parts of a system. For the above, I could have wrote three separate tests, one that ensures that for a given user input, the right date gets displayed. Another test for the the formatting of the date and time, another one for the main execution.

If you're curious about tests, I recommend you to search online why do we write unit tests and how to write tests in python. This helped me a lot in writing better code.

Separating into smaller function helps for reusability. In the given example, I tried to separate the logic from the user input and the display.
I used an IDE to auto indent and format. The above follows PEP8.
I extracted the formats into DATE_FORMAT and TIME_FORMAT. Again, we are not reusing anything, but imagine you'd like to reuse the formats somewhere else. It also makes it easier to change the formats as you only need to change one line to see a different result.
I've split the input into multi line string which makes it easier for readability. I also moved the displayed format for the user beside the format used by the code to display it, this way, if you change one of the formats, you can update the prompt for the user more easily.
I've added a main function and a if __name__ == "__main__": condition to call main. In this case, it will execute the main function if the script is invoked directly. Imagine your script is called ask_for_time.py, you could now import ask_for_time.DATE_FORMAT and the main execution won't be executed on import time.
The first line #!/usr/bin/env python is called a shebang, it's used to tell the shell what to use to execute the script in case it is invoked directly (example: ./ask_for_time.py instead of python ask_for_time.py.

You should always try to use the DRY principle.

For the datetime formatting, you can refer to the following documentation: http://strftime.org/

As an exercise, I suggest you to try and write unit tests to make sure the code you write does what you intend it to do.
Good luck and have fun learning python ✌


PHP login and session establishment I would like to ask your advice on my simple code to login and registration sessions.

In the User class, login function:

public function login($username, $password){
    $this->db->query("SELECT * FROM users WHERE username = :username AND status = :status LIMIT 1");
    $this->db->bind(':username', $username);
    $this->db->bind(':status', 1);
    $row = $this->db->single();
    $count = $this->db->rowCount();
        if ($count > 0) {
            if (password_verify($password, $row['password'])) {
                $_SESSION['session'] = [
                    'id' => $row['id'],
                    'username' => $row['username'],
                    'email' => $row['email'],
                    ];
                return true;
            } else {
                return false;
            }
        }
}

check logged:

public function isLoggedIn() {
    if (isset($_SESSION['session'])) {
    return true;
    }
}

And login page with check logged:

if ($user->isLoggedIn()) {
    header('Location: index.php');
    exit();
}

if(isset($_POST['login'])){

    //Retrieve the field values from our login form.
    $username = !empty($_POST['username']) ? trim($_POST['username']) : null;
    $password = !empty($_POST['password']) ? trim($_POST['password']) : null;

    if($user->login($username, $password)) {
        header('Location: index.php');
        exit();
    } else {
        $message[] = 'We found problems with the login, please try again.';
    }
}

The could looks like it would work properly, and I think it's clearly written and easy to understand.

When you check that a user is logged in, you should check for something in the session that clearly states they are logged in. For instance you could check that $_SESSION['user']['loggedIn'] === true. It's risky to simply test for  isset($_SESSION['session']) as another programmer, or even you in six months time, could write code to set that when the user is not logged in. Perhaps they will want to store a message in the session, like "sorry your password didn't match try again".

I would replace $_SESSION['session'] with $_SESSION['User']. You might then want to only access $_SESSION['User'] from the User class of your application. Other parts of the application may want to store other things in the session - messages to display on next request, or a basket of goods, or whatever the app deals with.

I assume you have called session_start() before the code we see runs.

Using password_verify() is good. Far too many apps implement their own insecure password algorithms.

I don't see the 'new' keyword, which is good - that means you may be doing dependency injection by having the constructor of User take a db as a parameter. If that's true you can easily write a test for the login function that uses a mock db.


Determine the length of the largest contiguous segment Problem Link- https://www.hackerearth.com/challenges/competitive/october-circuits-19/algorithm/ap-1-f43562f4/

  Problem Statement :

  You are given an integer array  of size A. You are also given  queries Q. In each query, you are given three integers L, R, and D respectively.

  You are required to determine the length of the largest contiguous segment in the indices range [L, R] of A that forms an arithmetic progression with a common difference of D

  Note: The segment whose length is 1 always forms an arithmetic progression of a common difference D

  Input format

  First line: Two space-separated integers N and Q respectively
  Second line: N space-separated integers denoting elements of A
  Next Q lines: Three space-separated integers L, R, and D (1<= L <= R <= N)       respectively

  Output format

  Print Q lines representing the answer such as the ith line denotes the answer for the ith query.

  Constraints

1 <= N <= 200000
1 <= Ai <= 200000
-200000 <= D <= 200000

  Sample Input

5 2
1 2 3 5 5
1 5 1
4 4 3

Sample Output
3
1

  Explanation

  For the first query, [1,2,3] forms an AP with difference 1.

  For the second query, [5] forms an AP.

I was attempting to solve this problem on hacker earth, however for large size N and Q the following code exceeds the time limit. Is there any way I can make this code run faster, other than changing the algorithm?

N, Q = [int(x) for x in input().split()]

A = list(map(int, input().split()))

for testcase in range(Q):
    L, R, D = [int(x) for x in input().split()]

    if L == R:
        print("1")
        continue

    count_length = 0
    count_temp = 0

    for index in range(L, R):
        if A[index] == A[index-1] + D:
            count_temp += 1
            if index == R-1:
                count_temp += 1
                count_length = max(count_length, count_temp)

        else:
            count_temp += 1 # to include the first element of AP
            count_length = max(count_length, count_temp)
            count_temp = 0

    print(count_length)

Code Review

PEP 8

PEP 8 recommends snake_case for variable names, so N, Q, A, L, R, and D all violate the guidelines.

A space should be used around operators: index - 1 and R - 1

Unused variables

N and testcase are never used, so should be replaced with the throwaway _ variable.

Variable naming

N, Q, A, L, R, and D are all "almost meaningless".  Yes, they are directly from the problem description, but you could do better naming them array_size, num_queries, array, left, right and difference.

Is count_length a count of lengths?  Doesn't seem to be; it is more like a max_length.

Is count_temp a count of temporaries?  Or is it more like a segment_length?

_, num_queries = [int(x) for x in input().split()]

array = list(map(int, input().split()))

for _ in range(num_queries):
    left, right, difference = [int(x) for x in input().split()]

    if left == right:
        print("1")
        continue

    max_length = 0
    segment_length = 0

    for index in range(left, right):
        if array[index] == array[index - 1] + difference:
            segment_length += 1
            if index == right - 1:
                segment_length += 1
                max_length = max(max_length, segment_length)

        else:
            segment_length += 1 # to include the first element of AP
            max_length = max(max_length, segment_length)
            segment_length = 0

    print(max_length)

Optimization

Without changing the Algorithm???

Ok, you're looking for just key-hole level optimizations.  Let's see what we can do.

    for index in range(left, right):
        if array[index] == array[index - 1] + difference:
            ...
        else:
            ...

The above is inefficient code.  Indexing into a list takes time, and you are doing it twice.  Moreover, subtraction by 1 takes time, and it was to get the value which was looked up on the previous iteration!

Compare the above code with:

    prev = array[left - 1]
    for curr in array[left:right]:
        if curr == prev + difference:
            ...
        else:
            ...
        prev = curr

The indexing has been replaced with iteration over an array slice, and the current value from one iteration is carried forward to as the previous value in the next iteration.  That should be a win, performance wise.

    segment_length = 0

    for ...:
        if ...:
            ...
            if ...:
                segment_length += 1
                max_length = max(max_length, segment_length)

        else:
            segment_length += 1 # to include the first element of AP
            max_length = ...
            segment_length = 0

Why are you starting the count from 0?  That means you need this segment_length += 1 to account for that first element all the time.  If you initialized segment_length = 1, you can skip that adjustment statement every time you reach the end of an arithmetic progression, saving another wee bit of time:

    segment_length = 1

    for ...:
        if ...:
            ...
            if ...:
                max_length = max(max_length, segment_length)

        else:
            max_length = ...
            segment_length = 1

Finally, the code I hate the most:

    for index in range(left, right):
        if ...:
            ...
            if index == right - 1:
                ...

        else:
            ...
        ...

When does the for loop end?  After index loops the final time with index = right - 1, of course.  So the only time this if condition will be true is during the last iteration of the loop.  During all of the remaining loop iterations, you are wasting time, subtraction 1 from right and comparing the result to index, when it can't possibly ever be true.  Simply move the code to follow the loop.

_, num_queries = [int(x) for x in input().split()]

array = list(map(int, input().split()))

for _ in range(num_queries):
    left, right, difference = [int(x) for x in input().split()]

    if left == right:
        print("1")
        continue

    max_length = 0
    segment_length = 1

    prev = array[left - 1]
    for curr in array[left:right]:
        if curr == prev + difference:
            segment_length += 1

        else:
            max_length = max(max_length, segment_length)
            segment_length = 1

        prev = curr

    max_length = max(max_length, segment_length)

    print(max_length)

With changing the Algorithm

You explicitly requested optimization without changing the algorithm, so I'll leave you to discover where there is an algorithmic improvement to be made.


Simple object oriented console calculator I'd like to learn OOP. Could someone check my code and tell me how to improve it?

package com.company;

import java.util.InputMismatchException;
import java.util.Scanner;

public class CalculatorApp {
    private static Scanner scanner = new Scanner(System.in);
    public static void main(String[] args) {
        System.out.println("Write two numbers and +, -, * or / sign");
        char operator = 0;
        double firstNum = 0;
        double secondNum = 0;
        try {
            firstNum = scanner.nextDouble();
            secondNum = scanner.nextDouble();
            operator = scanner.next().charAt(0);
        } catch (InputMismatchException ime) {
            System.out.println("invalid input");
        } finally {
            if (operator != '+' & operator != '-' & operator != '*' & operator != '/') {
                throw new InputMismatchException();
            }
            Calculator calculator = new Calculator(firstNum, secondNum,    operator);
            System.out.println(calculator.makeCalculation());
        }
    }
}

Calculator class:

package com.company;

import java.util.HashMap;
import java.util.Map;

public class Calculator {
    private char operation;
    private double operand1;
    private double operand2;
    private Map<Character, Operation> operationMap = new HashMap<>();

    public Calculator(double operand1, double operand2, char operation) {
        this.operand1 = operand1;
        this.operand2 = operand2;
        this.operation = operation;

        operationMap.put('+', new Addition());
        operationMap.put('-', new Subtraction());
        operationMap.put('*', new Multiplication());
        operationMap.put('/', new Division());
    }

    public double makeCalculation() {
        Operation operationMapValue = null;
        if (operationMap.containsKey(operation)) {
            operationMapValue = operationMap.get(operation);
            System.out.println(operationMap.get(operation));
        } else {
            System.out.println("Invalid sign");
        }
        return operationMapValue.calculateResult(operand1, operand2);
    }
}

Operation interface:

package com.company;

public interface Operation {
    double calculateResult(double left, double right);
}

One of implementing classes:

package com.company;

public class Addition implements Operation {
    @Override
    public double calculateResult(double left, double right) {
        return left + right;
    }
}

And Tests:

package com.company;

import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.*;

class CalculatorTest {
    Calculator calc = new Calculator(6.0, 2.0, '+');

    @Test
    void addsTwoNumbers() {
        assertEquals(8.0, calc.makeCalculation());
    }

    Calculator calc2 = new Calculator(6.0, 2.0, '-');
    @Test
    void subtractsTwoNumbers() {
        assertEquals(4.0, calc2.makeCalculation());
    }

    Calculator calc3 = new Calculator(6.0, 2.0, '*');
    @Test
    void multipliesTwoNumbers() {
        assertEquals(12.0, calc3.makeCalculation());
    }

    Calculator calc4 = new Calculator(6.0, 2.0, '/');
    @Test
    void dividesTwoNumbers() {
        assertEquals(3.0, calc4.makeCalculation());
    }
}

Exception handling

try {
    ...
} catch (InputMismatchException ime) {
     System.out.println("invalid input");
} finally {
     if (operator != '+' & operator != '-' & operator != '*' & operator != '/') {
         throw new InputMismatchException();
     }
     Calculator calculator = new Calculator(firstNum, secondNum,    operator);
     ...
}

That's pretty weird.

It makes no sense to continue the computation if the Scanner throws. What is the point of having a finally block that throws the same exception? I would rather let the exception pass through instead of catching and rethrowing (the original exception is likely to contain a more meaningful message, too).

Variable scope

Try and use the narrowest possible scope. For instance, here:

Calculator calc = new Calculator(6.0, 2.0, '+');

@Test
void addsTwoNumbers() {
     assertEquals(8.0, calc.makeCalculation());
}

there's no point in making calc a field. It can (and should) be just a local variable in the addsTwoNumbers() method. The same is true for other calc's and test methods.

Class design

I don't think that the Calculator is a good name for your class. I would expect a calculator to be able to be instantiated once and then evaluate results of different expression. Something like this:

Calculator calculator = new Calculator();
double sum = calculator.evaluateExpression(x, y, '+');
double difference = caclulator.evaluateExpression(x, y, '-');

I'd change it this way or rename this class to Expression.

Object reuse

Creating four new objects for each operation per each expression evaluation can be costly and is unreasonable because the operation are stateless.

You can use a static factory to reuse the same Operation objects in different expressions.


Bitwise operators for permissions I'm trying to create a read-write-execute permissions system for items using user-group-world system.

I already have the user/group system set up, just not the permissions.

This is what I wrote:

/** @param $requestedPerm the permission being requested: read write or execute - 4, 2, or 1 respectively
* @param $level the permission level: world, group, or user
* @param $permInt the permissions checked against:
   (like 754) which is user:read-write-exec, group:read-exec, world:read
*/
function check($requestedPerm,$level,$permInt){
    $digit = ($permInt/$level)%10;
    return ($requestedPerm & $digit)===$requestedPerm;
}

$read = 4;
$write = 2;
$exec = 1;

$world = 1;
$group = 10;
$user = 100;

$bool = check($read,$group,754);

It seems to work how I want it to, but I'm new to bitwise operations, so I'm not too confident.

In my production code, it will be object oriented, but basically it'll work the same.

Is this check() function reliable, as long as I provide the correct $requestedPerm, $level, and $permInt?

Your presentation of your problem indicates that you want to emulate the permission system as used by POSIX like systems (Unix). The three sets (user, group, and world), and three permissions (read, write, execute) are from that. Your actual implementation though is very different....

What you have will work, but is wasteful of bits, and is only a partial bitwise solution.

Consider the permissions 0777 (by the way, the 0 there is supposed to indicate that the value is Octal - did you know that? - see PHP's documentation on Integers)

The 7 permission is the read permission (4), the write permission (2), and the execute permission (1). The important thing is to look at it in binary though:

read  00000100
write 00000010
exec  00000001

If we 'OR' all those permissions, we get:

rwx   00000111

Now, the way things are supposed to work, is that the right-most 3 bits represent the permissions of the world users, the next are the group users, and then the actual user:

user rwx     00000001 11000000
group rwx    00000000 00111000
world rwx    00000000 00000111

The left-most 7 bits are also used on most systems to represent other properties about the file. For example, in Linux, the details of the bits are described in the stat system function (remember, the leading 0 on the numbers means the number is in Octal...!!!):

       S_IRWXU     00700   mask for file owner permissions

       S_IRUSR     00400   owner has read permission
       S_IWUSR     00200   owner has write permission
       S_IXUSR     00100   owner has execute permission

       S_IRWXG     00070   mask for group permissions
       S_IRGRP     00040   group has read permission
       S_IWGRP     00020   group has write permission
       S_IXGRP     00010   group has execute permission

       S_IRWXO     00007   mask for permissions for others
                           (not in group)
       S_IROTH     00004   others have read permission
       S_IWOTH     00002   others have write permission
       S_IXOTH     00001   others have execute permission

The second reminder about the Octal numbers is important. Note that octal numbers go from 0 to 7. This is conveniently 3-bits in binary (and hexadecimal is conveniently 4 binary bits per hex digit).

OK, so, that's how it's normally done. Setting read-write-execute permissions for user, group, and world results in the binary:

00000001 11111111

which is 511 in decimal. Note, if I want to test whether the user has execute permission, it is a simple case of testing with:

00000000 01000000

If the permission is $perm = 0777 (octal) and the test is $userexec = 0100 (execute for user in octal), then we can do:

if (userexec & perm != 0) {
    // user has execute permission
    ....

But, what does your code do? It does:

7 * 100 + 7 * 10 + 7

which is 777 in decimal, and, unfortunately, in binary it is

00000011 00001001

Now, if I want to determine if the user has write-permissions, I have to do divisions and bit-tests to get there (which is what you do, but it's not the way it is supposed to be).

So, with all that background, what should your code look like? First, let's talk about bit shifting...

If I have the bits (in binary):

00000000 00000111

and I then shift them all 3 positions to the left, I will have:

00000000 00111000

I can left-shift bits with the left-shift operator <<. So, the first value there is 0007 (octal), and I can do:

$threebits = 0007;
$threebits = $threebits << 3;

After that shift, $threebits will have the (octal) value 0070.

Note that we need to shift permission bits 0 times for the world permissions, 3 times for group, and 6 times for user.....

$read = 4;
$write = 2;
$exec = 1;

$world = 0;
$group = 3;
$user = 6;

function check($requestedPerm, $level, $permInt){
    return ($requestedPerm << $level) & $permInt != 0;
}

Note that using the above logic we expect the $permInt to be in a different format to what you had.... so, your current code is:

$bool = check($read,$group,754);

But, that will need to change (to add the 0 to make it octal...):

$bool = check($read, $group, 0754);


Queuing web service calls Every call to the web service goes through a custom queuing system. This has a limit that is set to 1. Every call that enters the same time with another call is purged and not handled. Not sure if this is completely thread safe.

public void QueueForExecution(T value)
{
    lock (_queueLock)
    {
        if (_entryCount < _queueLimit)
        {
            Interlocked.Increment(ref _entryCount);
            Logger.Trace(String.Format("Running task queue item for {0}.", _methodName));

            Task.Run(() => RunTask(value));
        }
        else
        {
            lock (_queueItemLock)
            {
                _queuedExecutionValue = value;
            }
            Logger.Trace(String.Format("Task queue limit reached for {0} - queued request - overriding any previous queued items.", _methodName));
        }
    }
}

If any locking on _queueItemLock is happening inside a lock on _queueLock then you can consider this as thread safe.

If any access of _entryCount is done in a lock on _queueLock then you don't need the Interlocked.Increment(ref _entryCount); but you can just increment in the normal way.

If you by any chance are using C# 6.0 (VS 2015) you can use string interpolation by using the $ operator without the need of calling string.Format() like so

Logger.Trace($"Running task queue item for {_methodName}.");

other than that there isn't much to say.


Get radial distance between vector1 and vector2 A small script to calculate total units of distance between two vector entities. Especially useful in games and grid snaps, inventory snaps etc..

typedef struct COORD
{
    int X, Y;
} coord;

int get_dist (coord entity1, coord entity2)
{
    int minuendX    = max(entity1.X, entity2.X);
    int minuendY    = max(entity2.Y, entity2.Y);
    int subtrahendX = min(entity1.X, entity2.X);
    int subtrahendY = min(entity2.Y, entity2.Y);

    return (minuendX - subtrahendX) + (minuendY - subtrahendY);
}

There is a typo in the y components, both times only entity2 is used. I believe that is a copy-paste error.

Your distance is better known as the manhattan distance. I suggest naming the method after it to avoid surprises.

An other method of getting it uses the absolute value of the differences:

int get_manhattan_dist (coord entity1, coord entity2)
{
    return abs(entity1.X - entity2.X) + abs(entity1.Y - entity2.Y);
}

Of course if you have the manhattan distance you can easily provide the euclidean distance

double get_euclid_dist (coord entity1, coord entity2)
{
    double diffX    = entity1.X - entity2.X;
    double diffY    = entity1.Y - entity2.Y;

    return hypot(diffX, diffY);
}


Temperature converter with JavaScript This is one of my first simple javascript projects. Feedback is appreciated! Basically, everytime the select option is changed, the result is shown.

<!DOCTYPE html>

  <meta charset="UTF-8">
  <title>Document</title>

  </select>

    <select onchange="onChange('from'); calculate()" id="selectFrom">
     <option value="Celcius">Celcius</option>
     <option value="Farenheit">Farenheit</option>
     <option value="Kelvin">Kelvin</option>
    </select>
    to
    <select onchange="onChange('to'); calculate()" id="selectTo">
     <option value="Celcius">Celcius</option>
     <option value="Farenheit">Farenheit</option>
     <option value="Kelvin">Kelvin</option>
    </select>

    <input type="text" id="from" onchange="calculate()">  to  <input type="text" id="to">

Some minor observations:

It's a good idea to check your html projects with a validator of some sort (for example: https://validator.w3.org/). There are some slight mistakes in your file which can easily be caught by any validator.
Usually you want to keep content, presentation and behavior separated from another. This means you put scripts, CSS etc. into their own files instead of having them all in your html file.
From a usability perspective I think it would be nicer to have explicit control over when the conversion takes place. E.g. via a button or something similiar.


A polymorphic union in C++ Sometimes I use std::unique_ptr<BaseClass> when all I really want is polymorphism.  For fun, I made this container that skips the heap allocation.  The downside is that it needs to know all possible derived types it will store in order to know how much space to reserve.

Any feedback is appreciated, but I'm especially interested in how this implementation enables/prevents compiler optimizations you would otherwise have with virtual types.

Edit: I am also intersted in supporting constexpr access, but right now the reinterpret_cast prevents constexpr dereference.

#include <algorithm>
#include <type_traits>

template <typename B, size_t Size, size_t Align>
struct PolyUnionSize {
    template <typename D>
    explicit PolyUnionSize(D&& d) noexcept (std::is_nothrow_copy_constructible_v<D>) {
        new(&storage) D(std::forward<D>(d));
    }

    template <typename D>
    PolyUnionSize& operator=(D&& d) noexcept(std::is_nothrow_assignable_v<B, D>) {
        this->~PolyUnionSize();
        *(new(&storage) D) = std::forward<D>(d);
        return *this;
    }

    ~PolyUnionSize() noexcept {
        (*this)->~B();
    }

    B const& operator*() const noexcept {
        return *reinterpret_cast<B const*>(&storage);
    }

    B& operator*() noexcept {
        return *reinterpret_cast<B*>(&storage);
    }

    B const* operator->() const noexcept {
        return reinterpret_cast<B const*>(&storage);
    }

    B* operator->() noexcept {
        return reinterpret_cast<B*>(&storage);
    }

private:
    typename std::aligned_storage<Size, Align>::type storage;
};

constexpr auto max(std::initializer_list<std::size_t> const& t) noexcept {
    return *std::max_element(std::begin(t), std::end(t));
}

template <typename B, typename ... Ds>
struct PolyUnion : public PolyUnionSize<B, max({sizeof(Ds)... }), max({alignof(Ds)... })> {
    using PUSize = PolyUnionSize<B, max({sizeof(Ds)... }), max({alignof(Ds)... })>;

    template <typename D>
    explicit PolyUnion(D&& d) noexcept(std::is_nothrow_copy_constructible_v<D>)
        : PUSize(std::forward<D>(d))
    {
        static_assert((std::is_base_of_v<B, Ds> && ...));
        AssertDinDs<D>();
    }

    template <typename D>
    PolyUnion& operator=(D&& d) noexcept(std::is_nothrow_assignable_v<B, D>) {
        AssertDinDs<D>();
        PUSize::operator=(std::forward<D>(d));
        return *this;
    }

private:
    template <typename D>
    constexpr void AssertDinDs() const& noexcept{
        static_assert((std::is_same_v<D, Ds> || ...));
    }
};

Sample usage:

struct Base {
    virtual int Foo() const { return 1; }
};

struct A : Base {
    int Foo() const final { return 2; }
};

struct B : Base {
    int Foo() const final { return 3; }
};

int main() {
    PolyUnion<Base, A, B> pu(A{});
    pu = B{};
    return pu->Foo();
}

Compiles with -std=c++17 or -std=c++2a

I suggest not codifying the derived types suggested in the type. Only note the ultimate base, size and alignment. Thus you are open to later change.
Add a templated alias to get the proper type from base plus candidates.

Assure that no over-sized object is ever assigned in PolyUnionSize, probably best using SFINAE. No need to defer to the user.

Don't assume constructing the new object will never fail.

PolyUnionSize::PolyUnionSize<class D>(D&& d) and PolyUnionSize::operator=<class D>(D&& d) use perfect forwarding. You don't account for that by using std::decay_t where needed, nor heed it when computing noexcept.

The implicit copy-/move- ctor / assignment are only appropriate for trivial types. And in that case, why override the dtor?

As casts should be used sparingly, consider delegating between op* and op->.


Determine if a path matches a pattern I have been struggling with a regular expression involving path names.  Immediately, this is a bit troublesome, owing to the embedded / in the pattern,  but braces to the rescue.

First the convention I have imposed:

Every path in the set looks like:

/ifmxdev/files/file.0123

but that top directory can take a suffix, for example:

/ifmxdev_test/files/file.8765

The final suffix is exactly 4 digits

After much struggle and an hour of composing this plea, as well as my own thoughts, I came up with a truly ugly but working pattern:

$rawfile_pattern = qr{/ifmxdev[_0-9A-Za-z]*/files/file.\d{4}};
if (! $fname =~ $rawfile_pattern) {....

Now I'd just like some help in making that pattern more elegant, mainly compacting that bracketed section of the regex.

It probably doesn't matter, but the first element of that path can be ifmxdev______ or ifmxdev_x_x_x with that regex.

I suggest you use use the Unicode property alnum, which is [A-Za-z0-9] - \w without the underscore

qr{ \A /ifmxdev (?:_\p{alnum}+)? /files /file\.\d{4} \z }x


Python 'check if number is divisible by 2' program I have written a simple python program which takes a number from the user and checks whether that number is divisible by 2:

# Asks the user for a number that is divisible by 2
# Keep asking until the user provides a number that is divisible by 2.
print 'Question 4. \n'

num = float(raw_input('Enter a number that is divisible by 2: '))

while (num%2) != 0:
    num = float(raw_input('Please try again: '))
print 'Congratulations!'

I would appreciate any feedback on this code, areas for improvement and/or things that I could have done differently.

Well, first of all, you should put code in an if __name__ == '__main__': block to make sure it doesn't run whenever the file is imported.  Thus, just copy pasting:

if __name__ == '__main__':
    print 'Question 4. \n'

    num = float(raw_input('Enter a number that is divisible by 2: '))
    while (num%2) != 0:
        num = float(raw_input('Please try again: '))

    print 'Congratulations!'

The next thing to notice is that a number being even is actually a fairly common check to make.  Such things should be turned into functions.  Mostly copy-pasting again:

def is_even(num):
    """Return whether the number num is even."""
    return num % 2 == 0

if __name__ == '__main__':
    print 'Question 4. \n'

    num = float(raw_input('Enter a number that is divisible by 2: '))
    while not is_even(num):
        num = float(raw_input('Please try again: '))

    print 'Congratulations!'

Notice the docstring:  it's important to document your code, and while the purpose of is_even is obvious, getting into the habit of adding documentation is good.  You could also put the following docstring at the top of the file:

"""Provide the is_even function.

When imported, print prompts for numbers to standard output and read numbers
from standard input separated by newlines until an even number is entered.  Not
entering an even number raises EOFError.

"""

In my opinion, you should get into the habit of programs working on files, not on user input; you'll find that programs are much easier to chain that way.

Possible further improvements are handling the EOFError (catching it and printing something like "No even numbers entered."), splitting the repeating functionality into a main function (questionable use), and maybe adding a read_float function that would be something along the lines of

def read_float(prompt):
    """Read a line from the standard input and return it as a float.

    Display the given prompt prior to reading the line.  No error checking is done.

    """
    return float(raw_input(prompt))

Here too, there is plenty of room for error checking.

I removed the parentheses around (num%2) as I went along; PEP8 discourages these, if I remember correctly.  Either way, it is a very valuable read.


Finding the greatest number from three numbers The user is prompted to enter three numbers. The function finds the largest number and outputs it to the user.

What do you think of my overall program flow and style?

#include <iostream>

using namespace std;

int maxNumber(int num1, int num2, int num3); //function  prototype/declaration

int main() {

int a, b, c, final;

cout << "Enter first number";
cin >> a;
cout << "Enter sec number";
cin >> b;
cout << "Enter third number";
cin >> c;

final = maxNumber(a, b, c);

cout << "Largest: " << final;

cin.get();
return 0;
}

//function def
int maxNumber(int num1, int num2, int num3)
{
    int result;

if (num1 > num2  && num1 > num3) {
    result = num1;
}
else if (num2 > num1  && num2 > num3) {
    result = num2;
}
else if (num3 > num1  && num3 > num2) {
    result = num3;
}
return result;

}

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

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Fix the formatting

I don't know if the indentation problems are a cut-and-paste error or really how your code looks, but it should be fixed.

Avoid using C++ keywords as variable names

The keyword final is being used here as a variable name.  While that's technically not an error, it's not good practice.  I'd suggest using some other name for that.

Simplify your code

The maxNumber function is more complex than it needs to be.  Here's an alternative that minimizes the number of comparisons made:

int maxNumber(int num1, int num2, int num3) {
    if (num1 > num2) {
        return num1 > num3 ? num1 : num3;
    }
    return num2 > num3 ? num2 : num3;
}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Compare two version numbers in the form Major.Minor.Patch.Build to see if one is less than or greater than the other Python 3.7.0

from sys import argv
class Version:
    def __init__(self, ver_str):
        if type(ver_str) != 'string':
            ver_list = ver_str.split('.')
            self.Major = int(ver_list[0])
            self.Minor = int(ver_list[1])
            self.Patch = int(ver_list[2])
            self.Build = int(ver_list[3])

    def __repr__(self):
        return "{}.{}.{}.{}".format(self.Major, self.Minor, self.Patch, self.Build)
    def __lt__(self, other):
        if other.Major != self.Major:
            return self.Major < other.Major
        elif other.Minor != self.Minor:
            return self.Minor < other.Minor
        elif other.Patch != self.Patch:
            return self.Patch < other.Patch

        else:
            return self.Build < other.Build

    def __gt__(self, other):
        if other.Major != self.Major:
            return self.Major > other.Major
        elif other.Minor != self.Minor:
            return self.Minor > other.Minor
        elif other.Patch != self.Patch:
            return self.Patch > other.Patch

        else:
            return self.Build > other.Build

    def printme(self):
        print("{}.{}.{}.{}".format(self.Major, self.Minor, self.Patch, self.Build))

def main(argv):
    validate_args(argv)

    ver_1 = Version(argv[1])

    ver_2 = Version(argv[2])
    op = argv[3]

    if op == '<':
        print("{} < {}: {}".format(ver_1, ver_2, ver_1 < ver_2))
    elif op == '>':
        print("{} > {}: {}".format(ver_1, ver_2, ver_1 > ver_2))
    else:
        print("Incorrect operator")
        exit(-1)

def validate_args(argv):
    no_of_args = len(argv)
    if no_of_args != 4:
        print("USAGE: {} 1.1.1.1 2.2.2.2 '<' or '>'".format(argv[0]))
        exit(-1)
    if (len(argv[1].split('.')) != 4) or (len(argv[2].split('.')) != 4):
        print("USAGE: {} 1.1.1.1 2.2.2.2. '<' or '>' IMPROPER VERSION FORMAT".format(argv[0]))
        exit(-1)
    if argv[3] != '>' and argv[3] != '<':
        print("USAGE: {} 1.1.1.1 2.2.2.2. '<' or '>' IMPROPER OPERATOR".format(argv[0]))
        exit(-1)

if __name__ == '__main__':
    main(argv)

First of all, have you looked at LooseVersion and/or StrictVersion from distutils.version?  They might already provide you with exactly what you need.

General “PEP-8” Comments:

You should have a blank line after your import, after class Version, and after the def __repr__(self) method.  The blank-lines inside of __lt__ and __gt__ functions before the else: clause are inconsistent.

Your class and methods could benefit from """doc strings""".

The class members Major, Minor, Patch and Build should all be lower case.  Only class names should begin with capital letters.  Since the members should be private to the class, they should also be prefixed with an underscore.

The constructor will silently fail if type(ver_str) != 'string' is ever False, and then the instance becomes useless, as the members which are required in every other method of the class become reference errors.  There is no need to check if ver_str is a string; just try to split it, and convert the parts to integers.  If you haven’t passed in a string, the split or conversion to integers will cause an exception.  (You could add a check that the split produces exactly 4 parts, to reject a version such as "1.2.3.4.5.6".)

Fortunately (as noted by @200_success in the comments), type(ver_str) will never equal 'string', because type(...) doesn't return strings; it returns types.  The code only works because the test inverts the condition, and reads != when the intention is for the type to == a string.  The correct test would be closer to if isinstance(ver_str, str):, but again explicit type checking is an anti-pattern.

Without the explicit type checking, and using a more functional approach, the constructor could be written as a one line method.  As a side-effect of the deconstruction assignment, the version string must contain exactly 4 parts or an exception will be raised.

def __init__(self, ver_str):
    self.Major, self.Minor, self.Patch, self.Build = map(int, ver_str.split('.'))

printme() is unused and could be deleted.  If you don’t wish to delete it, at least name it using snake_case to make it more readable: print_me().  You could write the function as simply print(repr(self)) to avoid duplicating code.

__repr__(self) should actually return a string like Version("1.2.3.4"), not the string "1.2.3.4" to conform to the repr() contract.  What you have written would make a great __str__(self) method though, and you could then implement __repr__(self) using the str() method.

Inside def main(...):, you are using argv.  This is a different argv, shadowing the one you import from sys using from sys import argv.

You should avoid shadowing the imported name, by using:

import sys

# ....

if __name__ == '__main__':
    main(sys.argv)

There is no need to exit(-1) from main(), since the program will terminate anyway immediately.  exit() will terminate the Python interpreter, which can be unexpected, and can cause resources to not be properly closed.  A unit test framework will abruptly terminate, preventing recording of the test results, and preventing other unit tests from running.  In short, don’t ever use exit().  Ever.

Since you are explicitly writing for Python 3.7.0, you can use f-strings.  This means you can replace many of your .format(...) calls with a more compact, readable representation where the substitution location and value are combined.  Ie:

    print("{} < {}: {}".format(ver_1, ver_2, ver_1 < ver_2))

could become simply:

    print(f"{ver_1} < {ver_2}: {ver_1 < ver_2}")


Memoized implementation of change-making algorithm Recently, I wrote a simple program to solve the change-making problem. Rather than the tabular dynamic programming solution (described in the link), I tried to write a solution that uses memoization.

usd = [25, 10, 5, 1]

def make_change(W, denoms=usd):
    """
    Solves the change-making problem where W is
    a given amount of money and denoms
    is a list of the denominations available

    >>> make_change(6,[3,1])
    (3, 3)
    """

    d = {}

    def inner(W, sol):
        if W == 0:
            return sol

        if W in d:
            return d[W]

        tmp = []
        for coin in denoms:
            w = W - coin

            if w >= 0:
                tmp.append(inner(w, sol + (coin,)))

        d[W] = min(tmp, key=lambda x : len(x))
        return d[W]
    return inner(W, tuple())

if __name__ == "__main__":
    import doctest
    doctest.testmod()

I'm wondering if there's a better way I should be doing the memoization (i.e. a decorator):

def memoize(f):
    d = {}
    def inner(args):
        if args not in d:
            d[args] = f(*args)
        return d[args]
    return inner

The above code caches the arguments so they don't get recomputed in future computations. Here, however, I think we only want to cache the solution for a given W, and I'm not sure a general purpose decorator like the one above is necessarily correct.

I'd say the main issue with your code is that you're confusing what the inner function is supposed to do:

def inner(W, sol):
    if W == 0:
        return sol   ## return result so far?

    if W in d:
        return d[W]  ## return best result for W?

Those are two different kinds of values. If you're just returning "smallest tuple for weight W", then W == 0 should yield tuple(). And your recursive step should just be:

tmp.append(inner(w) + (coin,))

With that, I would rename tmp. It's not really a temporary variable, we're actually building up our next possible step. So really something like:

next_iteration = (inner(W - coin) + (coin,)
                  for coin in denoms
                  if coin <= W)

And for your key argument to min, you need a function taking one argument and returning a value. You had:

key = lambda x: len(x)

But lambda x: len(x) is the same as len. So I'd reduce the whole thing to:

d = {0: tuple()} # to avoid the special case of 0
def inner(W):
    if W in d:
        return d[W]

    next_iteration = (inner(W - coin) + (coin,)
                      for coin in denoms
                      if coin <= W)
    d[W] = min(next_iteration, key=len)
    return d[W]

return inner(W)

For memoize, sure, with one minor modification:

def memoize(f):
    cache = {}                 # why d?
    def inner(*args):          # <== *args, not args
        if args not in cache:
            cache[args] = f(*args)
        return cache[args]
    return inner

And then just:

@memoize
def inner(W):
    if W == 0:
        return tuple()

    next_iteration = (inner(W - coin) + (coin,)
                      for coin in denoms
                      if coin <= W)
    return min(next_iteration, key = len)

return inner(W)

That does seem nicer. I like it.


Organizing types/namespaces within tree I'm working on a parsing engine as part of a personal project, and I'd like to expose certain .NET APIs to the parsed environment. I put together a very simple tagged union that allowed me to nest types, and put together a few functions that built up a tree of namespaces. Before going further, I decided I needed to refine the types I was using, and came up with this:

type Node =
    | Namespace of string * Node list

    | Class of string * Node list * System.Type
    | Interface of string * Node list * System.Type
    | Structure of string * Node list * System.Type
    | Enumeration of string * (string * Base.AnyInteger) list * System.Type

    | Property of string * System.Reflection.PropertyInfo
    | Field of string * System.Reflection.FieldInfo
    | Event of string * System.Reflection.EventInfo

    | Constructor of System.Reflection.ConstructorInfo
    | Function of string * System.Reflection.MethodInfo
    | Subroutine of string * System.Reflection.MethodInfo

Base.AnyInteger is another tagged union elsewhere that has cases for every .NET integer type except bigint. My expectation with these "Nodes" is that each one (except constructors) will contain their name in the string, a Node list containing any child nodes, and a reference to their System.Type or System.Reflection.___Info to provide additional information when needed. Last, I added the extra empty lines to help mentally align each level of nodes.

After defining a list of constraints that would have to be managed "in logic", I remembered that I'm not working with VB or C#, and that F#'s type system is better than what I'm used to. After about a half hour I came up with this:

type Lvl1 = Namespace of string * (Choice<Lvl1, Lvl2>) list

and Lvl2 =
    | Class of string * Choice<Lvl2, Lvl3, Constructor> list * System.Type
    | Structure of string * Choice<Lvl2, Lvl3, Constructor> list * System.Type
    | Interface of string * Choice<Lvl2, Lvl3> list * System.Type
    | Enumeration of string * (string * Base.AnyInteger) list * System.Type

and Lvl3 =
    | Property of string * System.Reflection.PropertyInfo
    | Field of string * System.Reflection.FieldInfo
    | Event of string * System.Reflection.EventInfo
    | Function of string * System.Reflection.MethodInfo
    | Subroutine of string * System.Reflection.MethodInfo

and Constructor = System.Reflection.ConstructorInfo

This new set of types correctly describes what each node can contain, using the type system to enforce everything rather than just implying how the levels are arranged with empty lines. Now nodes from Lvl1 can contain children from Lvl1 or Lvl2, nodes from Lvl2 can contain children from Lvl2 or Lvl3 (Classes and Structures may also contain Constructors), and nodes at Lvl3 cannot contain any children.

Now that I've used the type system to make sure I can't accidentally add a Namespace node inside a Class node, are there any other obvious changes I should be making here? I'll also admit I was originally using list references for each list of child nodes, because it made it easier to reason about in an imperative way; I've since revisited my code and have removed ref and started making use of map/fold/etc.

type Node

I agree with you that this is too weakly typed.

type Lvl1
and Lvl2
and Lvl3

I don't like the naming here. The name Lvl2 doesn't really say much. Better names would be something like Namespace, Type and Member (or maybe TypeMember).

Choice<Lvl2, Lvl3, Constructor>

I think that constructor is logically a type member, just like a property or function. So I would make Constructor another case in the Lvl3 (or Member) union. I think making the model simpler is worth having the possibility of a constructor in an interface.

With that change, struct, class and interface look exactly the same, so I would refactor it further by combining the three cases into one (called something like ComplexType), and distinguishing between them by an additional field (something like ComplexTypeKind).

Also, I would encode the possibility of a nested type by adding a case to Lvl3 (a.k.a. Member). But if you keep your current design, you should remove Lvl2 from the Interface case, because interfaces can't have nested types.

System.Reflection.PropertyInfo

There is no need to repeat the namespace name, you can open it.

Property of string * System.Reflection.PropertyInfo

Why do you keep the name, when it's already accessible through the PropertyInfo? This question applies to most of your cases.


Filtering out illegal moves from a map of available moves I'm writing a console blackjack game.

To hold all the possible moves a player can make, I have a map that maps a keyboard key to a move keyword:

(def move-map {\s :stand
               \h :hit
               \p :split
               \d :double})

Splitting and doubling are only available options during the first move though, so I'd like to filter them out of the available moves before verifying the user's input if they aren't legal moves.

My current method is to indicate which are legal using a flag for doubling and one for splitting, then filtering the map if a flag isn't set:

(defn- filter-moves [moves can-split? can-double?]
  (let [r-split (if can-split? moves (filter #(not= :split (second %)) moves))
        r-doub (if can-double? r-split (filter #(not= :double (second %)) r-split))]
    (into {} r-doub)))

The problem is, this contains a fair amount of redundancy. It should only iterate the map once because filter is lazy, but I'm still not happy with this solution.

Usage:

(filter-moves move-map true false)

=> {"s" :stand, "h" :hit, "p" :split}

Any improvement suggestions would be appreciated. I'm not against redesigning the function if it helps readability and maintainability.

You can iterate once by using remove in combination with a set of illegal moves:

(defn- filter-moves [moves can-split? can-double?]
  (let [illegal? (cond-> #{}
                   (not can-split?) (conj :split)
                   (not can-double?) (conj :double))]
    (into {} (remove #(illegal? (second %)) moves))))

What's more, If I understand correctly that moves is the map mapping keys to moves. In my opinion, this map has no place in your core game logic: have your filter-moves function manipulate only sets of moves and keep any notion of user input at the edge of your system.